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

Remap $PWD to empty string instead of "." #1563

Merged
merged 2 commits into from
Sep 27, 2022
Merged

Conversation

goffrie
Copy link
Contributor

@goffrie goffrie commented Sep 23, 2022

This fixes an inconsistency in file paths (included in e.g. panic info), wherein some paths would have a leading ./ and some would not.

I believe specifically generic code that is instantiated across a crate boundary would end up with an absolute path, then have it remapped to ., whereas other code would use the literal path passed on the command line to rustc, which is generally a relative path with no ./ prefix.

@google-cla
Copy link

google-cla bot commented Sep 23, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@krasimirgg
Copy link
Collaborator

Could this yield rewritten paths that look like absolute paths, e.g., if $pwd is /path/to/dir and a relative path in a message is to subdir/lib.rs, replacing $pwd with the empty string could yield messages with a path like /subdir/lib.rs, which looks like an absolute path and is confusing IMO.

@goffrie
Copy link
Contributor Author

goffrie commented Sep 26, 2022 via email

@krasimirgg
Copy link
Collaborator

Looks good then! @goffrie could you address this PR's google-cla check?

@goffrie
Copy link
Contributor Author

goffrie commented Sep 26, 2022

Done.

@UebelAndre
Copy link
Collaborator

In my testing, rustc is smart and does not do that (it strips the leading slash).

Could this be demonstrated by a test?

@goffrie
Copy link
Contributor Author

goffrie commented Sep 26, 2022

I added a test and manually verified that it fails without this change (the path has a leading ./).

Copy link
Collaborator

@krasimirgg krasimirgg left a comment

Choose a reason for hiding this comment

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

Thank you!

@krasimirgg krasimirgg merged commit c57e7a3 into bazelbuild:main Sep 27, 2022
@goffrie goffrie deleted the remap2 branch December 6, 2022 00:09
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.

3 participants