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

cargo build --all-targets executes build.rs twice in beta and nightly #5575

Closed
stepancheg opened this issue May 27, 2018 · 4 comments · Fixed by #6309
Closed

cargo build --all-targets executes build.rs twice in beta and nightly #5575

stepancheg opened this issue May 27, 2018 · 4 comments · Fixed by #6309

Comments

@stepancheg
Copy link
Contributor

build.rs

Repro

clone https://github.com/stepancheg/cargo-build-rs-twice
cd cargo-build-rs-twice

Stable:

% rm xx/*; cargo +stable build --all-targets && ls xx/
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Compiling libc v0.2.41
   Compiling fgfgf v0.1.0 (file:///Users/nga/tmp/cargo-build-rs-twice)
    Finished dev [unoptimized + debuginfo] target(s) in 2.59 secs
50421

Nightly:

% rm xx/*; cargo +nightly build --all-targets && ls xx/
   Compiling libc v0.2.41
   Compiling fgfgf v0.1.0 (file:///Users/nga/tmp/cargo-build-rs-twice)
    Finished dev [unoptimized + debuginfo] target(s) in 2.85s
50513	50514

build.rs writes a pid file into xx/ directory.

Two pid files in xx directory mean that build.rs was executed twice in nightly.

% cargo +nightly version
cargo 1.28.0-nightly (f352115d5 2018-05-15)
% cargo +stable version
cargo 1.26.0 (0e7c5a931 2018-04-06)
@matklad
Copy link
Member

matklad commented May 27, 2018

This is sort-of intentional behavior change. When executing build scripts, Cargo passes env-vars like PROFILE, OPT_LEVEL and such, which could be different for lib, tests and becnhmarks. Previously, cargo executed build.rs once, but that could have been wrong.

@ehuss @alexcrichton we've decided that this is an ok change of behavior, but probably it isn't? I certainly see how this could be surprising, and that in the common case, when build.rs does not inspect those env-vars, this could lead to genuinely duplicated work as well.

@matklad
Copy link
Member

matklad commented May 27, 2018

cc #5384, which introduced the changes.

@ehuss
Copy link
Contributor

ehuss commented May 27, 2018

Related to #4929 there's a question about how build should handle benchmarks if you don't specify --release. If we built them in dev mode, that would fix this issue†. I'm warming up to that idea, it would be more consistent. It would also provide a means to more easily debug a benchmark if they desire. I imagine it is rare that someone will try to build a benchmark and then run it manually (unless there is some IDE that does this instead of running bench?). build --release --bench ... would continue to use the "bench" profile.

I'm not sure how surprising build --benches building with "test" profile would be.

Let me know what the cargo team thinks of that idea, and I can work on it.

† Overridden test profile with different debug or opt-level would still trigger this behavior.

@alexcrichton
Copy link
Member

I'm personally ok with the change as is, but I agree with @ehuss that perhaps the best thing to do here is to have cargo build build nothing in release mode. I think that'd be a good solution for this!

bors added a commit that referenced this issue Nov 14, 2018
Use "test" profile for `cargo build` benchmarks.

When using `cargo build` (without `--release`), build benchmarks using the "test" profile. This was causing some confusion where the benchmark is placed in the `target/debug` directory, and also causing some duplicates that may not be expected. It also makes it easier to debug benchmarks (previously you had to edit the `[profile.bench]` profile).

Closes #5575, closes #6301, closes #4240, closes #4929.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants