Fix cargo add overwriting symlinked Cargo.toml files #15281

RaghavenderSingh · 2025-03-08T14:13:41Z

What does this PR try to resolve?

This PR fixes a bug where cargo add breaks symlinks to Cargo.toml files. Currently, when Cargo.toml is a symlink and cargo add is used to add a dependency, the symlink is replaced with a regular file, breaking the link to the original target file.

This issue was reported in #15241 where a user who relies on symlinked Cargo.toml files found that cargo add breaks their workflow.

Fixes #15241

How should we test and review this PR?

I've modified LocalManifest::write() to check if the path is a symlink, and if so, follow it to get the actual target path. This ensures we write to the actual file rather than replacing the symlink.

I've also added a test in tests/testsuite/cargo_add/symlink.rs that:

Creates a symlinked Cargo.toml file
Runs cargo add to add a dependency
Verifies the symlink is preserved and the dependency is added to the target file

I've manually tested this fix and confirmed it works correctly.

rustbot · 2025-03-08T14:13:45Z

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

tests/testsuite/cargo_add/symlink.rs

src/cargo/util/toml_mut/manifest.rs

epage · 2025-03-19T12:41:39Z

src/cargo/util/toml_mut/manifest.rs

This commit does not change the test. Does the previous commit fail cargo test or is it testing the wrong thing? Each commit should be atomic which includes passing tests. When we ask for a test commit to be split out first, it is to show the current behavior. The commit with the fix will also change the test and that diff shows how behavior changes.

Note that this is still not addressed.

From #15281 (comment)

Hi @epage, I implemented symlink preservation in write_atomic().

The test now shows how cargo add handles symlinks correctly. Let me know what you think.

So the test fails for the first commit and then passes for the second commit? In that case, these commits are not atomic, as in they do not stand on their own with every commit passing tests.

We ask for the splitting of test/fix commits so that the second commit shows how the behavior changes by how the test body changes to ensure it still passes. This is a clear way of communicating the way the behavior changed.

@RaghavenderSingh would you mind doing what epage suggested?

epage · 2025-03-24T14:19:50Z

@RaghavenderSingh you requested a review but it doesn't look like my review comments have been addressed.

src/cargo/util/toml_mut/manifest.rs

crates/cargo-util/src/paths.rs

weihanglo

This may need a rebase onto master to pass CI.

weihanglo · 2025-05-13T23:39:15Z

crates/cargo-util/src/paths.rs

+    let actual_path = if path.is_symlink() {
+        std::fs::read_link(path)?
+    } else {
+        path.to_path_buf()
+    };


To avoid unnecessary allocations. And we can continue call it path so we don't need to change other lines.
(It is good that shadowing is allowed in Rust)

Suggested change

let actual_path = if path.is_symlink() {

std::fs::read_link(path)?

} else {

path.to_path_buf()

};

let path = if path.is_symlink() {

&std::fs::read_link(path)?

} else {

path

};

weihanglo · 2025-05-13T23:43:05Z

src/cargo/util/toml_mut/manifest.rs

@RaghavenderSingh would you mind doing what epage suggested?

weihanglo · 2025-05-29T18:08:01Z

tests/testsuite/cargo_add/mod.rs

This test demonstrates that cargo add replaces symlinked Cargo.toml
files with regular files, breaking the symlink. The test currently
fails as expected.

FWIW, as previously noted, we prefer to the test pass in every commit.

This commit adding the test can be seen as a minimal reproducible example of the bugt.

When the test passes in the first commit, it captures the current problematic behavior. So, when the next real "fix" commit changes the behavior, the behavior change can show up in git diff.

This also helps if we want to bisect other unrelated things, so that the test suite won't be in a half-broken state

weihanglo · 2025-05-29T18:12:48Z

crates/cargo-util/src/paths.rs

-        let mode = meta.permissions().mode() & mask;
-
-        std::fs::Permissions::from_mode(mode)
+        const PERMS_MASK: u32 = 0o777;


This is unrelated, I guess? Could we keep it the old way?

weihanglo · 2025-05-29T18:14:02Z

crates/cargo-util/src/paths.rs

@@ -213,7 +222,7 @@ pub fn write_atomic<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Res
    tmp.write_all(contents.as_ref())?;

    // On unix platforms, set the permissions on the newly created file. We can use fchmod (called
-    // by the std lib; subject to change) which ignores the umask so that the new file has the same
+    // by the std lib method below) which ignores the umask so that the new file has the same


What do we change this? It is indeed subject to change

I know the old wording here might make less sense, but we should keep the focus on the issue, not other fly-by stuffs.

This test shows the current behavior where cargo add replaces symlinked Cargo.toml files with regular files. The test passes, documenting this problematic behavior.

weihanglo

Could you squah the last two commits into the second commit? They are unnecessary.

- Preserve symlinks when writing files atomically in write_atomic() - Update test to verify correct symlink preservation behavior - Apply rustfmt formatting This fixes the issue where cargo add would replace symlinked Cargo.toml files with regular files, breaking the symlink to the original target. Fixes rust-lang#15241

weihanglo

Thanks for your contribution!

