-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Implement CARGO_CONFIG_PATH config file search override #7894
Conversation
r? @Eh2406 (rust_highfive has picked a reviewer for you, use r? to override) |
1675a9b
to
619a149
Compare
This is a significant change and will require the team to talk it over, if not an RFC. I will add it to our agenda. |
We discussed this in the Cargo meeting, and came to the following conclusions:
|
Personal opinions:
|
@joshtriplett Ping. I've updated the documentation. |
☔ The latest upstream changes (presumably #8124) made this pull request unmergeable. Please resolve the merge conflicts. |
This implements the CARGO_CONFIG_PATH environment variable, which defines in which order `cargo` searches for config files. This overrides the default "search parent directories" order. Fixes rust-lang#7887
@joshtriplett Ping? |
@jsgf can you provide more details on your specific use case and motivations? I see that the issue mentions excluding I'm not opposed to this change, but I'm concerned that it is not very ergonomic. It seems like it would only be useful in the context of another build system (presumably you want to use this with Buck?). There are a number of other issues around Cargo's config discovery (#2930, #6728, #7621, #7723, #8148), and I think it would be good to avoid making the search rules too complicated. I think it would be good to explore what the alternatives are. If the main motivation is to restrict configs to the current workspace, maybe that would be a specific option? How would this interact with a possible enhancement where Cargo honors per-package config files? Maybe more options should be in Presumably, also, on Windows the path would be separated by I also wonder if it makes sense to have explicit filenames in the search list instead of always inferring |
Yes, I want this in a somewhat Buck-related context. I have a tool which can take a Cargo.toml specifying a set of third-party dependencies we want to use, and it vendors them and auto-generates Buck build rules for them. This tool uses While Cargo's current behaviour of "search up through .. and then $CARGO_HOME/config` is easy to describe and generally useful for individual developers, it's extremely non-hermetic. It means that it's possible to accidentally override Cargo's precise behaviour through both local config and latent config from current directory. (True story: I spent a couple of hours trying to debug a problem where I was using a temp directory in /tmp/random-generated-name as a working dir for cargo to try and isolate it from the environment, only to find that I had a left over /tmp/.cargo/config from some experiment weeks before which was interfering.) This PR is an attempt to give scripting or programmatic users of Cargo absolute control over where it finds its config to allow complete reproducability. I'm not precious about this - I'd be fine with any other proposal which gives the same level of control. That said, it looks to me like this mechanism is sufficiently powerful and general to address the needs of #2930, #6728, #7621, and maybe #7723 to an extent. In #2930's case, I think there's a lot to be said for the proposal that Cargo should have searched relative to the package dir rather than its own current directory, but it's too late to change now and having two separate "search up .. mechanisms" is too confusing to be useful. This PR at least allows that to be manually enforced if needed. #7723 proposes an include mechanism, which needs its own definition of where to search for includes. I don't think that's strictly related to this PR, other than whatever that search mechanism is, it should be subject to the same control constraints that this PR aims for. Or depending on what the goal of having an include mechanism is, maybe this PR's mechanism subsumes them? (The primary functional difference is that this allows common definitions and overrides to be specified and controlled by the environment, whereas an include mechanism in Cargo.toml means that it's specified by the Cargo.toml author.) #8148 seems to be more related to the current directory in which build scripts are run, so is orthogonal to this. |
☔ The latest upstream changes (presumably #8454) made this pull request unmergeable. Please resolve the merge conflicts. |
Sorry for the long radio silence. We discussed this (several times) in the Cargo team meetings, and I think it would be best to have a change of this nature go through the RFC process. There are a lot of facets to consider, and I think it would be best to ensure that the community at least has some input to it, to help ensure that we reach a solution that will address most concerns. Another problem is that nobody on the team has the time to really dig deep into this and help work out the design. I hope this doesn't sound like we are brushing you off, because we do think this is an important thing to improve in Cargo, but team-wise we are very limited. In regards to this specific design, one concern I would have is that it requires setting an environment variable to change the behavior. This would require setting the environment variable very specifically, and may need to be changed based on the project, which could be awkward. Cargo's current search behavior is not ideal, and it is certainly on the table to change it (perhaps with some manifest key?), though how it should work isn't clear. There could be more than one mechanism to change its behavior as well (though making it too complex isn't ideal). Would you be willing to try to solicit greater community feedback and work towards a design that could potentially address most of those issues? It might also be helpful if there are other tools of a similar nature that do config-file merging, and see what they do. And maybe collect a few alternative approaches. Also note PR #8652 also wanting to change the search behavior. |
Closing as this PR is quite old and per the comment above, I think we would prefer to make sure changes to config are considered very carefully (and unfortunately none of us have much time to invest in this). I have opened #9769 as a meta issue to cover this. |
This implements the CARGO_CONFIG_PATH environment variable, which defines
in which order
cargo
searches for config files. This overrides thedefault "search parent directories" order.
Fixes #7887