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

Add flag --incompatible_use_plus_in_repo_names #23103

Closed
wants to merge 3 commits into from

Conversation

Wyverald
Copy link
Member

Defaults to false. When set to true, we use + instead of ~ as the separator in canonical repo names.

Some more subtle changes include:

  • We now officially say that the "build" part of version strings (the part that begins with a plus) is ignored and stripped.
  • When the flag is set to true, we effectively increase the lockfile version by 1 (see code comment in BazelLockFileModule).
  • When true, we no longer insert a _main in front of names of repos generated by module extensions hosted in the main repo. (~abc as a name was problematic, but +abc is not.)
  • When true, we no longer insert a v in front of numerical versions in canonical repo names. (my_mod~1.1 could be a Windows short path, but my_mod+1.1 cannot.)

Work towards #22865.

@Wyverald Wyverald requested a review from fmeum July 24, 2024 22:03
@Wyverald Wyverald marked this pull request as ready for review July 24, 2024 22:03
@github-actions github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Jul 24, 2024
Defaults to false. When set to true, we use `+` instead of `~` as the separator in canonical repo names.

Some more subtle changes include:
- We now officially say that the "build" part of version strings (the part that begins with a plus) is ignored and stripped.
- When the flag is set to true, we effectively increase the lockfile version by 1 (see code comment in BazelLockFileModule).
- When true, we no longer insert a `_main` in front of names of repos generated by module extensions hosted in the main repo. (`~abc` as a name was problematic, but `+abc` is not.)
- When true, we no longer insert a `v` in front of numerical versions in canonical repo names. (`my_mod~1.1` could be a Windows short path, but `my_mod+1.1` cannot.)

Work towards #22865.
Copy link
Collaborator

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

Looks very thorough, including removing the special cases that are now unnecessary.

How certain we are that + is better than any other alternatives?

@Wyverald
Copy link
Member Author

Wyverald commented Jul 25, 2024

How certain we are that + is better than any other alternatives?

Let's look at all printable non-alphanumeric ASCII characters:

Character Verdict Comments
! bad on Windows with the !ENVVAR! syntax?
" quoting hell (also disallowed on Windows)
# begins a UNIX shell comment
$ begins a UNIX shell env var
% begins a Windows env var
& special meaning in UNIX shell
' quoting hell
( bad for so many reasons
) bad for so many reasons
* wildcard (and disallowed on Windows)
+ candidate! (only weirdness is with Batch copy)
, common separator in arguments (and weird with Batch move, for one)
- valid in version names
. valid in version names
/ nightmare (and disallowed on Windows)
: disallowed on Windows
; UNIX Shell statement separator
< disallowed on Windows
= may be problematic in env etc.?
> disallowed on Windows
? wildcard (and disallowed on Windows)
@ conflicts with marker files? and ... ugly?
[ UNIX Shell special character
\ nightmare (and disallowed on Windows)
] UNIX Shell special character
^ Batch script special character (wtf Windows)
_ valid in version names
` UNIX Shell special character
{ UNIX Shell special character
| disallowed on windows
} UNIX Shell special character
~ old candidate :(

So it looks like + is the best candidate so far, with , and = being other potential choices. WDYT?

@fmeum
Copy link
Collaborator

fmeum commented Jul 25, 2024

@ is also a special character in Bash afaik, otherwise that could have maybe been an (ugly) option. I agree with your assessment, the comprehensive list is highly appreciated! Mind linking to it from a code comment?

@meteorcloudy
Copy link
Member

@Wyverald Can you file an incompatible issue to track this change? https://github.com/bazelbuild/bazel/issues/new/choose

@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jul 29, 2024
@meteorcloudy
Copy link
Member

@bazel-io fork 7.3.0

bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Jul 29, 2024
Defaults to false. When set to true, we use `+` instead of `~` as the separator in canonical repo names.

Some more subtle changes include:
- We now officially say that the "build" part of version strings (the part that begins with a plus) is ignored and stripped.
- When the flag is set to true, we effectively increase the lockfile version by 1 (see code comment in BazelLockFileModule).
- When true, we no longer insert a `_main` in front of names of repos generated by module extensions hosted in the main repo. (`~abc` as a name was problematic, but `+abc` is not.)
- When true, we no longer insert a `v` in front of numerical versions in canonical repo names. (`my_mod~1.1` could be a Windows short path, but `my_mod+1.1` cannot.)

Work towards bazelbuild#22865.

Closes bazelbuild#23103.

PiperOrigin-RevId: 657202616
Change-Id: I015b2a04a823b1d951015a1b2e1b99b154dcc5a2
github-merge-queue bot pushed a commit that referenced this pull request Jul 29, 2024
Defaults to false. When set to true, we use `+` instead of `~` as the
separator in canonical repo names.

Some more subtle changes include:
- We now officially say that the "build" part of version strings (the
part that begins with a plus) is ignored and stripped.
- When the flag is set to true, we effectively increase the lockfile
version by 1 (see code comment in BazelLockFileModule).
- When true, we no longer insert a `_main` in front of names of repos
generated by module extensions hosted in the main repo. (`~abc` as a
name was problematic, but `+abc` is not.)
- When true, we no longer insert a `v` in front of numerical versions in
canonical repo names. (`my_mod~1.1` could be a Windows short path, but
`my_mod+1.1` cannot.)

Work towards #22865.

Closes #23103.

PiperOrigin-RevId: 657202616
Change-Id: I015b2a04a823b1d951015a1b2e1b99b154dcc5a2

Commit
2f67e57

Co-authored-by: Xdng Yng <[email protected]>
@Wyverald Wyverald deleted the wyv-repo-plus-flag branch July 29, 2024 18:36
copybara-service bot pushed a commit that referenced this pull request Jul 31, 2024
This PR essentially flips `--incompatible_use_plus_in_repo_names` (introduced in #23103) to true on master. In addition, it graveyards the flag, removes all logic to use `~`, and fixes all tests to use `+` instead.

Closes #23098.

RELNOTES: The format of canonical repo names has changed to use plus (`+`) instead of tilde (`~`). Effectively, this flips the flag `--incompatible_use_plus_in_repo_names` to true, and the flag is now a no-op (i.e. cannot be "unflipped").
PiperOrigin-RevId: 657913333
Change-Id: Ia97609049871db7a914fe3129556cf843d06f571
@malkia
Copy link

malkia commented Aug 17, 2024

There was a slight, but I guess expected fallout, I had to switch some per_file_copt from

build:clang-cl --per_file_copt="external/protobuf~/.*@-Wno-invalid-offsetof"

to

build:clang-cl --per_file_copt="external/protobuf\\+/.*@-Wno-invalid-offsetof"

And because + is part of the regex language it needs to be escaped twice (escape the escaper). Other than that I haven't found other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants