Fix cargo add overwriting symlinked Cargo.toml files #15281
Conversation
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
rustbot
added
A-manifest
RaghavenderSingh
force-pushed
the
fix-cargo-add-symlinks
branch
from
a45d668 to
53fdb96
Compare
RaghavenderSingh
force-pushed
the
fix-cargo-add-symlinks
branch
3 times, most recently
from
465de3c to
1708e12
Compare
RaghavenderSingh
force-pushed
the
fix-cargo-add-symlinks
branch
from
1708e12 to
4f2006f
Compare
src/cargo/util/toml_mut/manifest.rs
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Note that this is still not addressed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
fix-cargo-add-symlinks
branch
from
eec6ec3 to
b0850ce
Compare
RaghavenderSingh
force-pushed
the
fix-cargo-add-symlinks
branch
from
b0850ce to
26bd6b5
Compare
|
@RaghavenderSingh you requested a review but it doesn't look like my review comments have been addressed. |
RaghavenderSingh
force-pushed
the
fix-cargo-add-symlinks
branch
from
1d94106 to
bef4aa7
Compare
RaghavenderSingh
force-pushed
the
fix-cargo-add-symlinks
branch
from
bef4aa7 to
976b828
Compare
RaghavenderSingh
force-pushed
the
fix-cargo-add-symlinks
branch
from
976b828 to
4896a35
Compare
What does this PR try to resolve?
This PR fixes a bug where
cargo addbreaks symlinks to Cargo.toml files. Currently, when Cargo.toml is a symlink andcargo addis 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 addbreaks 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.rsthat:cargo addto add a dependencyI've manually tested this fix and confirmed it works correctly.