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

Inform build scripts whether cargo is checking or building #4001

Open
Tracked by #84
nrc opened this issue May 7, 2017 · 35 comments
Open
Tracked by #84

Inform build scripts whether cargo is checking or building #4001

nrc opened this issue May 7, 2017 · 35 comments
Labels
A-build-scripts Area: build.rs scripts A-environment-variables Area: environment variables C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-build Command-check E-medium Experience: Medium S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@nrc
Copy link
Member

nrc commented May 7, 2017

Because in cargo check there may be work that build.rs can skip

cc @wycats

@alexcrichton
Copy link
Member

Sounds like a good idea to me! The only catch that I can think of is that we need to ensure we don't cache the check'd output and use it for the real output, other than that should be relatively easy to do with a new env var!

@carols10cents carols10cents added A-build-scripts Area: build.rs scripts A-environment-variables Area: environment variables C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-build Command-check labels Oct 2, 2017
@ghost
Copy link

ghost commented Jan 4, 2018

The only catch that I can think of is that we need to ensure we don't cache the check'd output and use it for the real output

that is what is currently happening, ie.:

  1. touch build.rs
  2. cargo check
    updates eg. target/debug/build/recompile_self-feb93e46b6e91788/output
  3. cargo build
    doesn't build anything(because of 2.), but it should!

tested:
cargo 0.25.0
release: 0.25.0

@wycats
Copy link
Contributor

wycats commented Jan 4, 2018

I think this is an ok thing to expose to build scripts, but it further highlights that a better factoring around build scripts might reduce footguns

This kind of solution will help people who remember to check for the mode, but breaking up build.rs into more hooks, many of which wouldn't semantically run at all during check, would have a bigger effect.

But as long as build.rs is the only game in town, I'm cool with this!

@shepmaster
Copy link
Member

I'd expand the specific cases described in the title and first comment to include any subcommand, as I've seen Stack Overflow questions for "only do something for documentation" and "only do something for tests"

@kellpossible
Copy link

I have a use case where a bunch of heavy work in build.rs script can (and should) be skipped for both the check and test commands.

@jyn514
Copy link
Member

jyn514 commented Sep 7, 2020

This would be helpful for rust-lang/rust#76444

@shlevy
Copy link

shlevy commented Dec 22, 2020

This would be great. My use case is that I'm wrapping a C++ library that has static globals whose constructors read env vars (:scream:), and I'd like to set those appropriately for testing without impacting the normal build.

FWIW I'd prefer to just run a different script altogether if it exists (e.g. build/tests.rs), but not picky.

andreeaflorescu added a commit to andreeaflorescu/linux-loader that referenced this issue Dec 30, 2020
To be able to run the unit tests seamlessly on any dev machine, we need
to have the test resources downloaded. This can be achieved by calling
the post-checkout script. Since this does not need to be executed when
running with Buildkite, but all the time, the script is now moved to
.buildkite/download_resources.sh and it is no longer a Buildkite hook,
but rather a regular bash script.

The script is called from build.rs so that it is transparent to the user
when the resources are downloaded. The disadvantage with this approach
is that the resources will be downloaded even when calling cargo build,
and not only for cargo test. This can be updated in the future once
cargo adds support for identifying the build profile in build scripts.
Tracking issue: rust-lang/cargo#4001

Signed-off-by: Andreea Florescu <fandree@amazon.com>
andreeaflorescu added a commit to andreeaflorescu/linux-loader that referenced this issue Dec 30, 2020
To be able to run the unit tests seamlessly on any dev machine, we need
to have the test resources downloaded. This can be achieved by calling
the post-checkout script. Since this does not need to be executed when
running with Buildkite, but all the time, the script is now moved to
.buildkite/download_resources.sh and it is no longer a Buildkite hook,
but rather a regular bash script.

The script is called from build.rs so that it is transparent to the user
when the resources are downloaded. The disadvantage with this approach
is that the resources will be downloaded even when calling cargo build,
and not only for cargo test. This can be updated in the future once
cargo adds support for identifying the build profile in build scripts.
Tracking issue: rust-lang/cargo#4001

To be able to run this with the rust-vmm-container, the script needs to
be updated such that it uses curl instead of wget (wget is not available
in the container).

Signed-off-by: Andreea Florescu <fandree@amazon.com>
jiangliu pushed a commit to rust-vmm/linux-loader that referenced this issue Dec 30, 2020
To be able to run the unit tests seamlessly on any dev machine, we need
to have the test resources downloaded. This can be achieved by calling
the post-checkout script. Since this does not need to be executed when
running with Buildkite, but all the time, the script is now moved to
.buildkite/download_resources.sh and it is no longer a Buildkite hook,
but rather a regular bash script.