Update cargo 5 commits in 64a12460708cf146e16cc61f28aba5dc2463bbb4..f6bebc3abbd9b273a457f77c31f8e93bc77f93fc 2025-05-30 18:25:08 +0000 to 2025-06-02 19:04:43 +0000 - fix(trim-paths): remap all paths to `build.build-dir` (rust-lang/cargo#15614) - test(trim-paths): enable more tests for windows-msvc (rust-lang/cargo#15621) - fix(fingerprint): explicit reason rather than "stale; unknown reason" (rust-lang/cargo#15617) - Fix cargo add overwriting symlinked Cargo.toml files (rust-lang/cargo#15281) - chore(deps): update alpine docker tag to v3.22 (rust-lang/cargo#15616) r? ghost

rustbot assigned weihanglo Mar 8, 2025

rustbot added A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2025

weihanglo reviewed Mar 8, 2025

View reviewed changes

tests/testsuite/cargo_add/symlink.rs Show resolved Hide resolved

RaghavenderSingh force-pushed the fix-cargo-add-symlinks branch from a45d668 to 53fdb96 Compare March 8, 2025 19:25

weihanglo reviewed Mar 9, 2025

View reviewed changes

tests/testsuite/cargo_add/symlink.rs Show resolved Hide resolved

tests/testsuite/cargo_add/symlink.rs Outdated Show resolved Hide resolved

tests/testsuite/cargo_add/symlink.rs Show resolved Hide resolved

RaghavenderSingh force-pushed the fix-cargo-add-symlinks branch 3 times, most recently from 465de3c to 1708e12 Compare March 9, 2025 20:04

epage reviewed Mar 10, 2025

View reviewed changes

src/cargo/util/toml_mut/manifest.rs Outdated Show resolved Hide resolved

epage reviewed Mar 10, 2025

View reviewed changes

src/cargo/util/toml_mut/manifest.rs Outdated Show resolved Hide resolved

RaghavenderSingh force-pushed the fix-cargo-add-symlinks branch from 1708e12 to 4f2006f Compare March 19, 2025 10:36

RaghavenderSingh requested review from epage and weihanglo March 19, 2025 11:16

epage reviewed Mar 19, 2025

View reviewed changes

RaghavenderSingh force-pushed the fix-cargo-add-symlinks branch 2 times, most recently from b0850ce to 26bd6b5 Compare March 19, 2025 13:12

RaghavenderSingh requested a review from epage March 24, 2025 09:24

RaghavenderSingh force-pushed the fix-cargo-add-symlinks branch from 1d94106 to bef4aa7 Compare March 24, 2025 14:46

epage reviewed Mar 24, 2025

View reviewed changes

src/cargo/util/toml_mut/manifest.rs Outdated Show resolved Hide resolved

epage reviewed Mar 24, 2025

View reviewed changes

src/cargo/util/toml_mut/manifest.rs Outdated Show resolved Hide resolved

RaghavenderSingh force-pushed the fix-cargo-add-symlinks branch from bef4aa7 to 976b828 Compare March 24, 2025 15:24

epage reviewed Mar 24, 2025

View reviewed changes

crates/cargo-util/src/paths.rs Outdated Show resolved Hide resolved

RaghavenderSingh force-pushed the fix-cargo-add-symlinks branch from 976b828 to 4896a35 Compare March 24, 2025 15:30

weihanglo requested changes May 13, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 13, 2025

RaghavenderSingh closed this May 29, 2025

RaghavenderSingh force-pushed the fix-cargo-add-symlinks branch from 4896a35 to bffece8 Compare May 29, 2025 17:16

RaghavenderSingh reopened this May 29, 2025

weihanglo reviewed May 29, 2025

View reviewed changes

test: Add test demonstrating cargo add replaces symlinks

35d2a30

This test shows the current behavior where cargo add replaces symlinked Cargo.toml files with regular files. The test passes, documenting this problematic behavior.

RaghavenderSingh force-pushed the fix-cargo-add-symlinks branch from 88e7f30 to 5d07fe5 Compare May 30, 2025 10:00

RaghavenderSingh requested a review from weihanglo May 30, 2025 10:03

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels May 30, 2025

weihanglo requested changes Jun 2, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2025

RaghavenderSingh force-pushed the fix-cargo-add-symlinks branch from e62156f to ecfe3a9 Compare June 2, 2025 05:00

RaghavenderSingh requested a review from weihanglo June 2, 2025 06:38

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jun 2, 2025

weihanglo approved these changes Jun 2, 2025

View reviewed changes

weihanglo added this pull request to the merge queue Jun 2, 2025

Merged via the queue into rust-lang:master with commit d794004 Jun 2, 2025
23 checks passed

weihanglo mentioned this pull request Jun 3, 2025

Update cargo rust-lang/rust#141963

Closed

weihanglo mentioned this pull request Jun 6, 2025

Update cargo rust-lang/rust#142109

Open

Fix cargo add overwriting symlinked Cargo.toml files #15281

Fix cargo add overwriting symlinked Cargo.toml files #15281

Uh oh!

Conversation

RaghavenderSingh commented Mar 8, 2025

What does this PR try to resolve?

How should we test and review this PR?

Uh oh!

rustbot commented Mar 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Mar 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!