Skip to content

Conversation

@Veykril
Copy link
Member

@Veykril Veykril commented May 4, 2023

Closes #14728
cc #14430

  • Removes canonicalization (and forbids it from being used in a sense, clippy could help here again with its lint in the future)
  • Normalizes all paths in rust-project.json which we weren't doing in some cases

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2023
@Veykril
Copy link
Member Author

Veykril commented May 4, 2023

r? @matklad

(the stackoverflow post you linked outlines that clang also accepts relative paths to base, but I think given for this file we expect it to be generated anyways there is little use for allowing that)

@Veykril
Copy link
Member Author

Veykril commented May 4, 2023

Note to self: Comment on #4887 regarding the changes from this PR

@matklad
Copy link
Contributor

matklad commented May 4, 2023

Tbh, I’d keep relative paths, resolving relative to the location of the file seems like a common idiom, and it’s convenient to, eg, have several checkouts in different dirs using git worktree, or to check the file into VCS.

What we should avoid is cwd-relative paths, but that’s why the abspath infra exists.

@Veykril Veykril force-pushed the forbid-relative-json branch from 8beee3f to 939ebb4 Compare May 4, 2023 12:51
@Veykril Veykril changed the title Forbid relative paths in rust-project.json internal: Forbid canonicalization of paths and normalize all rust-project.json paths May 4, 2023
ProjectJson {
sysroot: data.sysroot.map(|it| base.join(it)),
sysroot_src: data.sysroot_src.map(|it| base.join(it)),
sysroot: data.sysroot.map(absolutize),
Copy link
Contributor

Choose a reason for hiding this comment

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

just

data.sysroot.map(|it| base.join(it)),

should do the trick. That's the semantics of Rust's join: if the path is absolute, base is ignored. It is a bit surprising, perhaps we can benefit from

impl AbsPath {
    pub fn absolutize(&self, path: impl AsRef<Path>) -> AbsPathBuf;
    pub fn join(&self, path: &RelPath) -> AbsPathBuf;
}
impl RelPath {
    pub fn join(&self, path: &RelPath) -> RelPathBuf;
}

@Veykril
Copy link
Member Author

Veykril commented May 13, 2023

@bors r+

@bors
Copy link
Contributor

bors commented May 13, 2023

📌 Commit f47caa6 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 13, 2023

⌛ Testing commit f47caa6 with merge a7944a9...

@bors
Copy link
Contributor

bors commented May 13, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing a7944a9 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Canonicalizing the rust-project.json breaks our VFS

4 participants