The script is called from build.rs so that it is transparent to the user
when the resources are downloaded. The disadvantage with this approach
is that the resources will be downloaded even when calling cargo build,
and not only for cargo test. This can be updated in the future once
cargo adds support for identifying the build profile in build scripts.
Tracking issue: rust-lang/cargo#4001

To be able to run this with the rust-vmm-container, the script needs to
be updated such that it uses curl instead of wget (wget is not available
in the container).

Signed-off-by: Andreea Florescu <fandree@amazon.com>
@eftychis
Copy link

Is there any solution or path towards providing this feature?

@ahicks92
Copy link

Also have a use case for this like the above: big C++ dependency that Rust builds via build.rs, but only necessary if things get as far as the link step.

@regexident
Copy link

Big +1 on this one:

Also have a use case for this like the above: big C++ dependency that Rust builds via build.rs, but only necessary if things get as far as the link step.

A beefy C++ dependency is effectively rendering cargo check useless for a project of mine.

@andrewdavidmackenzie
Copy link

+1 I would like to distinguish between cargo build, cargo clippy and cargo test in build.rs to avoid repeating heavy work.

@andrewdavidmackenzie
Copy link

andrewdavidmackenzie commented Nov 26, 2021

I have found two things that have helped me reduce the extra undesired work somewhat, although not a full fix yet:

In the Cargo.toml of the package with the build.rs build script that does so much work, avoid running it on tests and doctest (if that is acceptable for your crate):

[[bin]]
name = "flowstdlib"
path = "main.rs"
test = false
doctest = false

[lib]
name = "flowstdlib"
path = "lib.rs"
test = false
doctest = false

This is useful for me as I have a workspace project with many crates, and this only deactivates that for this crate, and cargo test will test all other crates in the workspace.

Corrected Text
I have also used CARGO_PRIMARY_PACKAGE env var to detect when my create was being built as a dependency, not the primary package, and skip the heavy work.

Here is some sample code you can use in build.rs to do that if it helps your case:

    if option_env!("CARGO_PRIMARY_PACKAGE").is_none() {
        println!("cargo:warning='flowstdlib' is not the primary package being build, skipping WASM generation");
        std::process::exit(0);
    }

nyurik added a commit to nyurik/cargo that referenced this issue Nov 27, 2021
Closes rust-lang#4001

This PR adds code to track and pass compile mode to the build scripts. Since the unit's mode is always `RunCustomBuild`, I had to add tracking for the parent's compile mode, and pass it around.

  - [ ] env var naming. I used `CARGO_MODE`, but perhaps another name is better?
  - [ ] env var values naming. I used: `Test`, `Build`, `Check_test/Check`, `Bench`, `Doc_with_deps/Doc`, `Doctest`, `Docscrape`, `RunCustomBuild`
  - [ ] how to represent bool values for `Check` and `Doc`. I created two separate values, but perhaps the `test` and `deps` values should be ignored?
  - [ ] figure out why `cargo bench` sets env var to `Test`
  - [ ] add unit tests
@nyurik
Copy link
Contributor

nyurik commented Nov 27, 2021

I have tried to implement this feature in #10126, but it has a few open questions such as the naming of the env var and its values, etc. If my approach is OKed, I will add the unit tests/try to get it to pass CI. (I'm new to Rust, please be patient 😺)

@jyn514
Copy link
Member

jyn514 commented Nov 14, 2022

@jyn514 I assume your proposal is to have them emit rerun-if-env-changed=CARGO_MODE or so? We'd still also want cargo to have separate caching for different modes (so that a check doesn't destroy the build cache), but then that seems like it should work, yeah

Yes, exactly :)

@ahicks92
Copy link

It could be useful to explore whether or not cc can compile the code without optimization or debug symbols, then never link the final objects. I would see value in knowing that I broke all my C code if I'm writing part of the Rust project in C.

Unfortunately I don't think cmake can do this (well, not reliably. You can ask it for a list of commands it would have run. But let's not...).

@tchernobog
Copy link

Unfortunately I don't think cmake can do this (well, not reliably.

It can. Use object libraries.

@RalfJung
Copy link
Member

RalfJung commented Nov 15, 2022 via email

@ahicks92
Copy link

You can't ask cmake to produce an object library on someone's behalf without modifying their CMake source, and so such a solution would fall outside the realm of libraries. This feature is good for that too, just not something we could feasibly ask the cmake crate for.

What might be useful there though is adding like -DIS_RUST_CHECKING=TRUE or something automatically, and documenting that this happens. However even then as a user, you can't just transparently use object libraries because iirc cmake doesn't propagate them via target_link_libraries without the generator expressions as well and so you would have to restructure your project for it in some form or another.

CBenoit added a commit to Devolutions/sspi-rs that referenced this issue Jan 31, 2023
On Windows, we rename symbols using a .def file.
For that, we emit a cargo instruction.

> cargo:rustc-link-arg=/DEF:sspi.def

However, when attempting to run unit tests this causes an error.

