Skip to content

Rebuild on changes to the deployment target when compiling Apple targets - #129342

Closed
madsmtm wants to merge 1 commit into
rust-lang:mainfrom
madsmtm:track-deployment-target
Closed

madsmtm wants to merge 1 commit into
rust-lang:mainfrom
madsmtm:track-deployment-target

Conversation

@madsmtm

@madsmtm madsmtm commented Aug 21, 2024 •

Copy link
Copy Markdown
Member

View all comments

Rebuild, both in Cargo and the incremental cache, when the *_DEPLOYMENT_TARGET environment variables change.

Fixes #118204. See #129432 for more motivation behind this change.

I am a bit unsure if I have implemented this correctly, I'm particularly uncertain whether creating a new field in Options the correct approach for busting the incremental cache? It's only the codegen that needs to be re-run, everything before that shouldn't be influenced by this, so if there's a way to make that happen, we should do that instead.

Builds upon #130068.

r? thomcc

CC @BlackHoleFox
@rustbot blocked

@rustbot rustbot added O-apple Operating system: Apple / Darwin (macOS, iOS, tvOS, visionOS, watchOS) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 21, 2024
@jieyouxu

Copy link
Copy Markdown
Member

I'd also like some input on how to add a test for these kinds of incremental compilation changes?

If the incremental test suite doesn't suit your needs, you could fallback to a run-make test if needed.

@madsmtm
madsmtm force-pushed the track-deployment-target branch from c9de51d to ed9370b Compare August 21, 2024 13:23
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Aug 21, 2024
@madsmtm

madsmtm commented Aug 21, 2024

Copy link
Copy Markdown
Member Author

I'd also like some input on how to add a test for these kinds of incremental compilation changes?

If the incremental test suite doesn't suit your needs, you could fallback to a run-make test if needed.

Thanks for the tip, I took a look at the incremental test suite, but ended up creating a run-make test instead. Note that this change is going to conflict with #128507, since I updated the test from in there.

@rust-log-analyzer

This comment has been minimized.

@madsmtm
madsmtm force-pushed the track-deployment-target branch from ed9370b to dc8b9f9 Compare August 21, 2024 13:37
@rust-log-analyzer

This comment has been minimized.

@madsmtm
madsmtm force-pushed the track-deployment-target branch from dc8b9f9 to fb6ccd3 Compare August 21, 2024 15:23
@rust-log-analyzer

This comment has been minimized.

@madsmtm
madsmtm force-pushed the track-deployment-target branch from fb6ccd3 to a45f61f Compare August 21, 2024 15:31
@jieyouxu

Copy link
Copy Markdown
Member

Note that this change is going to conflict with #128507, since I updated the test from in there.

I'll discuss this with @Oneirical, if this PR is updating the macos deployment test we might as well drop the port in #128507.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu self-assigned this Aug 21, 2024
Comment thread compiler/rustc_driver_impl/src/lib.rs Outdated
Comment thread compiler/rustc_interface/src/passes.rs
Comment thread compiler/rustc_interface/src/passes.rs
Comment thread compiler/rustc_target/src/spec/base/apple/mod.rs Outdated
Comment thread tests/run-make/apple-deployment-target/rmake.rs
Comment thread tests/run-make/apple-deployment-target/rmake.rs Outdated
@madsmtm
madsmtm force-pushed the track-deployment-target branch from a45f61f to 41c0b45 Compare August 21, 2024 17:23
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 21, 2024
@madsmtm
madsmtm force-pushed the track-deployment-target branch from 41c0b45 to c945329 Compare August 21, 2024 18:32
@madsmtm
madsmtm force-pushed the track-deployment-target branch from c945329 to 23bd0d5 Compare August 21, 2024 18:56
@bors

This comment was marked as resolved.

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 10, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 1, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? `@petrochenkov`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ```@petrochenkov```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ````@petrochenkov````
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? `````@petrochenkov`````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ``````@petrochenkov``````
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
Rollup merge of rust-lang#131037 - madsmtm:move-llvm-target-versioning, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ``````@petrochenkov``````
@bors

bors commented Nov 2, 2024

Copy link
Copy Markdown
Collaborator

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

@Dylan-DPC

Copy link
Copy Markdown
Member

@madsmtm any updates on this? thanks

@Dylan-DPC Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 10, 2024
@petrochenkov

Copy link
Copy Markdown
Contributor

This is currently blocked on #130883.
@rustbot blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 10, 2024
TaKO8Ki added a commit to TaKO8Ki/rust that referenced this pull request Mar 26, 2025
Add environment variable query

Generally, `rustc` prefers command-line arguments, but in some cases, an environment variable really is the most sensible option. We should make sure that this works properly with the compiler's change-tracking mechanisms, such that changing the relevant environment variable causes a rebuild.

This PR is a first step forwards in doing that.

Part of the work needed to do rust-lang#118204, see rust-lang#129342 for some discussion.

r? `@petrochenkov`
Zalathar added a commit to Zalathar/rust that referenced this pull request Mar 27, 2025
Add environment variable query

Generally, `rustc` prefers command-line arguments, but in some cases, an environment variable really is the most sensible option. We should make sure that this works properly with the compiler's change-tracking mechanisms, such that changing the relevant environment variable causes a rebuild.

This PR is a first step forwards in doing that.

Part of the work needed to do rust-lang#118204, see rust-lang#129342 for some discussion.

r? ``@petrochenkov``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2025
Rollup merge of rust-lang#130883 - madsmtm:env-var-query, r=petrochenkov

Add environment variable query

Generally, `rustc` prefers command-line arguments, but in some cases, an environment variable really is the most sensible option. We should make sure that this works properly with the compiler's change-tracking mechanisms, such that changing the relevant environment variable causes a rebuild.

This PR is a first step forwards in doing that.

Part of the work needed to do rust-lang#118204, see rust-lang#129342 for some discussion.

r? ``@petrochenkov``
@petrochenkov

Copy link
Copy Markdown
Contributor

#130883 has landed, unblocking.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 14, 2025
@rustbot

rustbot commented Apr 14, 2025

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@Dylan-DPC

Copy link
Copy Markdown
Member

@madsmtm this is no longer blocked so any updates on this?

christian-schilling pushed a commit to christian-schilling/rustc_codegen_cranelift that referenced this pull request Jan 27, 2026
…ochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang/rust#130883 to do rust-lang/rust#118204. See also rust-lang/rust#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang/rust#130435.

r? ``````@petrochenkov``````
christian-schilling pushed a commit to christian-schilling/rustc_codegen_cranelift that referenced this pull request Jan 27, 2026
…ochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang/rust#130883 to do rust-lang/rust#118204. See also rust-lang/rust#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang/rust#130435.

r? ``````@petrochenkov``````
@JohnCSimon

Copy link
Copy Markdown

@madsmtm
Ping from triage: This has been open for a bit. I'm closing this as inactive, please reopen when you're ready to continue

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Sep 27, 2026
@rustbot rustbot added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc O-apple Operating system: Apple / Darwin (macOS, iOS, tvOS, visionOS, watchOS) S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. 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.

Rebuild if deployment target changes

9 participants