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

Allow : in target and filenames if not the first character #23335

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Aug 19, 2024

Especially in the Perl community, filenames containing : are common. Labels can still be parsed unambiguously if target names can contain colons by declaring that the first colon in a given label string is interpreted as the separator between package and target name. This only requires prefixing with : when referring to targets containing colons in the same package.

Target and filenames starting with : are still not allowed to prevent confusion with references to targets.

Work towards #374

RELNOTES: Target and filenames can now contain : as long as it is not the first character. Same-package references to such targets have to be prefixed with :.

Especially in the Perl community, filenames containing `:` are common. Labels can still be parsed unambiguously if target names can contain colons by declaring that the first colon in a given label string is interpreted as the separator between package and target name. This only requires prefixing with `:` when referring to targets containing colons in the same package.

Target and filenames starting with `:` are still not allowed to prevent confusion with references to targets.

H
@fmeum fmeum changed the title Allow ':' in target and filenames if not the first character Allow : in target and filenames if not the first character Aug 19, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 19, 2024

@meisterT I'm trying to make some progress on #374 and #4327. What do you think of the approach taken by this PR? Space and colon appear to be the most commonly used characters unsupported by Bazel and it looks like supporting colon wouldn't be too problematic with a change like this.

@fmeum fmeum marked this pull request as ready for review August 19, 2024 18:21
@fmeum fmeum requested review from meisterT and removed request for gregestren and fweikert August 19, 2024 18:21
@github-actions github-actions bot added team-Documentation Documentation improvements that cannot be directly linked to other team labels awaiting-review PR is awaiting review from an assigned reviewer labels Aug 19, 2024
@Wyverald
Copy link
Member

There was a lengthy internal debate about how to handle such things. @brandjon

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 19, 2024

There are more tests that expect to fail when encountering a :. I will fix those up if the PR is otherwise viewed favorably.

@fmeum fmeum added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc and removed team-Documentation Documentation improvements that cannot be directly linked to other team labels labels Aug 19, 2024
@brandjon
Copy link
Member

Thanks for the PR.

We had a lot of internal discussion about admitting more label characters, and came up with a design that we didn't prioritize implementation of yet. I've paged it out of my head by now, but a key feature is that it adds an escaping scheme rather than allowing raw colons in the target part of the name. This is important to avoid breaking existing code (both in Starlark and in tooling that runs outside of the build itself) that expects to be able to parse labels based on naive matching of the colon character.

If we prioritize this capability, I'd be happy to discuss the design in more detail, but we don't have the bandwidth at the moment. In the meantime, it would be dangerous for us to allow raw colons in labels, and also dangerous to extend label syntax in other ways we haven't deeply thought through, since it's hard to reclaim pieces of the namespace we've given away.

@brandjon brandjon closed this Aug 20, 2024
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Core Skyframe, bazel query, BEP, options parsing, bazelrc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants