Improve verbose console and log for finding git repo in package check #5858
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
@alexcrichton this is ready for review and merge. Tests passing with new git2 0.7.5 release. |
|
Thanks for the PR! Just to make sure I understand, there's one new warning for publishing without a crate in git, and there's a different warning for if you're publish a crate where it's ignored in the nearest git repo. The tests then print out this warning because they're in the ignored If so, could these warnings perhaps only be enabled for Cargo's own tests suite? I don't think we want to start warning in general about not having a crate in git at all (or being ignored), as there's not much a user can do to squelch the warnings. Additionally, we run Cargo's tests suite in the rust-lang/rust repo which futzes a bit with the |
|
That's correct on the two warnings: Note that one run of Given that And yes of course part of the motivation was tying into the testsuite's checking of verbose output, to effectively improve coverage. Regarding running cargo's testsuite from rust-lang/rust, sounds like this would have come up for #5786 as well. I'm a little afraid to ask, but is that something I could easily enough try, in short order without having to build rust entirely? Given windows, is there some way to get rust-lang/rust CI to check this, against this branch? |
|
Ah ok thanks for the explanation! I missed the fact that it was verbose-only, and I think it makes sense to have it only in verbose mode for now. Let's go ahead and land this and we can always fixup the breakage later if it comes up in rust-lang/rust, thanks again! @bors: r+ |
|
📌 Commit fcd86f3 has been approved by |
…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!
|
☀️ Test successful - status-appveyor, status-travis |
Update cargo - Update transitioning url (rust-lang/cargo#5889) - Resolve some clippy lint warnings (rust-lang/cargo#5884) - Don't kill child processes on normal exit on Windows (rust-lang/cargo#5887) - fix a bunch of clippy warnings (rust-lang/cargo#5876) - Add support for rustc's --error-format short (rust-lang/cargo#5879) - Support JSON with rustdoc. (rust-lang/cargo#5878) - Fix rustfmt instructions in CONTRIBUTING.md (rust-lang/cargo#5880) - Allow `cargo run` in workspaces. (rust-lang/cargo#5877) - Change target filters in workspaces. (rust-lang/cargo#5873) - Improve verbose console and log for finding git repo in package check (rust-lang/cargo#5858) - Meta rename (rust-lang/cargo#5871) - fetch: skip target tests when cross_compile is disabled (rust-lang/cargo#5870) - Fully capture rustc and rustdoc output when -Zcompile-progress is passed (rust-lang/cargo#5862) - Fix test --example docs. (rust-lang/cargo#5867) - Add a feature to build a vendored OpenSSL (rust-lang/cargo#5865)
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
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!