```
Caused by:
  could not execute process `<redacted>` (never executed)

Caused by:
  %1 is not a valid Win32 application. (os error 193)
```

There is currently no built-in way to check if cargo is going to run the tests from the build.rs itself:
rust-lang/cargo#4001

This commit adds a new environment variable that when set prevent the instruction emission.
@sam0x17
Copy link

sam0x17 commented May 26, 2023

GPT-4 led me to believe cargo exposes this in build scripts via the "CARGO_CFG" environment variable. I was all excited to use it, but turns out it's not a thing. Wish it was....

@voltagex
Copy link

Please don't use LLMs for things like this.

@sam0x17
Copy link

sam0x17 commented May 28, 2023

Ok let me rephrase, we really should add something that gives build scripts some context about what is being built and why. It is ridiculous that we have all this information in the parent process, but none of it is shared with build scripts, and I don't see any good reason for why it is not shared.

This should include:

  • read access to all arguments passed to cargo
  • read access to any RUSTC arguments that may have been set

And write access to the above as well as the ability to mutate+inject environment variables that are used later in the build process

@jpramosi
Copy link

I have tried to find a workaround and checked for differences in environment variables within the build script with:

  let mut vars = String::from_str("VARS:\n").unwrap();
  for (key, value) in std::env::vars() {
      vars.push_str(&format!("{}={}\n", key, value));
  }
  std::fs::write("env-vars.log", vars).expect("Unable to write env-vars.log");

The most promising variable on my machine (ubuntu-22.04) seems to be 'RUST_BACKTRACE':

  if std::env::var_os("RUST_BACKTRACE").unwrap_or("".into()) == "short" {
      std::fs::write("_is-checking.log", "-").expect("Unable to write file");
      std::process::exit(0);
  } else {
      std::fs::write("_is-building.log", "-").expect("Unable to write file");
  }

You can also just check whether this variable exists at all.

@ahicks92
Copy link

Using RUST_BACKTRACE is almost certainly not a safe solution. Rust users are encouraged to do:

RUST_BACKTRACE=1 cargo run

For debugging, literally by crashes themselves (that is you don't even go as far as the internet, it just tells you).

cargo run frequently builds the packages. Ergo what you are probably checking for here isn't check vs build, it's "did the user want a backtrace" versus "did the user not care" and so your package will fail to build if the user wants a backtrace unless cargo is somehow deleting that env var between the user and the build script.

Similarly with other environment variables. Without help from Cargo, this isn't doable. All the env vars you might check are set for other reasons, and sometimes by the user for other purposes, e.g. RUST_BACKTRACE.

@sam0x17
Copy link

sam0x17 commented Nov 14, 2023

Yes, this is why we need more context information at the build script level

@kornelski
Copy link
Contributor

For now a workaround is to set DOCS_RS=1, which some sys crates use to skip building when compiled for the docs.rs website.

@SchmErik
Copy link

At this point, is the cargo project looking for someone to implement this?

@weihanglo
Copy link
Member

At this point, is the cargo project looking for someone to implement this?

No. Still in discussion.


We should be cautious of adding new environment variables for build script to read. It is not only a contract affecting Cargo, but also other external build systems.

From what I read from this thread so far, people want build scripts to know the mode Cargo is running because that enables a way to

  1. Skip building external dependency (mostly C and C++) as we only want to type-check our Rust code.
  2. Skip unnecessary works if we only need to generate documentation from rustdoc (which requires type-check first).
  3. Have a different logic for testing/benchmarking purposes.

Some design problems need to be resolved are:

  • By introducing the mode concept to build scripts, the build-script cache between cargo build and cargo check cannot be shared. That kinda slows down the build time if a user run cargo check and then run cargo build (see Inform build scripts whether cargo is checking or building #4001 (comment)).
    • Is there a proper transition for the community?
    • What is the story of type-check for C/C++ libraires today?
  • Should we map each Cargo subcommands to a different mode to inform build script?
    • Pass compile mode to the custom build script #10126 did that, though I feel it a bit too much. There are some overloads in build script usages because Cargo is lacking of functionalities for custom build tasks, though it was not designed as a general purpose build system from the beginning. By exposing mode to build script, we kinda push the already-overloaded build script use cases to the limit, this is something I personally want to avoid.
    • Would there be any compatibility issue if we postpone the support for other modes, and fix this only for distinguishing full builds and type-check builds?

Note that I didn't tie this to something like CARGO_MODE environment variable. The implementation could be something different from that. For example, having a new build.type-check key in Cargo.toml to specify the script from type checking, falling back to build.rs if not found.

@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. E-medium Experience: Medium and removed S-triage Status: This issue is waiting on initial triage. labels Jun 25, 2024
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 A-environment-variables Area: environment variables C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-build Command-check E-medium Experience: Medium S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

Successfully merging a pull request may close this issue.