[WIP] Improve package check git logging and workaround windows issue

[dekellum]

This is a second attempt at diagnostic logging leading to an eventual clear workaround for #5823.

This is a Work In Progress. Without a root-cause fix (in git2-rs or elsewhere), this is currently expected to fail tests on windows (and windows-only), but in informative ways.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (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.

[dekellum]

At 2c22de1 I have improved (try 2) logging that I think shows the fundamental issue in CI on windows:

https://ci.appveyor.com/project/rust-lang-libs/cargo/build/1.0.5379/job/1ptroibkr91klwvg#L1660

---- package::package_verbose stdout ----
[...]
Expected: execs
but: differences:
2 - |[PACKAGING] a v0.0.1 ([..])|

• |warning: No (git) Cargo.toml found at (a\Cargo.toml)|

Where as all tests succeed on not(windows): https://travis-ci.org/rust-lang/cargo/builds/410962431

Compare the windows failure above to the RUST_LOG=debug output on my local linux:

DEBUG 2018-08-01T19:51:47Z: cargo::ops::cargo_package: found a git repo at "/home/david/src/cargo/target/cit/t0/all/"
DEBUG 2018-08-01T19:51:47Z: cargo::ops::cargo_package: found (git) Cargo.toml at "Cargo.toml"

In this latter, success path DEBUG output, the path (derived from trimming the package workdir prefix off of the manifest_path) is correctly computed as "Cargo.toml". In the former failing windows console warnings, the same path is shown as "a\Cargo.toml", with workdir prefix not correctly removed.

[dekellum]

Being new to the project I hadn't noticed an important aspect of the appveyor setup until now. The two windows builds are not GNU vs MSVC, but rather a minimal-versions build vs latest-versions. I've been ignoring the fact that the minimal-versions build has been succeeding, while the latest-versions build has been the one failing!

Minimal Versions (SUCCESS)

• git2 v0.7.3
• libgit2-sys v0.7.6
• [...]

Latest Versions (FAILS)

• git2 v0.7.4
• libgit2-sys v0.7.7
• [...]

Now (just with cargo 28.0) there are plenty of other dependencies different between minimal and latest, see: https://deps.rs/crate/cargo/0.28.0

The ignore crate might also be important here? And joy, there are no release notes and no release tags for that crate between versions 0.4.0 (minimal) and 0.4.3. Oh, the irony, given I got here via #5786. These differences could be incrementally CI tested, if I have the stamina.

(edit: splitting remainder to comment below, for clarity)

[Eh2406]

The minimal version job on CI just runs cargo check at present. So I suspect it would fail a cargo test as well.

[dekellum]

That is also quite surprising, but I can confirm it. Thanks for the time saving tip, @Eh2406. Unfortunately it doesn't exclude the possibility that this is a dependency regression, it just invalidates the above evidence. Sigh this task would be much more suited to someone with a windows development setup. Here they could just take this branch with a custom lock file to test recent but older versions.

[Eh2406]

I am a windows based life form. If you give me cleare instructions I can help.

[dekellum]

Narrowly regarding the CI setup, I can read through the lines that there must be some failing due to complications with actually building and running tests for the minimal versions, but can Appveyor not be setup in a similar fashon to Travis with a minimal-version + test build config that is allowed to fail (e.g. MAY_FAIL), without failing the entire set of Travis/Appveyor builds? That would be helpful in this instance at least.

Edit: further info https://www.appveyor.com/docs/build-configuration/#allow-failing-jobs

Has this been considered? @dwijnand?

[dekellum]

With latest additional verbosity of 5fc271f, windows (git2 v0.7.4 libgit2-sys v0.7.7) fails with:

https://ci.appveyor.com/project/rust-lang-libs/cargo/build/1.0.5383/job/71165ym65al26lpo#L1660

warning: No (git) Cargo.toml found at a\Cargo.toml in workdir C:/projects/cargo/target/cit/t872/all/, manifest C:\projects\cargo\target\cit\t872\all\a\Cargo.toml

Being in the enviable position of testing this via Appveyor CI with ~20 minute build delay, I don't have the full debug output and I'm reluctant to hack that into CI via a commit. As poor substitute, RUST_LOG=debug output on my local linux:

DEBUG [..] found (git) Cargo.toml at "Cargo.toml" in workdir "/home/david/src/cargo/target/cit/t0/all/", manifest "/home/david/src/cargo/target/cit/t0/all/Cargo.toml"

Given the above windows output, I don't think the root cause it just the mix of forward and back slash paths. The calculated repo-relative-path a\Cargo.toml is off by one—it should be Cargo.toml as on linux success case (DEBUG output). Is the one-off due somehow to the extra C: path element on windows?

@alexcrichton @ehuss @Eh2406 can you look at this latest windows output above and let me know if anything comes to mind, either for fixing in git2-rs or as a workaround here with current versions? Thanks!

[dwijnand]

this task would be much more suited to someone with a windows development setup

It might be beneficial to have a little Windows VirtualBox to iterate on.

[Eh2406]

I will run tests locally and investigat, when my rustup update finishes.

[Eh2406]

I was able to repro locally! I was able to make the test pass with the addition of let path = path.to_string_lossy().replace('\\', "/"); let path = Path::new(&path);. I was also able to get it to pass with a patch-section to use https://github.com/alexcrichton/git2-rs/pull/341.

So It looks like it is just the \ vs / thing!

[dekellum]

Great thanks @Eh2406! I think your workaround is better than the one I originally, haphazardly found in #5820. But if alexcrichton/git2-rs#341 is close to being merged/released then it may not be worth applying that?

Also @Eh2406, since you saw my changes, would you be supportive of me pursuing a final PR for the logging+test improvements on this here? As in 65ded47, so its not quite so verbose. Any feedback?

[Eh2406]

https://github.com/alexcrichton/git2-rs/issues/334 had a 2 day tern around time, from my PR until a new version was out. So, I'd guess we can whate for it to land.

I generally like more logging! But, this is not a part of the code where I am expert enough to give advice.

[dekellum]

Closing in preference to #5858.