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

[RFC] cargo install should ignore the default build target #5850

Closed
japaric opened this issue Aug 2, 2018 · 11 comments · Fixed by #5874
Closed

[RFC] cargo install should ignore the default build target #5850

japaric opened this issue Aug 2, 2018 · 11 comments · Fixed by #5874

Comments

@japaric
Copy link
Member

japaric commented Aug 2, 2018

This is a papercut embedded users often encounter as our templates ship with a .cargo/config file
that sets a default build target:

$ cargo new --lib foo && cd $_

$ mkdir .cargo
$ cat >.cargo/config <<'EOF'
[build]
target = "thumbv7em-none-eabi"
EOF

$ cargo install cargo-binutils
    Updating registry `https://github.com/rust-lang/crates.io-index`
  Installing cargo-binutils v0.1.1
   Compiling unicode-xid v0.1.0
   Compiling failure_derive v0.1.2
   Compiling serde v1.0.70
   Compiling libc v0.2.42
   Compiling void v1.0.2
   Compiling semver-parser v0.7.0
   Compiling rustc-demangle v0.1.9
   Compiling ucd-util v0.1.1
error[E0463]: can't find crate for `std`
  |
  = note: the `thumbv7em-none-eabi` target may not be installed

The issue is that cargo-install compiles cargo-binutils for the thumbv7em-none-eabi target
and that fails because there's no pre-compiled std for that target. Even if the compilation
did succeed that would result in an ARM binary being installed and a x86_64 host would not be able
to run it in most cases.

I propose that cargo-install ignores the default build target if one is set in a Cargo
configuration file. In the, IMO, rare case where one does want to install a cross compiled binary
cargo install --target $T would be the way to go.

This is technically a breaking change but I doubt anyone is relying on this.

Thoughts?

cc @alexcrichton

@alexcrichton
Copy link
Member

This was originally asked for in #5441 and implemented in #5606, but it was reverted in #5614 to be more consistent with Cargo with --target being supported

It's perhaps not a bad idea though to specifically only recognize --target (and maybe env vars) but otherwise ignore [build] target!

@japaric
Copy link
Member Author

japaric commented Aug 2, 2018

@alexcrichton Interesting!

The case I'm describing is about installing a general purpose tool like ripgrep or racer. In my mind, by typing cargo install I really mean "install this for the host because I want this in my PATH" so I don't think it should matter from where I invoke the command: I could be invoking the command from a Cargo project (because I happen to be there atm) or from home or from somewhere else.

So, IMO, either cargo install (without --target) should ignore both .cargo/config and env vars, OR we should add a --host (or --local) flag to make cargo install compile the binary for the host. The later option seems redundant to me because I only ever use cargo install for local installs.

cc @matklad

@dwijnand
Copy link
Member

dwijnand commented Aug 2, 2018

Could bundle this behaviour change to cargo install with #5327 under edition = "2018".

@alexcrichton
Copy link
Member

I think that makes sense to me to ignore env vars and .cargo/config (aka ambient configuration) and only accept --target for cargo install invocations

japaric added a commit to japaric/cargo that referenced this issue Sep 1, 2018
bors added a commit that referenced this issue Sep 2, 2018
make `cargo install` ignore .cargo/config

closes #5850

r? @alexcrichton
@bors bors closed this as completed in #5874 Sep 2, 2018
@traviscross
Copy link

Regarding the question of whether anyone was relying on this, I was relying on this to set x86_64-unknown-linux-musl as my default target in ~/.cargo/config. The --target option is definitely an improvement, but it doesn't help in changing the default.

While it is surprising that a .cargo/config file in the current directory (or its parents) would affect the behavior of cargo install, it's equally surprising that a value in $HOME/.cargo/config would be ignored.

Perhaps the actually correct behavior is that cargo install should load only $HOME/.cargo/config instead of performing the normal tree walk. This would be a more consistent behavior, and would avoid any other surprises due to current or future .cargo/config values affecting cargo install.

That is, the expected behavior is that no value, not just build.target, from $PWD/.cargo/config (or $PWD/../.cargo/config, etc.) should affect cargo install. But all values from $HOME/.cargo/config should affect cargo install.

@alexcrichton
Copy link
Member

@traviscross sorry for the breakage, but that sounds pretty reasonable to me!

@japaric would that solution cause issues for the embedded usage perhaps?

@japaric
Copy link
Member Author

japaric commented Sep 13, 2018

@alexcrichton No, I don't see any trouble. We only use project local .cargo/configs.

@alexcrichton
Copy link
Member

Ok great! @traviscross want to open a dedicated issue for this?

@traviscross
Copy link

Thanks @alexcrichton, @japaric; sure, it's now opened as #6025.

@jnbr
Copy link
Contributor

jnbr commented Nov 2, 2018

Voidlinux relied on this. It would be nice to have breaking changes mentioned in the release notes, independent of whether someone beliefs if people rely on it.

@dwijnand
Copy link
Member

dwijnand commented Nov 2, 2018

Between this ticket and #6025, and the history Alex linked at the beginning of this ticket, there are quite a few use cases.

Looking at the void-linux/void-packages#4373 I wonder if cargo install --path . should read (local) .cargo/config. (Might be covered in said tickets, I didn't study them all).

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.

5 participants