rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.66k stars 2.41k forks source link

Tracking issue for config-include #7723

Open ehuss opened 4 years ago

ehuss commented 4 years ago

Implementation: #7649 Original proposal: #6699 Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#config-include Issues: https://github.com/rust-lang/cargo/labels/Z-config-include

Summary

Adds the include config key to include other config files.

Unresolved issues

FCP

ensc commented 3 years ago

Some additional wishes (from #9306):

jonhoo commented 2 years ago

Some quick thoughts on this:

Restrictions on the path. What's the motivation for restricting the paths? Cargo potentially having special files in .cargo/ in the future is a good one, though I also see an upside in being able to stash other config files under .cargo/ rather than adding another top-level directory to a project like .cargo-includes/. One option might be to say that we allow any path, except a path under .cargo/ that isn't prefixed with config-. So myconfig.toml, myconfig, .cargo/config-my.toml, .cargo/config-my are all okay, but .cargo/my.toml is not. Restrictions beyond that seem unwarranted to me (e.g. requiring .toml).

Multiple includes. I think the ability to have multiple includes is table-stakes for a feature like this. We could get that solely through globs (see below), but just allowing enumeration feels better. I think we should lean into TOML here though and say that the value can be either a string or a list of strings, rather than allowing the key to appear multiple times in a given context (it can, of course, appear multiple times in different tables).

Include globs. I'm torn on allowing globs. They tend to cause headaches as much as it fixes them. One option might be to instead say that Cargo implicitly has a top-level include for each file in .cargo/config.d/ instead? Which I think is a feature that could land independently of this. So my vote is to not include glob support.

Missing files. I think we should initially have just include, and say that it warns, but does not error, if the target file does not exist. We could then in the future add something like require (I don't love that name for this) if the need is great enough.

Placeholders. This feels like something worth punting on until we see use in practice, and not something that should block config-include. It may turn out to be useful, but I think realistically it could be added later. We may want to include checks in the code for config-include that checks the path for % though and warn about possible future-incompatibility (or just straight up disallow it).

ensc commented 2 years ago
jonhoo commented 2 years ago
  • This setup must be optional (e.g. no warnings when included files are not existing).

Why must it produce no warnings?

  • paths (see above) or other default features on Cargo.toml

I don't think this feature will help you with includes for Cargo.toml. It's only for Cargo's configuration files as I understand it.

  • the syntax must allow multiple same keys in the same context

I think that's simply just not allowed by the TOML spec, though I could be wrong. At the very least it would require some custom de/serialization to handle duplicate keys. Your alternative proposal of introducing table entries in the include array seems like a nice one to me, and crucially also seems like something that could be an extension of the feature, rather than something that's there from the get-go.

  • Or just the - prefix (which is used e.g. by GNU Make in -include, or systemd in EnvironmentFile to mark optional files)

I personally strongly prefer explicit syntax (required: true) over "magic" strings. The latter are, among other things, much harder to search for, and not at all immediately obvious to an unfamiliar reader.

ensc commented 2 years ago

This setup must be optional (e.g. no warnings when included files are not existing). Why must it produce no warnings?

Because the setup is not in git (it is local). When other people clone the project they will either be spammed by warnings about non-existing files, or they have to create them manually.

weihanglo commented 1 year ago

Try to push this toward stabilization a bit.

Here lists concerns that I feel like they could be a Future Extension an Expected Behavior, or a Blocker.

ensc commented 1 year ago

If someone checks config-include in their VCS, that must mean they want to share some configs

This is a wrong assumption. People can use config-include for local configuration (e.g. for build.target-dir). Such local configuration should never end in the VCS.

It's easy to fix by adding an empty file at that path

This means, a simple git clone ... && cargo build or cargo install bin-crate will not work because people have to create empty files first.

weihanglo commented 1 year ago

This is a wrong assumption. People can use config-include for local configuration (e.g. for build.target-dir). Such local configuration should never end in the VCS.

Then I will argue why the crate author added an include pointing to nowhere in the first place, and committing it into VCS? I feel like it shouldn't have happened.

One benefit I can see is that it forces people to put their config files in a "dedicated" path the crate author wants. In that case, shouldn't it be better as a hard error?

But anyway, since we consider config files as a kind of local environment, I agree with you that it may also make sense starting from a warning.

ensc commented 1 year ago

Then I will argue why the crate author added an include pointing to nowhere in the first place, and committing it into VCS? I feel like it shouldn't have happened.

It will happen when people use include files for optional local setup. How will you handle it else, when .cargo/config expands unconditionally to

[build]
target-dir = "/home/ensc/.cache/rust/r-tftpd"

IMO such configuration fragments should be optional and do not cause a dirty vcs tree. E.g. place it in local-conf.toml and add

include = "-local-conf.toml"

to .cargo/config.

Btw, glob expansion would solve many problems; either explicitly like

include = "conf.d/*.toml"

or by looking automatically below .cargo/conf.d. It will:

I agree with you that it may also make sense starting from a warning.

no; there should not be a warning. Missing files should be ignored silently.

weihanglo commented 1 year ago

Thanks for the feedback. The workflow of yours looks valid, though I don't agree on completely ignoring missing files. Downgrading to a warning seems fine to me. Ignoring is not.

If it were a silent ignore, how long would it take for people to figure out there is a typo in their include so that config [patch] never work? Cargo was changed to emit unused patches warning because they are really prone to typo. include without warning seems like repeating the old mistake to me.

That being said, one future work of -Zlints is that Cargo will have its own lint rules to turn on and off. Excessive warning can benefit from that.

Personally I lean toward not adding new feature at this time, they can be implemented afterward in a compatible way. -Zconfig-include was added three years ago and waiting another three years for newly added features doesn't look good to me.

ensc commented 1 year ago

If it were a silent ignore, how long would it take for people to figure out there is a typo in their include

Make it explicit whether file is required or optional. It was discussed some comments above:

three years ago and waiting another three years

I fear, that optional, local customization will never be implemented when some basic, half-baked include mechanism exists.

joshtriplett commented 1 year ago

@ensc We might be open to adding a feature for optional config files, such as include = { path = "some.toml", optional = true }. But that's a new feature proposal, and not something we should add at the last minute before stabilizing this existing feature.

ensc commented 1 year ago

@joshtriplett ok; by looking at the speed of implementing new features, this means that cargo will probably never support optional configuration files :(

weihanglo commented 1 year ago

The Cargo team discussed this on Tuesday. We are going to stabilize the feature as is.

We notice that there are some possible extensions, however, we believe they could be added later after the stabilization. The only last minute stuff added was #12298, which puts a restriction on config include path to allow only .toml extensions, reserving some spaces for Cargo to add its own directories.

Let's do a final check before the stabilization PR

@rfcbot fcp merge

rfcbot commented 1 year ago

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

ehuss commented 1 year ago

I'm still a little fuzzy on the exact use cases for this. Can you show an example of when this would be used?

The original motivation wasn't a very strong one. To some degree, it was just a clever generalization of the real feature which was the ability to add additional configs via the CLI (or env, which was never implemented), which offered more manual control. But having unconditional includes in the config file itself wasn't strongly motivated.

I have two concerns about potential use cases:

Without supporting any sort of conditionals, then it seems like it isn't much different from just including the settings all in one file. To me, it seems significantly less useful if it doesn't have any conditionals.

ensc commented 1 year ago

I'm also curious what those extra settings would actually be

[build]
target-dir = "/home/ensc/.cache/rust/r-tftpd"

or cargo vendor setup

epage commented 1 year ago

I'm also concerned about the lack of end-users driving this. Use cases from end-users help us make sure we are delivery features in the way needed rather than tying us down with compatibility with something no one uses. Looking at #6699 and #7649, this seems more driven as a "could this potentially be useful" rather than someone saying "I have problem X that I need solved". #9306 is linked to in this thread but that requires further extensions (glob or optional config support). If this were a personal project of mine, like clap, I'd either work towards #9306 or remove this feature.

weihanglo commented 1 year ago

Thanks for the feedback. There are some details I can share with.

People want to have some controls over configuration file loading for a long while. This is just one of those possible solutions. ##9769 is a collection of them.

Some configs in .cargo/config.toml file are "per-pakcage" configuration, like rustflags and $PATH. They shouldn't be shared across all packages. At $WORK, a "meta" build system generates Cargo configs on a per-package per-machine basis. Those configs shouldn't be shared across all pacakges because some configs like rustflags and $PATH are filled with local paths.

One workaround is to inject all the generated config into ./.cargo/config.toml under the package root. However, that implies a user now may need to check in generated configs with their own configs.

If we have -Zconfig-include. We can just generate a local .cargo/shared-config.toml that can then be included from users' project-local .cargo/config.toml (which we'd generate if it doesn't exist). users could then check in .cargo/config.toml (with the include field) to their VCS, but exclude the generated file.


If this is intended for local (per-machine) settings, then that implies that the per-machine settings wouldn't be checked into source control, which would mean the cargo packages would fail without first doing some extra tasks.

I was worried about this but forgot to write it out. I am more worried that cargo package succeeds but the tarball actually is broken due to the lack of some configs. Although broken packages exist on crates.io today, include may make the situation happening more often.

Dessix commented 1 year ago

The concept of .local and .dev configurations is pretty much a requirement as systems like alternate registries or Tokio's unstable mode begin coercing users into checking in .cargo/config.toml. As it stands, it's a hackfest to make something "optional" by instead generating another config.toml at the include target location if and only if it doesn't exist prior to builds in CI.

If you're concerned with satisfying end-user cases, all of our end-user cases rely on optionality, though it is something we've had to hack in, even with the addition of config-include.

weihanglo commented 11 months ago
  • For truly user-specific settings, those should go in CARGO_HOME.

For a large monorepo that lots of people collaborate on, it is easier to educate people to put their project-local config.toml in the same location, say, .cargo/local-config.toml.

  • If this is intended for local (per-machine) settings, then that implies that the per-machine settings wouldn't be checked into source control, which would mean the cargo packages would fail without first doing some extra tasks. I'm also curious what those extra settings would actually be (if there is perhaps a different solution for them).

A project that already checked .cargo/config.toml in VCS. It may have

[profile.test]
debug = false

to speed up the test build and reduce memory usage in CI pipeline.

For local development, user might want to turn it own at per-project-level, but without -Zconfig-include, their option is either using --config CLI arg, or set up a .cargo/config.toml one level up from the project root, or set the value in ~/.cargo/config.toml. These options are either not convenience for a large team, or doesn't reflect the truth of per-project-level setup.

Granted, we have git update-index --assume-unchanged or git update-index --skip-worktree to ignore local modified files and prevent from committing into git. These git options doesn't really good to learn and remember.

Nemo157 commented 4 months ago

I have a usecase for something in this area now, but it requires the support for "optional includes". We have a shared workspace config committed into the repository for the xtask pattern, but we also commonly want to be able to apply patches while testing changes across multiple repositories:

# .cargo/config.toml
include = "config.local.toml"
[alias]
xtask = "run --package xtask --bin xtask --"
# Cargo.toml
...
# If you want to get `foobar` from your local machine then create a `.cargo/config.local.toml` in this directory with:
#
#   [patch."ssh://git@github.com/foobar/foobar.git"]
#   foobar.path = "../foobar"
#
# (adapting the path as needed relative to this directory)

Having to edit a version-controlled file and remember to not commit it (or keep reverting and reapplying the changes when using a VCS like Jujutsu that doesn't allow not committing) is painful.

Re-reading the docs now I also notice that the precedence is backwards for this sort of usecase, to get the correct precedence (local config can override shared config) requires committing two config files into the repo:

# .cargo/config.toml
include = [ "config.shared.toml", "config.local.toml" ]
# .cargo/config.shared.toml
[alias]
xtask = "run --package xtask --bin xtask --"
twitu commented 4 months ago

I just hit a use case where this feature will be very useful. I'm using pyo3 in my project and it frequently recompiles the whole project even for small change. This happens because of the difference in Python path in my cli and vscode rust analyzer. Running a cargo command in CLI invalidates the build artifacts from rust analyzer

The solution^1 is to make the rust analyzer and CLI use the same python path by setting the PYO3_PYTHON variable in .cargo/config.toml. As you can see though, the path is user specific so I can't commit it. Having local project specific config will be a big help here.

[env]
PYO3_PYTHON = "/home/twitu/.cache/pypoetry/virtualenvs/nautilus-trader-_5j1sq6Z-py3.10/bin/python"

I hope this gets merged soon :smile:

weihanglo commented 4 months ago

. This happens because of the difference in Python path in my cli and vscode rust analyzer. Running a cargo command in CLI invalidates the build artifacts from rust analyzer

A workaround is to have an intermediate directory containing additional .cargo/config.toml so that you don't need to commit the file to VCS. It is not ergonomic though.

bradjc commented 3 months ago

For tock, it would be helpful for us to be able to include different config files (and hence different compiler/linker flags) for different hardware platforms and different architectures. For example, there are certain flags we only want to include on RISC-V platforms, and we would benefit from keeping those in one (optional) cargo config file rather than copying them for each individual platform (making maintenance more difficult). Right now we are accomplishing this using if statements in a Makefile.

bradjc commented 3 months ago

Also, we (tock) generally use nightly, so we can use the unstable feature, but it doesn't seem to work when enabled in a config file. For example:

❯ cargo config get
build.rustflags = ["-C", "linker-flavor=ld.lld"]
build.target = "thumbv7em-none-eabi"
unstable.build-std = ["core", "compiler_builtins"]
unstable.config-include = true
unstable.unstable-options = true
# The following environment variables may affect the loaded values.
# CARGO_HOME=/Users/bradjc/.cargo
❯ cargo -Zconfig-include config get
build.rustflags = ["-C", "linker=rust-lld", "-C", "linker-flavor=ld.lld", "-C", "relocation-model=static", "-C", "link-arg=-nmagic", "-C", "link-arg=-icf=all", "-C", "symbol-mangling-version=v0", "-C", "lto", "-C", "linker=rust-lld", "-C", "linker-flavor=ld.lld", "-C", "relocation-model=static", "-C", "link-arg=-nmagic", "-C", "link-arg=-icf=all", "-C", "symbol-mangling-version=v0", "-C", "lto", "-C", "linker-flavor=ld.lld"]
build.target = "thumbv7em-none-eabi"
unstable.build-std = ["core", "compiler_builtins"]
unstable.config-include = true
unstable.unstable-options = true
# The following environment variables may affect the loaded values.
# CARGO_HOME=/Users/bradjc/.cargo

My include key is only included with the command line flag and not the [unstable] setting.

bradjc commented 3 months ago

I can confirm that https://github.com/rust-lang/cargo/pull/14196 fixed my issue.

The config-include feature is very useful and it would be great to re-try stabilizing it.

weihanglo commented 3 months ago

Right now we are accomplishing this using if statements in a Makefile.

@bradjc Would you mind elaborating this more, and sharing the Makefile? I am now at the point rethinking that the optionality of config-include feature is a requirement before stabilization.

bradjc commented 3 months ago

I think this open PR is the most clear explanation before and after: https://github.com/tock/tock/pull/4075/files

We optionally add -C flags that we pass to rustc in the makefile today, and that PR would move those flags to config.toml files optionally included by each binary crate (in the boards/ directory).

rfcbot commented 1 week ago

:bell: This is now entering its final comment period, as per the review above. :bell:

ehuss commented 1 week ago

@rfcbot concern syntax

I would like to consider changing the syntax before stabilizing to make it easier to possibly extend in the future. Currently we have:

include = "path/to/file.toml"
# or
include = ["array/of/files.toml", "two.toml"]

However, if we intend to add other properties like optional, then we have to change the type which is a bit difficult with TOML and how our config processing works (particularly with backwards compatibility). We could always introduce new top-level keys, but this seems like a relatively easy change to make now.

I don't know exactly what syntax we should use. Above it was suggested to use a table, like:

include = { path = "some.toml", optional = true }
# or
include = { paths = ["a.toml", "b.toml"], optional = true }

Another option is to make it an array of tables, which would allow specifying settings for each file:

[[include]]
path = "a.toml"
optional = true
[[include]]
path = "b.toml"
optional = false

I also don't know exactly what it would look like if we want to add conditions (or even what conditions we might have). This feature was originally modeled after gitconfig, which uses a conditional syntax like:

[includeIf "hasconfig:remote.*.url:https://example.com/**"]
    path = foo.inc

Also note that git silently ignores missing includes, whereas cargo will error. I'm not sure which default is the right one.

ehuss commented 1 week ago

@rfcbot concern syntax