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

Update cc-rs to 1.0.73 for compiler + bootstrap #99477

Closed
wants to merge 1 commit into from

Conversation

dpaoliello
Copy link
Contributor

WARNING: This may be considered a breaking change due to rust-lang/cc-rs@8858713 - specifically, cc-rs now correctly detects that it's running in a Visual Studio Command Prompt and will use the ambient LIB path rather than trying to auto-detect it.

Other improvements that may be valuable for the Rust Compiler:

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 19, 2022
@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2022
@ChrisDenton
Copy link
Member

Support for detecting the Windows SDK from environment variables

This might also allow a better replacement for the hack introduced in #88797, Although properly fixing #88796 would be better still.

@Mark-Simulacrum Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 23, 2022
@Mark-Simulacrum
Copy link
Member

r? @wesleywiser to make a call on whether this merits a T-compiler(?) FCP given the possible breakage, and in general since I'm not really clear on what the implications of such a change are.

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are basically two cases here:

  1. rustc is invoked inside of a Developer Command Prompt for VS environment
  2. rustc is invoked in any other console/environment

From what I've seen, scenario 2 is the more common one and that isn't affected by the bug fix. In scenario 1, it's completely unexpected that we would use a different version of link.exe than the one in the Developer Command Prompt environment so while it is possible this could change which linker is invoked, that seems unlikely in practice and the new behavior aligns much more closely with what users would expect.

@wesleywiser
Copy link
Member

Since this is technically a user-facing change, I think we should FCP it.

Summary

This PR updates cc-rs to a newer version which contains a bug fix that causes the code which locates the msvc linker to now respect some environment variables set by vcvars which is used by the Visual Studio Developer Command Prompt to configure which compilers/linkers/other tools are used by default in the shell session. This code has existed within cc-rs for a while but never ran because of a simple bug.

As a result, there is the small potential for a user-facing change if a user was running rustc inside a VS Developer Command Prompt but we had found a different (probably newer) version of the linker than the one that is normally used in that environment.

I think the likelihood of this actually breaking someone is small and outweighed by the benefits of making rustc use the expected linker within the VS Developer Command Prompt environment.

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 27, 2022

Team member @wesleywiser has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 27, 2022
@wesleywiser
Copy link
Member

It might be good to land this earlier in a release cycle so that we have the maximum amount of time to detect any unexpected fallout from this (as I said above, I think this is the correct thing to do and there isn't likely to be a negative impact here, but better safe than sorry).

@cjgillot @estebank @nikomatsakis @pnkfelix please let me know if you have questions/concerns we should address. Thanks!

@klensy
Copy link
Contributor

klensy commented Aug 12, 2022

But it already 1.0.73, it was bumped in #100117 ?

@wesleywiser
Copy link
Member

Thanks for heads up @klensy! It's unfortunate this already landed in such a nondescript PR but consensus looks to be that everyone is ok with it.

@rfcbot cancel

@dpaoliello If you think we should still update the Cargo.toml files, I can r+ otherwise I guess we can just close this PR.

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 12, 2022

@wesleywiser proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 12, 2022
@dpaoliello
Copy link
Contributor Author

@wesleywiser #100117 only updates bootstrap - it doesn't change how rustc discovers the environment.

As such, we know have an inconsistency: when building Rust, bootstrap will be aware of the ambient VS Command Prompt (and be able to use VS 2022), but the rustc it generates and uses will not be aware of these...

@klensy
Copy link
Contributor

klensy commented Aug 12, 2022

@wesleywiser #100117 only updates bootstrap - it doesn't change how rustc discovers the environment.

As such, we know have an inconsistency: when building Rust, bootstrap will be aware of the ambient VS Command Prompt (and be able to use VS 2022), but the rustc it generates and uses will not be aware of these...

Nope, not bootstrap (title says bootstrap, but actually it isn't):

rust/Cargo.lock

Lines 520 to 527 in 0068b8b

[[package]]
name = "cc"
version = "1.0.73"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "2fff2a6927b3bb87f9595d67196a70493f627687a71d87a0d692242c33f58c11"
dependencies = [
"jobserver",
]

[[package]]
name = "cc"
version = "1.0.73"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "2fff2a6927b3bb87f9595d67196a70493f627687a71d87a0d692242c33f58c11"

@dpaoliello
Copy link
Contributor Author

@wesleywiser #100117 only updates bootstrap - it doesn't change how rustc discovers the environment.
As such, we know have an inconsistency: when building Rust, bootstrap will be aware of the ambient VS Command Prompt (and be able to use VS 2022), but the rustc it generates and uses will not be aware of these...

Nope, not bootstrap (title says bootstrap, but actually it isn't):

Huh, I don't understand how this works then: if the lock file is moved to a later version, but the toml files weren't changed...

@wesleywiser
Copy link
Member

wesleywiser commented Aug 12, 2022

IIRC this is one of the big differences between NPM style version constraints and Rust/Cargo ones. In NPM, foo = "1.0.0" means exactly the 1.0.0 version. In Cargo, foo = "1.0.0" means "any version 1.0.0 or greater that isn't 2.0". In essence, Cargo's foo = "bla" is NPM's foo = "^bla".

https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-cratesio

This means the version of cc we will use is 1.0.73 as specified in the lock file because it matches the constraint from the Cargo.toml file of 1.0.69 or greater.

@dpaoliello dpaoliello closed this Aug 12, 2022
@dpaoliello dpaoliello deleted the cc branch August 12, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants