Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

No git repo found on windows (only) for package_verbose test #5823

Closed
dekellum opened this issue Jul 28, 2018 · 3 comments · Fixed by #5858
Closed

No git repo found on windows (only) for package_verbose test #5823

dekellum opened this issue Jul 28, 2018 · 3 comments · Fixed by #5858

Comments

@dekellum
Copy link
Contributor

This was discovered with testing for #5786. See that discussion. At present on master, the package::package_verbose test is not engaging, on windows only, the check_not_dirty function of cargo_package for a sub-package, even though the sub-package is in a git repo. On other platforms the (parent) repo is found and the dirty check occurs.
@dekellum
Copy link
Contributor Author

Assign to @dekellum.

@dekellum
Copy link
Contributor Author

Wondering if this relates to alexcrichton/git2-rs#334

This was referenced Jul 28, 2018
Closed
Closed
@dekellum
Copy link
Contributor Author

dekellum commented Jul 30, 2018 •
edited
Loading

According to @ehuss (in #5820), alexcrichton/git2-rs#334 is the root cause.

This was referenced Aug 1, 2018
Closed
Merged
bors added a commit that referenced this issue Aug 7, 2018
@bors
Auto merge of #5858 - dekellum:git-check-logging-and-test, r=alexcric…
0e7a46e 
…hton

Improve verbose console and log for finding git repo in package check

Third attempt to resolve #5823 by improving logging and tests. This exposes the issue to testing,  via verbose console output and is dependent on alexcrichton/git2-rs#341 as just released in git2 0.7.5 crate. Thus tests *should* now pass on all platforms, incl. windows, but I also intend to bump the minimal git2 release dependency (in a subsequently added commit).

cc: @Eh2406 thanks for your fix and help!
@bors bors closed this as completed in #5858 Aug 7, 2018
@dekellum dekellum mentioned this issue Aug 13, 2018
Merged
bors added a commit that referenced this issue Aug 20, 2018
@bors
Auto merge of #5886 - dekellum:record-package-git-head-3, r=alexcrichton
0ec7281 
Generate .cargo_vcs_info.json and include in `cargo package` (take 2)

Implements #5629 and supersedes #5786, with the following improvements:

* With an upstream git2-rs fix (tracked #5823, validated and min version update in: #5858), no longer requires windows/unix split tests.

* Per review in #5786, drops file system output and locks for .cargo_vcs_info.json.

* Now uses serde `json!` macro for json production with appropriate escaping.

* Now includes a test of the output json format.

Possible followup:

* Per discussion in #5786, we could improve reliability of both the VCS dirty check and the git head sha1 recording by preferring (and/or warning otherwise) the local repository bytes of each source file, at the same head commit. This makes the process more appropriately like an atomic snapshot, with no sentry file or other locking required.  However given my lack of a window's license and dev setup, as exhibited by troubles of #5823, this feel intuitively like too much to attempt to change in one iteration here.  I accept the "best effort" concept of this feature as suggested in #5786, and think it acceptable progress if merged in this form.

@alexcrichton r?
@joshtriplett cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
1 participant
@dekellum