Fix warning suppression for config.toml vs config compat symlinks #13793

ijackson · 2024-04-23T18:23:54Z

What does this PR try to resolve?

Background: the cargo config file is being renamed from .cargo/config to .cargo/config.toml. There's code in new cargo to look for both files (for compatibility), to issue a warning when onliy the old filename is found, and also to issue a warning if both files are found. The warning suggests making a symlink if compatibility with old cargo is wanted.

An attempt was made to detect when both the old and new files exists, but one is a symlink to the other, but as reported in #13667, this code is not effective. (It would work only if the symlink had the precise absolute pathname that cargo has decided to use for the lookup, which would be an unnatural way to make the link.)

Logically, the warning should appear when both files exist but are different. That is the anomalous situation that will generate confusing behaviour. By "different" we ought to mean "aren't the very same file".

That's what this MR implements, where possible. On Unix, we use the information from stat(2). That's not available on other platforms; on those, we arrange to also tolerate a symlink referring to precisely config.toml as a relative pathname, which is also fine, since by definition the target is then in the same directrory as config.

Fixes #13667.

How should we test and review this PR?

I have interleaved the new tests with the commits that support them. In each case, a functional commit is followed by a test which fails just beforehand.

(This can be observed by experimentally reordering the branch.)

I have also done ad-hoc testing.

Additional information

I'm making the assumption that a symlink containing a relative path does something sane on Windows. This assumption may be unwarranted. If so, "Handle config -> config.toml (without full path)" needs to be dropped, and the test case needs to be #[cfg(unix)].

But also, in this case, we should probably put some warnings in the stdlib docs!

rustbot · 2024-04-23T18:23:59Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

epage · 2024-04-23T18:34:36Z

Please add all test cases in the initial commit, passing. This should show what the current behavior is. As the commits are made that change behavior, the test cases will then be updated to pass with the behavior changes.

This has a couple of benefits

Helps test the tests
Makes it more clear to anyone reading this what the behavior changes are

tests/testsuite/config.rs

src/cargo/util/context/mod.rs

ijackson · 2024-04-23T19:27:54Z

Please add all test cases in the initial commit, passing. This should show what the current behavior is. As the commits are made that change behavior, the test cases will then be updated to pass with the behavior changes.

OK. When should I rebase the branch then? Normally I wouldn't do that without an OK from the reviewers.

epage · 2024-04-23T19:28:06Z

OK. When should I rebase the branch then? Normally I wouldn't do that without an OK from the reviewers.

yes, please

ijackson · 2024-04-23T19:31:22Z

OK. Thanks everyone for the comments. LMK about the samefile thing, and I'll probably provide a revised version tomorrow.

"symlink A to B" is confusing; it is ambiguous (at leaset to me) whether it means A -> B or B -> A. And I'm about to introduce a function that does the reverse, and also one that makes a relative rather than full path link. So rename this function.

ijackson · 2024-04-24T10:30:14Z

I've revised this and rebased/reordered it, as discussed.

While I was writing the code, I felt dirty writing if let Ok(...) = , because that's an ignored error bug. But I don't think this MR is the right time to address this, so I just added a comment about it. Ideally we'd change this here, but also use clippy to forbid use of the buggy exists function throughout the codebase. If folks agree, I could do that as a followup.

ijackson · 2024-04-24T10:38:14Z

Err, I have discovered a mistake. Give me a moment, will rewind this again. Sorry.

ijackson · 2024-04-24T11:40:12Z

I think this is ready now. CI failures are unrelated.

src/cargo/util/context/mod.rs

This is 100% reliable on Unix, and better on Windows. (In this commit I avoid reindenting things to make review easier; the formatting will be fixed in the next commit.) Fixes rust-lang#13667

Apply deferred indentation changes. Whitespace change only.

src/cargo/util/context/mod.rs

epage · 2024-04-24T16:31:20Z

@bors r+

bors · 2024-04-24T16:31:23Z

📌 Commit 2f16838 has been approved by epage

It is now in the queue for this repository.

Fix warning suppression for config.toml vs config compat symlinks ### What does this PR try to resolve? Background: the cargo config file is being renamed from `.cargo/config` to `.cargo/config.toml`. There's code in new cargo to look for both files (for compatibility), to issue a warning when onliy the old filename is found, and also to issue a warning if both files are found. The warning suggests making a symlink if compatibility with old cargo is wanted. An attempt was made to detect when both the old and new files exists, but one is a symlink to the other, but as reported in #13667, this code is not effective. (It would work only if the symlink had the precise absolute pathname that cargo has decided to use for the lookup, which would be an unnatural way to make the link.) Logically, the warning should appear when both files exist *but are different*. That is the anomalous situation that will generate confusing behaviour. By "different" we ought to mean "aren't the very same file". That's what this MR implements, where possible. On Unix, we use the information from stat(2). That's not available on other platforms; on those, we arrange to also tolerate a symlink referring to precisely `config.toml` as a relative pathname, which is also fine, since by definition the target is then in the same directrory as `config`. Fixes #13667. ### How should we test and review this PR? I have interleaved the new tests with the commits that support them. In each case, a functional commit is followed by a test which fails just beforehand. (This can be observed by experimentally reordering the branch.) I have also done ad-hoc testing. ### Additional information I'm making the assumption that a symlink containing a relative path does something sane on Windows. This assumption may be unwarranted. If so, "Handle `config` -> `config.toml` (without full path)" needs to be dropped, and the test case needs to be `#[cfg(unix)]`. But also, in this case, we should probably put some warnings in the stdlib docs!

bors · 2024-04-24T17:02:12Z

⌛ Testing commit 2f16838 with merge 32b17f3...

bors · 2024-04-24T17:06:07Z

💔 Test failed - checks-actions

epage · 2024-04-24T17:35:17Z

@bors retry

bors · 2024-04-24T17:36:37Z

⌛ Testing commit 2f16838 with merge 52dae0c...

bors · 2024-04-24T18:03:27Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 52dae0c to master...

Update cargo 9 commits in c9392675917adc2edab269eea27c222b5359c637..b60a1555155111e962018007a6d0ef85207db463 2024-04-23 19:35:19 +0000 to 2024-04-26 16:37:29 +0000 - fix(toml): Remove underscore field support in 2024 (rust-lang/cargo#13804) - fix: emit 1.77 syntax error only when msrv is incompatible (rust-lang/cargo#13808) - docs(ref): Index differences between virtual / real manifests (rust-lang/cargo#13794) - refactor(toml): extract dependency-to-source-id to function (rust-lang/cargo#13802) - Add where lint was set (rust-lang/cargo#13801) - fix(toml): Don't double-warn when underscore is used in workspace dep (rust-lang/cargo#13800) - fix(toml): Be more forceful with underscore/dash redundancy (rust-lang/cargo#13798) - Fix warning suppression for config.toml vs config compat symlinks (rust-lang/cargo#13793) - Cleanup linting system (rust-lang/cargo#13797) r? ghost

rustbot assigned weihanglo Apr 23, 2024

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 Apr 23, 2024

epage reviewed Apr 23, 2024

View reviewed changes

tests/testsuite/config.rs Outdated Show resolved Hide resolved

epage reviewed Apr 23, 2024

View reviewed changes

src/cargo/util/context/mod.rs Outdated Show resolved Hide resolved

weihanglo reviewed Apr 23, 2024

View reviewed changes

src/cargo/util/context/mod.rs Outdated Show resolved Hide resolved

ijackson added 2 commits April 24, 2024 10:03

Tests: Add test case for config -> config.toml (relative)

dcce007

ijackson force-pushed the symlink branch from d9cb950 to a0bbc1f Compare April 24, 2024 10:26

ijackson force-pushed the symlink branch from a0bbc1f to 2f016c4 Compare April 24, 2024 10:36

Tests: Add test case for config.toml -> config

91f3e45

ijackson force-pushed the symlink branch from 2f016c4 to aaef099 Compare April 24, 2024 10:47

epage reviewed Apr 24, 2024

View reviewed changes

src/cargo/util/context/mod.rs Outdated Show resolved Hide resolved

ijackson added 2 commits April 24, 2024 15:28

config reading: use same_file for suppressing "both files" warning

23440c0

This is 100% reliable on Unix, and better on Windows. (In this commit I avoid reindenting things to make review easier; the formatting will be fixed in the next commit.) Fixes rust-lang#13667

config reading: use same_file for suppressing "both files" warning (fmt)

2f16838

Apply deferred indentation changes. Whitespace change only.

ijackson force-pushed the symlink branch from aaef099 to 2f16838 Compare April 24, 2024 14:29

epage reviewed Apr 24, 2024

View reviewed changes

src/cargo/util/context/mod.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2024

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 24, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2024

bors merged commit 52dae0c into rust-lang:master Apr 24, 2024
23 checks passed

This was referenced Apr 25, 2024

Advice on symlinking config to config.toml leads to a different warning #13803

Closed

Update cargo rust-lang/rust#124416

Merged

rustbot added this to the 1.79.0 milestone Apr 27, 2024

weihanglo mentioned this pull request May 13, 2024

dubiously overconstrained warnings about project-level configuration file name #13912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix warning suppression for config.toml vs config compat symlinks #13793

Fix warning suppression for config.toml vs config compat symlinks #13793

ijackson commented Apr 23, 2024

rustbot commented Apr 23, 2024

epage commented Apr 23, 2024

ijackson commented Apr 23, 2024

epage commented Apr 23, 2024

ijackson commented Apr 23, 2024

ijackson commented Apr 24, 2024

ijackson commented Apr 24, 2024

ijackson commented Apr 24, 2024

epage commented Apr 24, 2024

bors commented Apr 24, 2024

bors commented Apr 24, 2024

bors commented Apr 24, 2024

epage commented Apr 24, 2024

bors commented Apr 24, 2024

bors commented Apr 24, 2024

Fix warning suppression for config.toml vs config compat symlinks #13793

Fix warning suppression for config.toml vs config compat symlinks #13793

Conversation

ijackson commented Apr 23, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Apr 23, 2024

epage commented Apr 23, 2024

ijackson commented Apr 23, 2024

epage commented Apr 23, 2024

ijackson commented Apr 23, 2024

ijackson commented Apr 24, 2024

ijackson commented Apr 24, 2024

ijackson commented Apr 24, 2024

epage commented Apr 24, 2024

bors commented Apr 24, 2024

bors commented Apr 24, 2024

bors commented Apr 24, 2024

epage commented Apr 24, 2024

bors commented Apr 24, 2024

bors commented Apr 24, 2024