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

Tracking issue for config-include #7723

Open
1 task
ehuss opened this issue Dec 19, 2019 · 24 comments
Open
1 task

Tracking issue for config-include #7723

ehuss opened this issue Dec 19, 2019 · 24 comments
Labels
C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge proposed-final-comment-period An FCP proposal has started, but not yet signed off. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. T-cargo Team: Cargo Z-config-include Nightly: `include` config key
Projects

Comments

@ehuss
Copy link
Contributor

ehuss commented Dec 19, 2019

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

Summary

Adds the include config key to include other config files.

Unresolved issues

  • Should there be any restriction on the filename? Currently there isn't. I was initially considering restricting to .cargo/config-* in case we want to safely use other filenames in .cargo/ for other purposes. However, it seems like an unnatural restriction. Another restriction is that it should maybe end in .toml?
@ehuss ehuss added C-tracking-issue Category: A tracking issue for something unstable. Z-config-include Nightly: `include` config key labels Dec 19, 2019
@ehuss ehuss added this to Unstable, baking in Roadmap Dec 30, 2019
bors added a commit that referenced this issue Feb 18, 2020
Support `--config path_to_config.toml` cli syntax.

Updates the `--config` flag so that if the argument appears to be a file, it will load the file.

cc #7722, #7723
@ensc
Copy link

ensc commented Apr 7, 2021

Some additional wishes (from #9306):

  • allow to include multiple files

    include = "foo.toml"
    include = "bar.toml"
    include = "conf.d/*.toml"
  • allow to ignore missing files; e.g. either by a special syntax like leading "-":

    include = "-can-be-missing.toml"
    include = "required.toml"

    or by using special keywords (e.g. required vs include)

    This feature is required for local configuration (e.g. the setup of build.target-dir) within a more compliex environment

  • substitute placeholders within the filename; e.g.

    • %h ... local hostname
    • %u ... local username
    • %H ... hash of absolute Cargo.toml path
    • ...

@jonhoo
Copy link
Contributor

jonhoo commented Jan 25, 2022

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
Copy link

ensc commented Jan 25, 2022

  • my main reason for this feature is local customization. E.g. I want .cargo/config as part of the project in git, but local setup (e.g. for build.target-dir) should be possible and only in my environment. This setup must be optional (e.g. no warnings when included files are not existing).

    Atm, my git work tree is always dirty in rust projects because there is some local setup (paths (see above) or other default features on Cargo.toml). Moving these settings in optional files would be very helpful

  • when using distinct keywords (include for optional, require for mandatory includes), the syntax must allow multiple same keys in the same context. E.g.

     include: optional-00.conf        ## sets A = 0
     require: mandatory-00.conf       ## sets A = 1
     include: optional-01.conf        ## sets A = 2

    has a different load order than

     include:
     - optional-00.conf        ## sets A = 0
     - optional-01.conf        ## sets A = 2
     require:
     - mandatory-00.conf       ## sets A = 1

    "A" will be 2 in the first example but "1" in the latter one.

    That's why, some indication whether mandatory or not should be part of the file name. Perhaps

    include:
    - path: optional-00.conf
      optional: true
    - mandatory-00.conf
    - path: optional-01.conf
      optional: true

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

@jonhoo
Copy link
Contributor

jonhoo commented Jan 25, 2022

  • 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
Copy link

ensc commented Jan 26, 2022

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.

@ehuss ehuss added the S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. label Apr 25, 2023
@weihanglo
Copy link
Member

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.

  • Blocker: Name restriction
    I quite like the idea putting a restriction on config files name under .cargo/config-*. Requiring .toml makes no sense to me. Cargo will continue support .cargo/config forever, why only allow the standard config file to bypass that?

    There is the other unresolved question: If we put a rule on config file name, should we let the CLI --config bypass that rule, for example cargo --config .cargo/myconfig.toml? This is allowed today. My answer is yes, but should the behavior sticky to all include key loaded via --config or just the first one?

  • Blocker: Loading Priority
    Not really a blocker just want it to be clarified. What is the loading precedence of the array include = ["a", "b", "c"]. I assume that it's like this: config in c gets the highest priority. Then its direct descendants, and down to leaf descendants. Then b and descendants. The last is a. Can als define it in reverse-order. We just need to confirm on a definition.

  • Future Extension, Blocker: glob/gitgnore style syntax
    This can be added afterward. It may be a tiny breaking change if someone uses *!? in their filename but that shouldn't be too common. If we want this to happen, we need to reserve such characters before stabilization.

  • Future Extension, Expected Behaivor, : ENOENT errors
    Currently this is an error when no file found at given config path. We can conservatively just keep it an error when stable. If someone checks config-include in their VCS, that must mean they want to share some configs. It's easy to fix by adding an empty file at that path. This can also be enhanced with [[include]] extension mentioned below. In addition, we could improve the error message a bit before stabilization.

    If you currently use include = […] and people uses your package via git clone may get lots of ENONET errors, you can change to include = "config-containing-other-include-array.toml", so people only need to create one empty config file to proceed to build.

  • Future Extension: array of tables [[include]]
    This is not too hard to add after the stabilization. Needn't do it now.

  • Future Extension: Placeholder for path
    Likewise, we can prohibit some special characters before stabilization. However, it does seem like a request similar to Support environment variable expansion in config.toml #10789. Maybe we should consider them as a whole? Anyway, I can think of one hacky patch: adding a new key env-substitution and when it's turned on Cargo use the new syntax to perform variable substitutions. I don't think we need to do it now.

@ensc
Copy link

ensc commented Jun 18, 2023

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
Copy link
Member

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
Copy link

ensc commented Jun 18, 2023

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:

  • allow local setup without dirtying the vcs tree
  • allow overriding by defining a load order

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
Copy link
Member

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
Copy link

ensc commented Jun 18, 2023

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:

  • use a special - prefix (e.g. include = "-optional-config"); this is a common practice (e.g. in make or systemd)
  • add a special annotation tag (optional: true)

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.

@weihanglo weihanglo added the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Jun 18, 2023
@joshtriplett
Copy link
Member

@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
Copy link

ensc commented Jun 20, 2023

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

@weihanglo weihanglo removed the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Jun 20, 2023
@weihanglo weihanglo added the T-cargo Team: Cargo label Jun 22, 2023
@weihanglo
Copy link
Member

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
Copy link
Collaborator

rfcbot commented Jun 22, 2023

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

No concerns currently listed.

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.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Jun 22, 2023
@ehuss
Copy link
Contributor Author

ehuss commented Jun 22, 2023

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:

  • 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).
  • For truly user-specific settings, those should go in CARGO_HOME.

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
Copy link

ensc commented Jun 22, 2023

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
Copy link
Contributor

epage commented Jun 22, 2023

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
Copy link
Member

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
Copy link

Dessix commented Aug 3, 2023

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
Copy link
Member

  • 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
Copy link
Member

Nemo157 commented Jun 3, 2024

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
Copy link

twitu commented Jun 5, 2024

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 solution1 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 😄

Footnotes

  1. https://github.com/PyO3/pyo3/issues/1708

@weihanglo
Copy link
Member

. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge proposed-final-comment-period An FCP proposal has started, but not yet signed off. S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. T-cargo Team: Cargo Z-config-include Nightly: `include` config key
Projects
Status: Unstable, baking
Status: FCP merge
Roadmap
  
Unstable, baking
Development

No branches or pull requests

10 participants