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 renaming a project using build scripts #4818

Merged
merged 1 commit into from
Dec 18, 2017

Conversation

alexcrichton
Copy link
Member

This commit fixes an issue in Cargo where if a project's folder was
renamed but it also contained a build script the project could break.
Cargo would continue to use the previous rustc-link-search arguments
to configure env vars like LD_LIBRARY_PATH but the literal values from
the previous compilation would be stale as the directories would no
longer be there.

To fix this when parsing the build script output we now retain a log of
the previous output directory of a build script invocation as well as
the current output, tweaking paths as appropriate if they were contained
in the output folder.

Closes #4053

@rust-highfive
Copy link

r? @matklad

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

@matklad
Copy link
Member

matklad commented Dec 14, 2017

Am I correct that currently we don't set cwd for build scripts? If so, let's lock cwd here as well?

@matklad
Copy link
Member

matklad commented Dec 14, 2017

Nevermind, we do set cwd in fill_env

@@ -364,6 +391,13 @@ impl BuildOutput {
_ => bail!("Wrong output in {}: `{}`", whence, line),
};

let path = |val: &str| {
Copy link
Member

Choose a reason for hiding this comment

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

Could we, instead of post-processing after reading, do some pre-processing before writing, so as not to store absolute paths at the moment of generation at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm yeah I wasn't sure what was the best option here, we don't have a serialized format for Cargo's parsed data structures for the output file so it'd have to be added, it just wasn't clear to me how much of a benefit we'd get from it.

Do you think that the preprocessed version would be less buggy though?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there are any tangible benefits one way or the other, but not having absolute paths feels slightly cleaner. But I am fine either way!

@alexcrichton alexcrichton force-pushed the rename-link-paths branch 2 times, most recently from 0cbd399 to c3c9fae Compare December 15, 2017 15:26
@alexcrichton
Copy link
Member Author

@bors: r+

@bors
Copy link
Collaborator

bors commented Dec 18, 2017

📌 Commit d6f8f45 has been approved by alexcrichton

@alexcrichton
Copy link
Member Author

Er, @bors: r=matklad

@bors
Copy link
Collaborator

bors commented Dec 18, 2017

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Collaborator

bors commented Dec 18, 2017

📌 Commit d6f8f45 has been approved by matklad

@bors
Copy link
Collaborator

bors commented Dec 18, 2017

⌛ Testing commit d6f8f45979c39bd963f12cd56a5e14645dd1d14d with merge e783bdaa306eff6a86d43840d4319979a3512a15...

@bors
Copy link
Collaborator

bors commented Dec 18, 2017

💔 Test failed - status-travis

This commit fixes an issue in Cargo where if a project's folder was
renamed but it also contained a build script the project could break.
Cargo would continue to use the previous `rustc-link-search` arguments
to configure env vars like `LD_LIBRARY_PATH` but the literal values from
the previous compilation would be stale as the directories would no
longer be there.

To fix this when parsing the build script output we now retain a log of
the previous output directory of a build script invocation as well as
the current output, tweaking paths as appropriate if they were contained
in the output folder.

Closes rust-lang#4053
@alexcrichton
Copy link
Member Author

@bors: r=matklad

@bors
Copy link
Collaborator

bors commented Dec 18, 2017

📌 Commit f0c0a6a has been approved by matklad

@bors
Copy link
Collaborator

bors commented Dec 18, 2017

⌛ Testing commit f0c0a6a with merge 4bbfc70...

bors added a commit that referenced this pull request Dec 18, 2017
Fix renaming a project using build scripts

This commit fixes an issue in Cargo where if a project's folder was
renamed but it also contained a build script the project could break.
Cargo would continue to use the previous `rustc-link-search` arguments
to configure env vars like `LD_LIBRARY_PATH` but the literal values from
the previous compilation would be stale as the directories would no
longer be there.

To fix this when parsing the build script output we now retain a log of
the previous output directory of a build script invocation as well as
the current output, tweaking paths as appropriate if they were contained
in the output folder.

Closes #4053
@bors
Copy link
Collaborator

bors commented Dec 18, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 4bbfc70 to master...

@bors bors merged commit f0c0a6a into rust-lang:master Dec 18, 2017
@alexcrichton alexcrichton deleted the rename-link-paths branch December 21, 2017 15:16
bors added a commit that referenced this pull request Nov 18, 2018
Fix renaming directory project using build scripts with cross-compiling.

The rename-protection introduced in #4818 checks for paths with a prefix of `/…/target/debug`, but this does not work for paths used during cross-compiling.

This change checks for paths with the full `OUT_DIR` prefix, and the `build/PKG/root-output` file now includes that full `OUT_DIR` instead of `/…/target/debug`.

This also includes a few other changes:
- Support fixing KIND=PATH style paths.
- Support fixing paths in metadata (like `cargo:root=…` or `cargo:include=…` which are common).
- Adds a "version" value to the metadata hash to ensure that cargo doesn't get confused with the new `root-output` file format.

Fixes #6177
@ehuss ehuss added this to the 1.24.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo:rustc-link-search becomes invalid when you move a Cargo project
5 participants