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

VFS: add RootedPathAndCasing #10357

Closed
wants to merge 1 commit into from

Conversation

laszlocsomor
Copy link
Contributor

@laszlocsomor laszlocsomor commented Dec 3, 2019

This class stores a RootedPath and the String
value of its relative part.

This class allows distinguishing RootedPaths on
case-insensitive filesystems, if two RootedPaths
refer to the same file but use different casing.

The motivation is to support validating package
path casing. (See issue #8799.) My plan is to add
a new SkyFunction called PathCasingLookupFunction,
which validates that a certain RootedPath is
correctly cased. The SkyKey can't just take a
RootedPath, because on Windows the path semantics
dictate that RootedPath objects are compared
without regard to casing. Thus we must also store
the particular casing in the SkyKey, and so we
need the RootedPathWithCasing class.

The class only compares the casing of the relative
parts, not of the root parts. Reason is, this
class will be used to validate the casing of
package paths, which are always relative to the
package root whose casing we needn't worry about.

See #8799

This class stores a RootedPath and the String
value of its relative part.

This class allows distinguishing RootedPaths on
case-insensitive filesystems, if the RootedPaths
refer to the same file but use different casing.

The class only compares the casing of the relative
parts, not of the root parts. Reason is, this
class will be used to validate the casing of
package paths, which are always relative to the
package root whose casing we needn't worry about.

See bazelbuild#8799

Change-Id: Ie742f95a7feb3935c93cbba6db9278b63cfcc63a
@meteorcloudy
Copy link
Member

The change itself looks good. But can you explain a bit more how are we going to use this? Replacing RootedPath? And will it cause significant memory regression?

@laszlocsomor
Copy link
Contributor Author

laszlocsomor commented Dec 3, 2019

Sure! I updated the commit message to explain the purpose of the class.

Good question about memory tax. I haven't measured it.
What I do know is I'll put the new path-casing validation logic behind an experimental flag. Unless you enable that flag, no extra SkyFunctions will be computed, hence no memory cost.

This class will not replace RootedPath; it builds on top of it.

Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks for the explaining!

@bazel-io bazel-io closed this in be5630b Dec 4, 2019
@laszlocsomor laszlocsomor deleted the path-casing-a branch December 4, 2019 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants