Normalize the `target` paths

[linyihai]

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 makes the target path absolute then apply the paths::normalize_path to remove the relative part.
• As the path is absolute, so it needs to strip the package_root during packaging.

The package.build field doesn't really apply to this process as it's not stores as a PATH, it needs to change to path and apply the above process then turn it to string, thats very clumsy.

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]

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]

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]

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]

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]

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, https://github.com/rust-lang/cargo/pull/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]

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]

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.

[bors]

:umbrella: The latest upstream changes (presumably #14591) made this pull request unmergeable. Please resolve the merge conflicts.

[linyihai]

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]

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

[linyihai]

FYI I posted https://github.com/rust-lang/cargo/pull/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.

[linyihai]

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]

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!

[epage]

@bors r+

[bors]

:pushpin: Commit a456c225318c5210ce15bd40fa1cffdcd289a7df has been approved by epage

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit a456c225318c5210ce15bd40fa1cffdcd289a7df with merge d050945fe44585755781d36efbaf8d05d85585a5...

[bors]

:sunny: Test successful - checks-actions Approved by: epage Pushing d050945fe44585755781d36efbaf8d05d85585a5 to master...