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.

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

Removing hash from output files when using MSVC #7400

Merged
merged 1 commit into from
Oct 2, 2019
Merged

Conversation

snf
Copy link
Contributor

@snf snf commented Sep 21, 2019

This is the fix for #7358 suggested by @alexcrichton . Tested and working.

I see a few tests failling but seem unrelated. I'll investigate them tomorrow.

@rust-highfive
Copy link

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2019
@snf
Copy link
Contributor Author

snf commented Sep 21, 2019

Failing tests are:

  • build::uplift_pdb_of_bin_on_windows
  • collisions::collision_example
  • collisions::collision_export
  • freshness::changing_bin_features_caches_targets

Will dig into the issues later.

@alexcrichton
Copy link
Member

Thanks for the PR here @snf! This is technically a breaking change for MSVC, which is why some tests are failing. To explain a bit more with the purpose of those hashes, the intention is that if you do a sequence of commands like:

$ cargo build --features a
$ cargo build --features b
$ cargo build --features a

The last command ideally should have everything fully cached. To do that we tell the compiler that for --features a and --features b we actually produce entirely distinct executables (different hash blobs at the end). What this change is doing is saying that specifically for MSVC executables we don't produce a big "hash blob" and we instead natively generate the final executable. Typically Cargo just hard-links the final executable to the actual name (like foo.exe instead of foo-xxxxxx.exe). For some platforms (as listed in this if statement) the final executable depends on the filename that it produces, so we can't do this trick and we have to actually produce the final executable.

When I say "breaking change" though that's still fine, this is only breaking in that the exact behavior of Cargo is changing, but it's unlikely to affect too too many folks. We could also fix this behavior in time I believe, just emitting foo.exe to a unique output directory and caching that. In any case, I think it's fine to just update the tests here and get them passing with respect to this change!

As for each test:

  • build::uplift_pdb_of_bin_on_windows - this I'm not entirely sure what's going on, so some investigation would be needed. We likely just need a tweaked assertion though.
  • collisions::collision_example - this is fine to just ignore the test on MSVC now, or just update it to have different assertions on MSVC
  • collisions::collision_export - similar to above, this should be fine to ignore and/or have target-specific assertions
  • freshness::changing_bin_features_caches_targets - again like above, this is fine to ignore on MSVC since its rules for executables are different

@snf
Copy link
Contributor Author

snf commented Sep 24, 2019

Thanks for the detailed explanation Alex! A lot of assumptions ahead, please verify them.

I'm thinking that the problem is that the linker will generate the internal names to match the output files. If we want the file names with hashes in the intermedary directory, it could be possible to link foo.exe there and then move it to foo-hash.exe. The internal names, then, will all have foo.exe without the hashes. And everything else should be the same without breaking changes right?

If this works, I'm not even sure where should I start but I can give a shot at it next week. Also, confirm if this might be a too complex solution.

@alexcrichton
Copy link
Member

That... I think could work! It'd be a bit wonky though because we would generate foo.exe, then copy/link it to foo-xxxxx.exe when we actually build something. If we determine it's fresh, though, we'd copy foo-xxxxx.exe to foo.exe (as well as the pdb files and such). I think that should work in terms of using the right caches and such, but it may not be the easiest to implement with the way Cargo is written today. In any case I'm fine either updating the test cases or updating the strategy here, whichever you would prefer!

@snf
Copy link
Contributor Author

snf commented Sep 24, 2019

Thanks! Given my current load, I'll work on fixing the failling tests and I can check for the "better" solution later when I have more cycles available.

@bors
Copy link
Contributor

bors commented Sep 26, 2019

☔ The latest upstream changes (presumably #7425) made this pull request unmergeable. Please resolve the merge conflicts.

@snf snf marked this pull request as ready for review October 2, 2019 05:08
@snf
Copy link
Contributor Author

snf commented Oct 2, 2019

Fixed the tests. Please let me know if they look ok. Commits squashed.

@alexcrichton
Copy link
Member

@bors: r+

👍

@bors
Copy link
Contributor

bors commented Oct 2, 2019

📌 Commit e7c5579 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2019
@bors
Copy link
Contributor

bors commented Oct 2, 2019

⌛ Testing commit e7c5579 with merge 70c335a8f7119f9b64799453fa51f1a6f266e228...

@bors
Copy link
Contributor

bors commented Oct 2, 2019

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 2, 2019
@alexcrichton
Copy link
Member

@bors: retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2019
bors added a commit that referenced this pull request Oct 2, 2019
Removing hash from output files when using MSVC

This is the fix for #7358 suggested by @alexcrichton . Tested and working.

I see a few tests failling but seem unrelated. I'll investigate them tomorrow.
@bors
Copy link
Contributor

bors commented Oct 2, 2019

⌛ Testing commit e7c5579 with merge 9f82a74...

@bors
Copy link
Contributor

bors commented Oct 2, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 9f82a74 to master...

@bors bors merged commit e7c5579 into rust-lang:master Oct 2, 2019
@ehuss
Copy link
Contributor

ehuss commented Oct 4, 2019

(Sorry, I'm a bit behind on reviews.)

I'm concerned this changed the semantics of the --out-dir feature. Examples on MSVC are no longer possible to export to a directory. The uplift code uses the hardlink value to know whether or not something should be exported. But since examples are no longer hard linked, they won't be exported. Can that behavior be retained? (This is relevant to the collision_export test.)

@alexcrichton
Copy link
Member

@ehuss I'm not sure I quite understand, but it sounds like I'm missing some consequences of switching to using no metadata here perhaps?

bors added a commit to rust-lang/rust that referenced this pull request Oct 8, 2019
Update Cargo

To pull rust-lang/cargo#7482

List of merged PRs:
- Fix wrong directories in PATH on Windows (rust-lang/cargo#7482)
- Update SPDX list to 3.6 (rust-lang/cargo#7481)
- Mark Emscripten's .wasm files auxiliary (rust-lang/cargo#7476)
- Update `curl-sys` dependency requirement (rust-lang/cargo#7464)
- add dependencies for `pkg-config` (rust-lang/cargo#7443)
- Removing hash from output files when using MSVC (rust-lang/cargo#7400)
- Disable preserving mtimes on archives (rust-lang/cargo#7465)
- Removed redundant borrow (rust-lang/cargo#7462)
- Public dependency refactor and re-allow backjumping (rust-lang/cargo#7361)
- unify the quote in Cargo.toml (rust-lang/cargo#7461)
@ehuss
Copy link
Contributor

ehuss commented Oct 8, 2019

I filed #7493 with some more detail explaining the scenario.
Also filed #7492 to re-enable some tests here.

bors added a commit that referenced this pull request Oct 8, 2019
Re-enable some MSVC tests.

These tests were disabled for MSVC in #7400, but I would prefer to keep them running.
@ehuss ehuss added this to the 1.40.0 milestone Feb 6, 2022
@dnbln dnbln mentioned this pull request Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants