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

chore: Do not bust Rust build cache when opening projects with dev build #26278

Merged
merged 2 commits into from
Mar 7, 2025

Conversation

osiewicz
Copy link
Contributor

@osiewicz osiewicz commented Mar 7, 2025

Problem

Running cargo run . twice in Zed repository required a rebuild two times in a row. The second rebuild was triggered around libz-sys, which in practice caused a rebuild of the ~entire project.

Some concrete examples:

cargo test -p project # Requires a rebuild (warranted)
cargo run . # Requires a rebuild (warranted)
cargo test -p project # Requires a rebuild (unwarranted)

or

cargo run . # Requires a rebuild (warranted)
cargo run . # Requires a rebuild (unwarranted)

What's going on

Zed build script on MacOS sets MACOSX_DEPLOYMENT_TARGET to 10.15. This is fine. However, cargo propagates all environment variables to child processes during cargo run. This then affects Rust Analyzer spawned by dev Zed - it clobbers build cache of whatever package it touches, because it's behavior is not same between running it with cargo run (where MACOS_DEPLOYMENT_TARGET gets propagated to child Zed) and running it directly via target/debug/zed or whatever (where the env variable is not set, so that build behaves roughly like Zed Dev.app).

Solution

We'll unset that env variable from user environment when we're reasonably confident that we're running under cargo run by exploiting other env variables set by cargo: https://doc.rust-lang.org/cargo/reference/environment-variables.html CARGO_PKG_NAME is always set to zed when running it via cargo run, as it's the value propagated from the build.

The alternative I've considered is running via a custom runner, though the problem here is that we'd have to use a shell script to unset the env variable - that could be problematic with e.g. fish. I just didn't want to deal with that, though admittedly it would've been cleaner in other aspects.

Redact all above. We'll just set MACOSX_DEPLOYMENT_TARGET regardless of whether you have it in your OG shell environment or not. This means that target/debug/zed is now the one that can run into the same problem, but.. 🤷

Release Notes:

  • N/A

Running `cargo run .` twice in Zed repository required a rebuild two times in a row.
The second rebuild was triggered around libz-sys, which in practice caused a rebuild of the ~entire project.

Zed build script on MacOS sets MACOSX_DEPLOYMENT_TARGET to 10.15. This is fine.
However, **cargo propagates all environment variables to child processes during `cargo run`**.
This then affects Rust Analyzer spawned by dev Zed - it clobbers build cache of whatever package it touches,
because it's behavior is not same between running it with `cargo run` (where MACOS_DEPLOYMENT_TARGET gets propagated to child Zed)
and running it directly via `target/debug/zed` or whatever (where the env variable is not set, so that build behaves roughly like Zed Dev.app).

We'll unset that env variable from user environment when we're reasonably confident that we're running under `cargo run` by exploiting
other env variables set by cargo: https://doc.rust-lang.org/cargo/reference/environment-variables.html
CARGO_PKG_NAME is always set to `zed` when running it via `cargo run`, as it's the value propagated from the build.
@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Mar 7, 2025
@osiewicz osiewicz enabled auto-merge (squash) March 7, 2025 13:40
@osiewicz osiewicz disabled auto-merge March 7, 2025 13:40
I've realized that 6079b7b breaks things the other way.
If one has MACOSX_DEPLOYMENT_TARGET set in their shell profile, they'd start experiencing these issues until they unset it.

Thus, let's instead ensure that regardless of whether MACOSX_DEPLOYMENT_TARGET is set or not, we always set it for cargo, which should allow us to skip rebuilds too.
As a bonus, the runner approach I've mentioned in OP is no better in that regard.
@osiewicz osiewicz enabled auto-merge (squash) March 7, 2025 13:54
@osiewicz osiewicz merged commit d1c6789 into main Mar 7, 2025
16 checks passed
@osiewicz osiewicz deleted the do-not-bust-build-cache branch March 7, 2025 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant