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

Canonicalize input paths in source rewriter #479

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

randomPoison
Copy link
Contributor

@randomPoison randomPoison commented Dec 10, 2024

Make RootDirectory and OutputDirectory canonical paths immediately after reading CLI args. #432 makes them absolute, but we need them to be fully canonical to reliably compare.

@fw-immunant
Copy link
Contributor

We might prefer llvm::sys::fs::make_absolute just to follow the pattern of using llvm functions here and avoid the <filesystem> include.

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Isn't this already done in #432?

@randomPoison
Copy link
Contributor Author

Oh weird you're right. I'm pretty sure I've synced and rebuilt IA2 since then, so I'm not sure why I was still seeing issues with relative paths. I'll dig into it on my end, maybe I just didn't have the latest version if IA2.

@randomPoison
Copy link
Contributor Author

Seems like the logic added in #432 isn't correctly canonicalizing RootDirectory and OutputDirectory. If I run the rewriter with --root-directory=.. --output-directory=. and have the rewriter print out RootDirectory and OutputDirectory, I get:

RootDirectory: /home/legare/zlib/build/..
OutputDirectory: /home/legare/zlib/build/.

It looks like make_absolute doesn't fully canonicalize paths:

Path before make_absolute: ..
Path after make_absolute: /home/legare/zlib/build/..

@kkysen
Copy link
Contributor

kkysen commented Dec 13, 2024

Does it need to fully canonicalized them or just make them absolute? If we need full canonicalization, can you fix #432 to use the canonicalization function instead? Either LLVM's if it has one or std's if it doesn't like you did here.

@ayrtonm
Copy link
Contributor

ayrtonm commented Dec 13, 2024

Does it need to fully canonicalized them or just make them absolute?

I think it has to be canonical because we compare with paths we get from querying clang for things.

This fixes some issues with the rewriter when non-canonical paths are specified for the root or output directories.
@randomPoison randomPoison force-pushed the legare/cononicalize-paths branch from 3ccaad1 to dd23c93 Compare December 13, 2024 19:34
@kkysen
Copy link
Contributor

kkysen commented Dec 13, 2024

Does it need to fully canonicalized them or just make them absolute?

I think it has to be canonical because we compare with paths we get from querying clang for things.

Ah, let's canonicalize then.

@randomPoison
Copy link
Contributor Author

Yeah I'm seeing bad behavior from the rewriter (e.g. trying to copy files to the wrong directory) with the current absolute path logic. Things only seem to work with a fully canonicalized path.

I've updated the PR to use remove_dots after making the path absolute. I couldn't find a direct canonicalize path function, but there's an internal one in LLVM that just does make_absolute followed by remove_dots, and it works when I test with relative paths.

@randomPoison randomPoison merged commit e7efee3 into main Dec 16, 2024
34 checks passed
@randomPoison randomPoison deleted the legare/cononicalize-paths branch December 16, 2024 19:49
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.

4 participants