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

Should build.rs be compiled like plugins? #5436

Open
ehuss opened this issue Apr 28, 2018 · 1 comment
Open

Should build.rs be compiled like plugins? #5436

ehuss opened this issue Apr 28, 2018 · 1 comment
Labels
A-build-scripts Area: build.rs scripts C-enhancement Category: enhancement S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@ehuss
Copy link
Contributor

ehuss commented Apr 28, 2018

Currently, build.rs scripts (and build-dependencies) are built with the same restrictions as compiler plugins (panic and lto cannot be set, maybe other issues?). I don't think this is quite correct because build scripts are not used the same way proc-macro/plugins are.

This is caused by several parts of the code that check for_host() but don't differentiate for build scripts (but some places do!).

One consequence of the current implementation is that a dependency that is shared between regular dependencies and build-dependencies will be built without panic set. I'm not sure if there is much of a problem with this because dependencies built with unwind seem to be fine? (I'd like to better understand the difference, since the assembly output looks almost identical.) Another oddity is that with cargo test or cargo bench, these shared dependencies will be built twice, but with the exact same settings.

Repro:

cargo new --lib foo
cd foo
cat >> Cargo.toml <<EOL
shared = {path="shared"}
[build-dependencies]
shared = {path="shared"}
[profile.dev]
panic = "abort"
EOL

cargo new --lib shared
echo "fn main() {}" > build.rs

cargo build -v   # <- Notice that neither `build.rs` or `shared` is built with panic.

I think to fix it so that build.rs includes panic, it is a relatively simple change to avoid walking the RunCustomBuild unit in build_used_in_plugin_map.

Alternatively, to keep the current behavior and fix the shared dependencies, we should be clearing the panic flag in get_profile (instead of build_base_args), though I'm uncertain what other issues there might be.

In general, it seems to be dangerous to use for_host() if the code is really wanting to know if something is a plugin.

@alexcrichton
Copy link
Member

In general the tweaks to things like panic/LTO are primarily intended to affect the final artifact rather than intermediate artifacts so I don't think we necessarily need to try too too hard to have 100% fidelity with things like build scripts. For exaple if we compiled all build scripts with LTO that'd actually be pretty bad in the sense that it'd be a lot of lost build time for not a lot of gain.

I'd be fine tweaking the panic functionality, though, it seems like it probably wouldn't cause too many probles!

bors added a commit that referenced this issue Oct 15, 2018
Track `panic` in Unit early.

This is an alternate solution for #5445. It ensures that `panic` is cleared in the Profile for "for_host" targets (proc-macro/plugin/build-scripts) and dependencies. This helps avoid unnecessary recompiles in some situations (though does add extra units in some situations, see below).

Some examples where extra rebuilds are now avoided:
* A workspace with a dependency shared with normal and build-deps.  `build --all` should build everything, and then `build -p foo` was causing a recompile because the shared dep was no longer in the `used_in_plugin` set. Now it should not recompile.
* `panic=abort`, with a shared dependency in build and dev, `build` would cause that shared dependency to be built twice (exactly the same without panic set). Now it should only build once.

Some examples where `panic` is now set correctly:
* `panic=abort`, with a binary with a shared dependency in build and normal, `test` would cause that shared dependency to be built twice (exactly the same without panic set).  Now it is still built twice, but the one for the normal (bin) dependency will correctly have `panic` set.

Some examples where new units are now generated:
* `panic=abort`, with shared dependency between normal and proc-macro or build. Previously the shared dependency was built once with `panic=unwind`. Now it is built separately (once with `panic`, once without). I feel like that this is more correct behavior — that now the normal dependency avoids adding landing pads.

   For `panic=abort` cross-compiling, this makes no difference in compile time since it was already built twice.

Additional notes:
- I left build-scripts with the old behavior where `panic` is cleared for it and all its dependencies. There could be arguments made to change that (#5436), but it doesn't seem important to me.
- Renamed and refactored `ProfileFor` to `UnitFor`. I expect this API to continue to evolve in the future.

Closes #6143, closes #6154.
@ehuss ehuss added the A-build-scripts Area: build.rs scripts label Sep 23, 2019
@epage epage added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. C-enhancement Category: enhancement labels Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts C-enhancement Category: enhancement S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

3 participants