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

config: merge lists in precedence order #12515

Merged
merged 2 commits into from
Aug 22, 2023
Merged

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Aug 16, 2023

When merging configuration lists, the current order does not match the expected precedence. This makes merged lists follow precedence order, with higher precedence items merged later in lists.

When a list in configuration exists in multiple places, Cargo merges the lists together. The ordering of this merging is unexpected and does not follow the precedence rules that non-list configuration uses.

The current merging order appears to be:

  • project-specific config.toml
  • global config.toml
  • command-line (--config)
  • environment variable (CARGO_*)

This PR changes the order to follow the precedence rules with higher precedence configuration merging later in the lists.

  • global config.toml
  • project-specific config.toml
  • environment variable (CARGO_*)
  • command-line (--config)

This aligns with config such as build.rustflags where later flags take precedence over earlier ones.

Since --config is relatively new, it's unlikely to cause too much breakage by making it come after environment variables.

Switching global and project-specific ordering is more likely to cause breakage, since it's been around longer (reported as an issue in #8128). Projects relying on global configuration flags (in $CARGO_HOME\config.toml or in .cargo/config.toml further from the project) being merged first in lists will be broken.

For most uses of merged lists (such as build.rustflags), if the flags do not conflict with each other, there will be no impact.

Fixes #12506
Fixes #8128

When merging configuration lists, the current order does not match
the expected precedence. This makes merged lists follow precedence
order, with higher precedence items merged later in lists.
@rustbot
Copy link
Collaborator

rustbot commented Aug 16, 2023

r? @weihanglo

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

@rustbot rustbot added A-configuration Area: cargo config files and env vars S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 16, 2023
@arlosi arlosi changed the title Merge higher precedence config lists later config: merge lists in precedence order Aug 16, 2023
@arlosi
Copy link
Contributor Author

arlosi commented Aug 16, 2023

Since this is a bug fix breaking change, let's make sure everyone is OK with it.

@rfcbot merge

@weihanglo
Copy link
Member

weihanglo commented Aug 16, 2023

You need
@rustbot label +T-cargo
to do an FCP

@rustbot rustbot added the T-cargo Team: Cargo label Aug 16, 2023
@arlosi
Copy link
Contributor Author

arlosi commented Aug 16, 2023

Thanks @weihanglo. Let's try again.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 16, 2023

Team member @arlosi 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.

@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 Aug 16, 2023
@epage
Copy link
Contributor

epage commented Aug 16, 2023

Could the PR re-summarize the situation, especially who will be broken and the level of impact of that?

@rustbot rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Aug 16, 2023
@weihanglo
Copy link
Member

Should we do a crater run for this?
I doubt it'll catch anything interesting with crater though. Usually this kind of configuration is deeply rooted in a user's or an enterprise environment.

@ehuss
Copy link
Contributor

ehuss commented Aug 17, 2023

I don't think crater would be able to run in situations were there would be separate configuration files to merge.

@weihanglo
Copy link
Member

I'll check my box here but want to raise the awareness to a wider audience. I knew some large cooperations do weird configurations around build.rustflags. We should have a way to solicit feedback before specifying an unspecified behavior. An FCP period sounds too short to me.

@rfcbot reviewed
@rfcbot concern raise-awareness

@weihanglo
Copy link
Member

We've talked about this in the Cargo weekly meeting today. We'll have a blog post for this incoming change, and solicit feedback from users. Since beta is just branched off, we'll make this change available on nightly as soon as possible.

@rustbot resolve raise-awareness

We would like to make sure this will get into release note as a compatibility note, so

@rustbot label +relnotes

@rustbot rustbot added the relnotes Release-note worthy label Aug 22, 2023
@weihanglo
Copy link
Member

Oh no it's rfcbot

@rfcbot resolve raise-awareness

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Aug 22, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented Aug 22, 2023

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

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this today and update the submodule. Thanks!

/// If `force` is true, primitive (non-container) types will override existing values.
/// If false, the original will be kept and the new value ignored.
/// If `force` is true, primitive (non-container) types will override existing values
/// of equal priority. For arrays, incoming values of equal priority will be placed later.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks correct. Just what to make sure we have test to verify this behavior.

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2023

