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

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

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

ijackson
Copy link
Contributor

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

rustbot commented Apr 23, 2024

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

@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 Apr 23, 2024
@epage
Copy link
Contributor

epage commented Apr 23, 2024

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 Outdated Show resolved Hide resolved
@ijackson
Copy link
Contributor Author

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

epage commented Apr 23, 2024

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

yes, please

@ijackson
Copy link
Contributor Author

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

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

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

@ijackson
Copy link
Contributor Author

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

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.
@epage
Copy link
Contributor

epage commented Apr 24, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 24, 2024

📌 Commit 2f16838 has been approved by epage

It is now in the queue for this repository.

@bors 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 a commit that referenced this pull request Apr 24, 2024
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
Copy link
Contributor

bors commented Apr 24, 2024

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

@bors
Copy link
Contributor

bors commented Apr 24, 2024

💔 Test failed - checks-actions

@bors 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
@epage
Copy link
Contributor

epage commented Apr 24, 2024

@bors retry

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

bors commented Apr 24, 2024

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

@bors
Copy link
Contributor

bors commented Apr 24, 2024

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

@bors bors merged commit 52dae0c into rust-lang:master Apr 24, 2024
23 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 26, 2024
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
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2024
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 rustbot added this to the 1.79.0 milestone Apr 27, 2024
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 S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make .cargo/config deprecation warnings silent
5 participants