rust-lang/cargo

cargo install [--git?] ignores .cargo/config.toml, making packages publishable-but-unbuildable, allthewhile they work when built normally

nabijaczleweli opened this issue · 8 comments

Problem

As given in title.

Steps

  1. git clone --branch v2.0.0 https://github.com/thecoshman/http
  2. cargo build it from the directory (works)
  3. cargo install --git --branch v2.0.0 https://github.com/thecoshman/http (or whatever the spelling is, this is currently HEAD) –
   Compiling rand v0.7.3
error[E0554]: `#![feature]` may not be used on the stable release channel
 --> /home/nabijaczleweli/.cargo/registry/src/index.crates.io-6f17d22bba15001f/brotli-6.0.0/src/lib.rs:8:39
  |
8 | #![cfg_attr(feature = "simd", feature(portable_simd))]
  |                                       ^^^^^^^^^^^^^
  1. Removing features = simd from brotli fixes it.

Possible Solution(s)

No response

Notes

May have something to do with .cargo/config.toml setting env.RUSTC_BOOTSTRAP = 1? But the thing it's used for in the crate itself does work.

Version

$ cargo version --verbose
cargo 1.78.0 (54d8815d0 2024-03-26)
release: 1.78.0
commit-hash: 54d8815d04fa3816edc207bbc4dd36bf18014dbc
commit-date: 2024-03-26
host: x86_64-unknown-linux-gnu
libgit2: 1.7.2 (sys:0.18.2 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Debian 12 (bookworm) [64-bit]

The config env.RUSTC_BOOTSTRAP = 1 is telling stable rust to ignore that error. And config files are ignored for install. That is why you are seeing the error. RUSTC_BOOTSTRAP is a very heavy hammer, one only intended for building the compiler itself.

And yet it is the only way to achieve feature parity of win32 targets with unix ones, since I use it for st_dev and st_ino (rust-lang/rust#63010). But, whatever.

Indeed, I haven't tested cargo install --git on windows, but this does also break that:

error[E0554]: `#![feature]` may not be used on the stable release channel
 --> src/main.rs:1:44
  |
1 | #![cfg_attr(target_os = "windows", feature(windows_by_handle))]
  |                                            ^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0554`.
error: could not compile `https` (bin "http") due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `https` (bin "httplz") due to 1 previous error
error: failed to compile `https v2.0.1 (http://github.com/thecoshman/http#152f4d70)`, intermediate artifacts can be found at `T:\-_-TEM~1\cargo-installbcOmti`.
To reuse those artifacts with a future compilation, set the environment variable `CARGO_TARGET_DIR` to that path.

So this is not "RUSTC_BOOTSTRAP doesn't inherit", as I thought, but
"cargo expressly ignores package configuration when installing", so just make it not do that?

Fixing "we choose to make certain packages publishable-but-unbuildable by ignoring configuration found in the distribution that allows them to be built in the usual case" is done by just not doing that.

Or, as I have just tested, having users set RUSTC_BOOTSTRAP=1 before installing. Not great, but sure.

So if I'm understanding, you are wanting cargo install to use the config file from the --git repo, right?

Config is environmental and not tied to the project (#2930, see #12738 for finding ways the right abstraction for representing config in the project). According to these rules, cargo install should use the config of your current-dir independent of where the source is. You can see this if you do cargo check --manifest-path <some-dir> that the config from <some-dir> won't be used, instead requiring something like #10098. For various reasons, cargo install varies from this pattern and only the user-wide configuration is used and not the config for the current-dir.

Well, if I publish a package (be it via git or by uploading a tarball to crates.io), the fundamental expectation is that it will be built in the same way on the user's machine. This expectation is not being up-held when using the cargo install short-hand but it is if the user explicitly does what cargo install promises to fuse: downloading the tarball (or checking out a worktree), cargo build --release, mv target/... $CARGO_BINS.

If you already have an exception for cargo install for "various reasons", then this means that you can append this issue to those reasons and edit the exception to use the package's configuration, rather than none at all.

Well, if I publish a package (be it via git or by uploading a tarball to crates.io), the fundamental expectation is that it will be built in the same way on the user's machine.

That is not a fundamental expectation. cargo build does dev profile by default while cargo install does release profile. See also #7169.

btw #11036 is a related issue.

And as mentioned, that this is behaving something like cargo build in that its like

$ cargo build --manifest-path ~/.cargo/git/checkouts/somedep-e7ff1db891893a9e/d04355a/Cargo.toml

(ie we aren't changing the current-dir on the users behalf)

The difference is that current-dir is ignored.

This gets to the core of problem: config is environmental and not scoped to a project. If you aren't in the environment then Cargo doesn't load the config.

If you already have an exception for cargo install for "various reasons", then this means that you can append this issue to those reasons and edit the exception to use the package's configuration, rather than none at all.

This is something that could be re-evaluated. It would take some research to see what the trade offs would be. However, more likely to move forward is efforts like #12738.

Either way, this would be use-case driven and env.RUSTC_BOOTSTRAP = 1 is not a sympathetic use case to me as you are publishing a package that purportedly can work on any stable release but fundamentally can't. This is instead a package that requires a nightly toolchain and should recognize that.

if you're trying to imply that because cargo install defaults to --release (which you, in your belaboured expansion, also omitted, like me, because it's irrelevant) while cargo build doesn't, and this is ideologically significant, then idk what to tell you except "lol"

If it's ignored, then unignore it and use the one next to the manifest. or chdir to the build root. this seems very easy to fix.

And no, I'm publishing a package that can work on stable releases which include the windows_by_handle feature in its current or similar form, which is all of them since 1.38 (if I'm reading github's git tag --contains equivalent right; the code, after moving twice, is unchanged and still blames back to the original merge from exactly 5 years ago); this is no different to using any other feature, other than you don't like it.
Usually this is just called MSRV and ignored because it's implicit and fluctuates by accident. I opted into it explicitly. Egad!
I doubt you'll find many packages conforming to your much more stringent "works on any stable release" criterion precisely for this reason.

It's also obviously false that it requires a nightly toolchain, as proven by me using a range of stable toolchains to build it for the past seven years.

if you're trying to imply that because cargo install defaults to --release (which you, in your belaboured expansion, also omitted, like me, because it's irrelevant) while cargo build doesn't, and this is ideologically significant, then idk what to tell you except "lol"

I recommend to please respectfully engage with others and assume they are trying to also participate in good faith with you. I am going to step away from this conversation for several days (at least) to let things calm down before further participation.

Hey folks, please treat each other with kindness and respect.