📌 Commit 66bda83 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 22, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 22, 2023
@bors
Copy link
Contributor

bors commented Aug 22, 2023

⌛ Testing commit 66bda83 with merge 2cc50bc...

@bors
Copy link
Contributor

bors commented Aug 22, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 2cc50bc to master...

@bors bors merged commit 2cc50bc into rust-lang:master Aug 22, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 23, 2023
Update cargo

3 commits in 80eca0e58fb2ff52c1e94fc191b55b37ed73e0e4..2cc50bc0b63ad20da193e002ba11d391af0104b7
2023-08-19 00:52:06 +0000 to 2023-08-22 22:43:08 +0000
- config: merge lists in precedence order (rust-lang/cargo#12515)
- ci: test `resolver-tests` in a separate job (rust-lang/cargo#12540)
- refactor: Use clap to suggest alternative argument for unsupported arguments (rust-lang/cargo#12529)

r? ghost
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Sep 1, 2023
@ehuss ehuss added this to the 1.74.0 milestone Oct 11, 2023
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 8, 2024
Pkgsrc changes:

 * Remove NetBSD-8 support (embedded LLVm requires newer C++
   than what is in -8; it's conceivable that this could still
   build with an external LLVM)
 * undo powerpc 9.0 file naming tweak, since we no longer support -8.
 * Remove patch to LLVM for powerpc now included by upstream.
 * Minor adjustments, checksum changes etc.


Upstream changes:

Version 1.74.1 (2023-12-07)
===========================

- [Resolved spurious STATUS_ACCESS_VIOLATIONs in LLVM]
  (rust-lang/rust#118464)
- [Clarify guarantees for std::mem::discriminant]
  (rust-lang/rust#118006)
- [Fix some subtyping-related regressions]
  (rust-lang/rust#116415)

Version 1.74.0 (2023-11-16)
==========================

Language
--------

- [Codify that `std::mem::Discriminant<T>` does not depend on any
  lifetimes in T]
  (rust-lang/rust#104299)
- [Replace `private_in_public` lint with `private_interfaces` and
  `private_bounds` per RFC 2145]
  (rust-lang/rust#113126)
  Read more in
  [RFC 2145](https://rust-lang.github.io/rfcs/2145-type-privacy.html).
- [Allow explicit `#[repr(Rust)]`]
  (rust-lang/rust#114201)
- [closure field capturing: don't depend on alignment of packed fields]
  (rust-lang/rust#115315)
- [Enable MIR-based drop-tracking for `async` blocks]
  (rust-lang/rust#107421)

Compiler
--------

- [stabilize combining +bundle and +whole-archive link modifiers]
  (rust-lang/rust#113301)
- [Stabilize `PATH` option for `--print KIND=PATH`]
  (rust-lang/rust#114183)
- [Enable ASAN/LSAN/TSAN for `*-apple-ios-macabi`]
  (rust-lang/rust#115644)
- [Promote loongarch64-unknown-none* to Tier 2]
  (rust-lang/rust#115368)
- [Add `i686-pc-windows-gnullvm` as a tier 3 target]
  (rust-lang/rust#115687)

Libraries
---------

- [Implement `From<OwnedFd/Handle>` for ChildStdin/out/err]
  (rust-lang/rust#98704)
- [Implement `From<{&,&mut} [T; N]>` for `Vec<T>` where `T: Clone`]
  (rust-lang/rust#111278)
- [impl Step for IP addresses]
  (rust-lang/rust#113748)
- [Implement `From<[T; N]>` for `Rc<[T]>` and `Arc<[T]>`]
  (rust-lang/rust#114041)
- [`impl TryFrom<char> for u16`]
  (rust-lang/rust#114065)
- [Stabilize `io_error_other` feature]
  (rust-lang/rust#115453)
- [Stabilize the `Saturating` type]
  (rust-lang/rust#115477)
- [Stabilize const_transmute_copy]
  (rust-lang/rust#115520)

Stabilized APIs
---------------

- [`core::num::Saturating`]
  (https://doc.rust-lang.org/stable/std/num/struct.Saturating.html)
- [`impl From<io::Stdout> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStdout%3E-for-Stdio)
- [`impl From<io::Stderr> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`impl From<OwnedHandle> for std::process::Child{Stdin, Stdout, Stderr}`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`impl From<OwnedFd> for std::process::Child{Stdin, Stdout, Stderr}`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`std::ffi::OsString::from_encoded_bytes_unchecked`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.from_encoded_bytes_unchecked)
- [`std::ffi::OsString::into_encoded_bytes`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.into_encoded_bytes)
- [`std::ffi::OsStr::from_encoded_bytes_unchecked`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.from_encoded_bytes_unchecked)
- [`std::ffi::OsStr::as_encoded_bytes`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.as_encoded_bytes)
- [`std::io::Error::other`]
  (https://doc.rust-lang.org/stable/std/io/struct.Error.html#method.other)
- [`impl TryFrom<char> for u16`]
  (https://doc.rust-lang.org/stable/std/primitive.u16.html#impl-TryFrom%3Cchar%3E-for-u16)
- [`impl<T: Clone, const N: usize> From<&[T; N]> for Vec<T>`]
  (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T: Clone, const N: usize> From<&mut [T; N]> for Vec<T>`]
  (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26mut+%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Arc<[T]>`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#impl-From%3C%5BT;+N%5D%3E-for-Arc%3C%5BT%5D,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Rc<[T]>`]
  (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#impl-From%3C%5BT;+N%5D%3E-for-Rc%3C%5BT%5D,+Global%3E)

These APIs are now stable in const contexts:

- [`core::mem::transmute_copy`]
  (https://doc.rust-lang.org/beta/std/mem/fn.transmute_copy.html)
- [`str::is_ascii`]
  (https://doc.rust-lang.org/beta/std/primitive.str.html#method.is_ascii)
- [`[u8]::is_ascii`]
  (https://doc.rust-lang.org/beta/std/primitive.slice.html#method.is_ascii)

Cargo
-----

- [fix: Set MSRV for internal packages]
  (rust-lang/cargo#12381)
- [config: merge lists in precedence order]
  (rust-lang/cargo#12515)
- [fix(update): Clarify meaning of --aggressive as --recursive]
  (rust-lang/cargo#12544)
- [fix(update): Make `-p` more convenient by being positional]
  (rust-lang/cargo#12545)
- [feat(help): Add styling to help output ]
  (rust-lang/cargo#12578)
- [feat(pkgid): Allow incomplete versions when unambigious]
  (rust-lang/cargo#12614)
- [feat: stabilize credential-process and registry-auth]
  (rust-lang/cargo#12649)
- [feat(cli): Add '-n' to dry-run]
  (rust-lang/cargo#12660)
- [Add support for `target.'cfg(..)'.linker`]
  (rust-lang/cargo#12535)
- [Stabilize `--keep-going`]
  (rust-lang/cargo#12568)
- [feat: Stabilize lints]
  (rust-lang/cargo#12648)

Rustdoc
-------

- [Add warning block support in rustdoc]
  (rust-lang/rust#106561)
- [Accept additional user-defined syntax classes in fenced code blocks]
  (rust-lang/rust#110800)
- [rustdoc-search: add support for type parameters]
  (rust-lang/rust#112725)
- [rustdoc: show inner enum and struct in type definition for concrete type]
  (rust-lang/rust#114855)

Compatibility Notes
-------------------

- [Raise minimum supported Apple OS versions]
  (rust-lang/rust#104385)
- [make Cell::swap panic if the Cells partially overlap]
  (rust-lang/rust#114795)
- [Reject invalid crate names in `--extern`]
  (rust-lang/rust#116001)
- [Don't resolve generic impls that may be shadowed by dyn built-in impls]
  (rust-lang/rust#114941)

Internal Changes
----------------

These changes do not affect any public interfaces of Rust, but they represent
significant improvements to the performance or internals of rustc and related
tools.

None this cycle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-documenting-cargo-itself Area: Cargo's documentation disposition-merge FCP with intent to merge finished-final-comment-period FCP complete relnotes Release-note worthy S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
7 participants