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

Add an unstable --no-config flag and an equivalent environment variable #8757

Closed
wants to merge 2 commits into from

Conversation

phil-opp
Copy link
Contributor

@phil-opp phil-opp commented Oct 6, 2020

This provides a way to ignore configuration values set in .cargo/config files.

This PR implements both an unstable command line flag named --no-config and a CARGO_NO_CONFIG_UNSTABLE environment variable for this, which can be set to "1". Note that the environment variable does not have a feature gate yet since I wasn't sure how to make an environment variable unstable (hence the the UNSTABLE suffix).

Discussed in https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Opt-out.20for.20config.20file.20values/near/212360029

This flag allows to ignore the contents of `.cargo/config` files.
@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 6, 2020
@alexcrichton
Copy link
Member

Sorry for the delay in responding here! We talked about this at a Cargo meeting awhile back and I was bad and am only just getting around now to typing up a reply here.

As I recall it, we were unfortunately still fairly lukewarm on merging this. The main reason, I believe is that we are hesitant to merge unstable features which don't have a clear path to stability. None of us were too thrilled about this feature as-is and we didn't think that it was likely to be stabilized.

I believe we had some more nuanced discussion about the problem that you're having specifically, but unfortunately I took long enough to reply here that I've forgotten what our conclusions were in that arena. I know that this isn't really a great reply in that you're still stuck and Cargo still isn't working as you'd like, and I'm sorry about that :(

@phil-opp
Copy link
Contributor Author

@alexcrichton No worries! Delays happen and I also understand your hesitation on this.

However, I would still like to move forward on this issue in some way, since this unfortunately blocks the next iteration of https://os.phil-opp.com/. Perhaps it makes sense to reiterate my problem in a minimal way, maybe you or someone else has a better idea how to solve it:

Current State

The project is always compiled for a custom target using the build-std functionality. By setting this in a .cargo/config.toml file, we can use the normal cargo check/build/doc commands on the project. We also want to support cargo run so we also set a custom runner command there. In combination, the config file looks like this:

[unstable]
build-std = ["core", "alloc"]

[build]
target = "x86_64-blog_os.json"

[target.'cfg(target_os = "none")']
runner = "bootimage runner"

This all works fine and allows us to run and test our kernel in QEMU using cargo run/cargo test. The bootimage command is an external crate that the user has to install via cargo install.

New Design

For the new version, I want to replace the bootimage tool by a custom executable that the users write themselves (to make it more clear what's going on). So I want to change the runner key in the config file to something like this:

[target.'cfg(target_os = "none")']
runner = "cargo run --package qemu_runner --bin qemu_runner"

This is similar to the cargo-xtask-pattern, which e.g. the rust-analyzer project uses. The qemu_runner crate in this case is a separate crate in the same workspace that should be compiled and run on the host system.

The Problem

Since the runner executable is invoked in the same directory as the cargo run command, the same .cargo/config.toml file applies. So instead of compiling the runner executable for the host system, cargo tries to build it for the target system too, which fails.

Unfortunately, there seems to be no way to fix this problem currently. There are no command line arguments for bringing back the build.target and unstable.build-std keys to their default state. Fully ignoring config files is not possible either.


Please let me know if you have any ideas how to solve this problem (with or without modifying cargo). I would be happy to do the implementation work if we find a reasonable solution.

@alexcrichton
Copy link
Member

Ok the past two weeks also haven't disappointed in terms of whirlwinds of events...

In any case I feel like the best solution for your use case is to support the target field in Cargo.toml. We've discussed this from time to time but haven't ever really gotten off the ground. I think the design is somewhat simple although not 100% obvious in all cases.

But the general idea is that x86_64-blog_os.json would be specified in the kernel's top-level Cargo.toml rather than in .cargo/config.toml so Cargo knows that it's package-specific. The idea then is that if the crate is built without --target it defaults to whatever is in Cargo.toml.

Does that make sense? Or am I perhaps saying something that's already been suggested and pursued...

@phil-opp
Copy link
Contributor Author

phil-opp commented Nov 7, 2020

Thank you for your thoughts on this. After thinking about this a bit more over the past two weeks, I came to the same conclusion that replicating the setting in Cargo.toml would be the best solution. A few days ago, I therefore created a general proposal on the internals forum to replicate some .cargo/config.toml settings in Cargo.toml.

It's great to hear that you (and Josh) are generally in favor of moving some more settings to Cargo.toml. I'm gonna close this PR for now and hope that we find a good design for new Cargo.toml fields. Thanks again for looking into this!

@phil-opp phil-opp closed this Nov 7, 2020
@Nemo157
Copy link
Member

Nemo157 commented Nov 11, 2020

I just happened to notice the zulip discussion on this. I recently lamented the lack of cargo --no-config elsewhere; I have some global settings in ~/.cargo/config that I generally want to apply to all builds, except sometimes I want see the default behavior when testing stuff. Having a flag I can pass on the command line would be much easier than deleting and recreating the config file, or spinning up a VM/container to run the build in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants