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

Normalize the target paths #14497

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

linyihai
Copy link
Contributor

@linyihai linyihai commented Sep 5, 2024

What does this PR try to resolve?

This adds a struct AbsolutePathTomlTarget to contruct the absolute path for the TargetToml, and apply the paths::normalize to eliminate 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
Copy link
Collaborator

rustbot commented Sep 5, 2024

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

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

epage commented Sep 5, 2024

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.

@rustbot rustbot added the A-manifest Area: Cargo.toml issues label Sep 7, 2024
@linyihai linyihai changed the title Normalize the relative path in the bin table. Normalize the target paths Sep 7, 2024
@linyihai linyihai force-pushed the issue-14227 branch 2 times, most recently from d71aea5 to 90753b6 Compare September 7, 2024 15:07
@linyihai linyihai marked this pull request as draft September 8, 2024 07:35
@linyihai linyihai force-pushed the issue-14227 branch 7 times, most recently from ea8f33e to a15bb7a Compare September 9, 2024 10:00
@linyihai
Copy link
Contributor Author

linyihai commented Sep 9, 2024

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

@linyihai linyihai marked this pull request as ready for review September 9, 2024 12:45
@epage
Copy link
Contributor

epage commented Sep 9, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@rustbot rustbot assigned epage and unassigned ehuss Sep 18, 2024
@epage
Copy link
Contributor

epage commented Sep 18, 2024

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
Copy link
Collaborator

bors commented Sep 27, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The file path in the diagnostic message contains duplicate separators on windows
5 participants