Normalize the `target` paths#14497

linyihai · 2024-09-05T07:09:49Z

What does this PR try to resolve?

The targets path of the normalized_toml could be relative, which isn't user-friendly.

What is this PR doing?

This PR applys the paths::normalize_path to remove the relative part.

Fixes #14227

How should we test and review this PR?

Add a test originted from the issue, and fixing it in the next commit.

Additional information

rustbot · 2024-09-05T07:09:54Z

r? @ehuss

rustbot has assigned @ehuss.
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-09-05T14:09:13Z

This PR does it during the conversion from TomlBinTarget to Target. Previously, #13729 did this during prepare_for_publish. I wonder if we could consolidate these by moving both to the resolve phase.

Also, this needs to be applied to all targets like #13729 did.

linyihai · 2024-09-09T12:44:26Z

This PR does it during the conversion from TomlBinTarget to Target. Previously, #13729 did this during prepare_for_publish. I wonder if we could consolidate these by moving both to the resolve phase.

Also, this needs to be applied to all targets like #13729 did.

In the read_manifest I had applied the paths::normalize_path to the normalized_toml.

But it is not very aceptable for custom build, see https://github.com/rust-lang/cargo/actions/runs/10760922235/job/29839507217, because custom build can be outside the current package, the .. will be stripped after normalized.

I also want to apply the normalize_path_sep to the targets in normalized_toml, but it fails. see https://github.com/rust-lang/cargo/actions/runs/10767799732/job/29855771840, the rustc will still output \\src\\lib, but the cargo is src/lib.rs

epage · 2024-09-09T16:41:25Z

But it is not very aceptable for custom build, see https://github.com/rust-lang/cargo/actions/runs/10760922235/job/29839507217, because custom build can be outside the current package, the .. will be stripped after normalized.

Hmm, sounds like normalize_path expects the path to be absolute. We should document and maybe add an assert about that.

That also makes me question using it elsewhere. I forgot that we leave these paths relative. I wonder if we should make the absolute during normalization and relative during publish.

linyihai · 2024-09-10T10:58:20Z

I wonder if we should make the absolute during normalization and relative during publish.

It's not work for vendor, the absolute path of the vendor contain the vendor project path, which is hard to strip it.

Previously, #13729 did this during prepare_for_publish. I wonder if we could consolidate these by moving both to the resolve phase.

This way is trickier than I thought.

linyihai · 2024-09-18T07:09:41Z

r? @epage

I wonder if we should make the absolute during normalization and relative during publish.

See my PR description, I think this problem has been improved, please continue to review and leave valuable comments

epage · 2024-09-18T13:43:23Z

See my PR description, I think this problem has been improved, please continue to review and leave valuable comments

Looking at

This adds a struct AbsolutePathTomlTarget to contruct the absolute path for the TargetToml, and apply the paths::normalize to eliminate the relative part.

its telling me what its doing. What I'm wondering is why this route was chosen. AbsolutePathTomlTarget seems like a fairly invasive change for what I'd expect.

linyihai · 2024-10-30T01:41:48Z

its telling me what its doing. What I'm wondering is why this route was chosen. AbsolutePathTomlTarget seems like a fairly invasive change for what I'd expect.

I have given up using this scheme and instead just absolutize the path in 'target' and do 'normalize_path'.

epage · 2024-10-30T19:54:29Z

But it is not very aceptable for custom build, see https://github.com/rust-lang/cargo/actions/runs/10760922235/job/29839507217, because custom build can be outside the current package, the .. will be stripped after normalized.

FYI I posted #14750

epage · 2024-10-30T19:56:21Z

src/cargo/util/toml/targets.rs

+        if let Some(PathValue(path)) = lib.path.as_ref() {
+            let path = package_root.join(path);
+            let path = paths::normalize_path(&path);
+            lib.path = Some(PathValue(path.into()));
+        }


If we're doing the joins here, should we remove the joins in the to_ functions?

epage · 2024-10-30T19:56:45Z

src/cargo/util/toml/targets.rs

        targets.push(Target::custom_build_target(
            &name,
-            package_root.join(custom_build),
+            paths::normalize_path(package_root.join(custom_build).as_path()),


This is happening during a to_ operation while all the rest are during a normalize operation

linyihai · 2024-10-31T15:42:35Z

FYI I posted #14750

Waiting on this PR to be merged.

If the paths::normalized_path can handle .. relative path, then the fixed solution doesn't need absolutize the target path.

fix(util): Respect all `..`s in `normalize_path` ### What does this PR try to resolve? The fact that `normalize_path` was only designed for absolute paths bit us when working out #14497 and so I decided to make sure it worked. The other alternative I considered was having it assert that the path was absolute. Since I did try out the assert and Cargo tests hit it, this likely fixes something but I haven't dug through to be able to say what. ### How should we test and review this PR? ### Additional information

linyihai · 2024-11-01T07:28:19Z

This PR does it during the conversion from TomlBinTarget to Target. Previously, #13729 did this during prepare_for_publish. I wonder if we could consolidate these by moving both to the resolve phase.

This PR chose this route.

That also makes me question using it elsewhere. I forgot that we leave these paths relative. I wonder if we should make the absolute during normalization and relative during publish.

This approach requires more changes and is not ideal. If I understand you correctly, this needs absolutize the target path in the normalize_toml, the to_real_manifest then assums target path is absolute and makes to_targests do not join the package_root. This is what the read_manifest will change.

If we accept that targets path had been absolutized, then it has a big drawback in prepare_for_publish, because in prepare_for_publish, it calls prepare_toml_for_publish to yield the normalize_toml with relative path targets, this normalize_toml doesn't meet the to_real_manifest what needs a normalize_toml with absolute path.

epage · 2024-11-04T21:18:11Z

src/cargo/util/toml/targets.rs

+        Some(StringOrBool::String(build_file)) => {
+            let build_file = paths::normalize_path(Path::new(build_file));
+            let build = build_file
+                .into_os_string()
+                .into_string()
+                .map_err(|_err| anyhow::format_err!("non-UTF8 `package.build`"))?;
+            Some(StringOrBool::String(build))


The error message is inaccurate as package.build can only be UTF-8 because StringOrBool::String is a String.

This could instead be

.expect("`build_file` started as a String and `normalize_path` shouldn't have changed that")

src/cargo/util/toml/targets.rs

epage · 2024-11-04T21:20:25Z

This approach requires more changes and is not ideal. If I understand you correctly, this needs absolutize the target path in the normalize_toml, the to_real_manifest then assums target path is absolute and makes to_targests do not join the package_root. This is what the read_manifest will change.

Thank you for your patience in this PR and working towards what is right,even if it has been a lot of churn on your side!

src/cargo/util/toml/targets.rs

…phase

epage · 2024-11-06T17:09:36Z

@bors r+

bors · 2024-11-06T17:09:38Z

📌 Commit a456c22 has been approved by epage

It is now in the queue for this repository.

bors · 2024-11-06T17:10:48Z

⌛ Testing commit a456c22 with merge d050945...

bors · 2024-11-06T17:43:21Z

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

Update cargo 16 commits in 0310497822a7a673a330a5dd068b7aaa579a265e..4a2d8dc636445b276288543882e076f254b3ae95 2024-11-01 19:27:56 +0000 to 2024-11-09 19:10:33 +0000 - test: adjust `cargo_test_env` to unblock rust submodule update (rust-lang/cargo#14803) - feat(warnings): add build.warnings option (rust-lang/cargo#14388) - Revert "feat: Add `CARGO_RUSTC_CURRENT_DIR`" (rust-lang/cargo#14799) - CI: make the `lint-docs` job required (rust-lang/cargo#14797) - Switch CI from bors to merge queue (rust-lang/cargo#14718) - docs(test): Document Execs assertions based on port effort (rust-lang/cargo#14793) - fix(test): Make redactions consistent with snapbox (rust-lang/cargo#14790) - test(gc): Update remaining unordered tests to snapbox (rust-lang/cargo#14785) - Normalize the `target` paths (rust-lang/cargo#14497) - rustfix: replace special-case duplicate handling with error (rust-lang/cargo#14782) - test: Update some emaining unordered tests to snapbox (rust-lang/cargo#14781) - Change config paths to only check CARGO_HOME for cargo-script (rust-lang/cargo#14749) - Enable transfer feature in triagebot (rust-lang/cargo#14777) - Add transactional semantics to `rustfix` (rust-lang/cargo#14747) - doc: fix `GlobalContext` reference (rust-lang/cargo#14773) - chore: update handlebars to v6, fix build error (rust-lang/cargo#14772)

rustbot assigned ehuss Sep 5, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2024

linyihai force-pushed the issue-14227 branch from e14e582 to 2fba2ad Compare September 7, 2024 12:13

rustbot added the A-manifest Area: Cargo.toml issues label Sep 7, 2024

linyihai force-pushed the issue-14227 branch from 2fba2ad to 0b72c74 Compare September 7, 2024 13:13

linyihai changed the title ~~Normalize the relative path in the bin table.~~ Normalize the target paths Sep 7, 2024

linyihai force-pushed the issue-14227 branch 2 times, most recently from d71aea5 to 90753b6 Compare September 7, 2024 15:07

linyihai marked this pull request as draft September 8, 2024 07:35

linyihai force-pushed the issue-14227 branch 7 times, most recently from ea8f33e to a15bb7a Compare September 9, 2024 10:00

linyihai marked this pull request as ready for review September 9, 2024 12:45

linyihai force-pushed the issue-14227 branch from a15bb7a to 910d06b Compare September 11, 2024 10:51

rustbot added the Command-vendor label Sep 11, 2024

linyihai force-pushed the issue-14227 branch 2 times, most recently from 9b4515c to ef810f6 Compare September 18, 2024 06:19

rustbot assigned epage and unassigned ehuss Sep 18, 2024

epage mentioned this pull request Oct 30, 2024

fix(util): Respect all ..s in normalize_path #14750

Merged

epage reviewed Oct 30, 2024

View reviewed changes

test: Add a test for issue rust-lang#14227

f8df39b

linyihai force-pushed the issue-14227 branch from f40216a to 99e8123 Compare November 1, 2024 06:45

epage reviewed Nov 4, 2024

View reviewed changes

src/cargo/util/toml/targets.rs Show resolved Hide resolved

linyihai force-pushed the issue-14227 branch from 99e8123 to fb346c4 Compare November 5, 2024 03:31

epage reviewed Nov 5, 2024

View reviewed changes

src/cargo/util/toml/targets.rs Outdated Show resolved Hide resolved

fix: Apply path::normalize_path to targets in toml::normalize_toml …

a456c22

…phase

linyihai force-pushed the issue-14227 branch from fb346c4 to a456c22 Compare November 6, 2024 07:06

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 Nov 6, 2024

bors merged commit d050945 into rust-lang:master Nov 6, 2024

bors mentioned this pull request Nov 6, 2024

Implement RFC3695: Allow boolean literals as cfg predicates #14649

Merged

linyihai deleted the issue-14227 branch November 7, 2024 02:07

weihanglo mentioned this pull request Nov 9, 2024

Update cargo rust-lang/rust#132829

Merged

rustbot added this to the 1.84.0 milestone Nov 10, 2024

Conversation

linyihai commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented Sep 5, 2024

Uh oh!

epage commented Sep 5, 2024

Uh oh!

linyihai commented Sep 9, 2024

Uh oh!

epage commented Sep 9, 2024

Uh oh!

linyihai commented Sep 10, 2024

Uh oh!

linyihai commented Sep 18, 2024

Uh oh!

epage commented Sep 18, 2024

Uh oh!

linyihai commented Oct 30, 2024

Uh oh!

epage commented Oct 30, 2024

Uh oh!

epage Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

epage Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

linyihai commented Oct 31, 2024

Uh oh!

linyihai commented Nov 1, 2024

Uh oh!

epage Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

epage commented Nov 4, 2024

Uh oh!

Uh oh!

epage commented Nov 6, 2024

Uh oh!

bors commented Nov 6, 2024

Uh oh!

bors commented Nov 6, 2024

Uh oh!

bors commented Nov 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

linyihai commented Sep 5, 2024 •

edited

Loading