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

Make lockfile's RepoSpec attributes more readable #19684

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Sep 30, 2023

String-valued tag and repo rule attributes in the MODULE.bazel.lock file have to be serialized in a way that distinguishes them from label-valued attributes. This commit uses the fact that the string representation of labels always starts with @@ to escape strings only if they start with @@ (or happen to be of the form of an escaped string). Since string-valued attributes rarely start with @@, in particular when specified by humans and not macros, this greatly reduces the need for escaping in the lockfile.

This commit raises the lockfile version to 3.

Along the way also disables HTML escaping for attributes as it isn't needed and results in less readable representations of certain characters.

@fmeum fmeum force-pushed the prettier-lockfile branch 5 times, most recently from 03b2f8f to 0f32f31 Compare September 30, 2023 16:23
@meteorcloudy
Copy link
Member

To fix the bootstrap and determinism tests, you'll have to update the hack at:
https://cs.opensource.google/search?q=%22lockFileVersion%5C%22:%201%2F%5C%22lockFileVersion%5C%22:%202%2F%22&sq=&ss=bazel

@fmeum fmeum marked this pull request as ready for review October 2, 2023 14:49
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Oct 2, 2023
@fmeum fmeum force-pushed the prettier-lockfile branch 2 times, most recently from bcb5951 to e864e28 Compare October 2, 2023 19:48
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 3, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Oct 3, 2023
@iancha1992
Copy link
Member

@bazel-io fork 6.4.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Oct 3, 2023
@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Oct 4, 2023
@copybara-service copybara-service bot closed this in 5bdb208 Oct 5, 2023
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Oct 5, 2023
@iancha1992
Copy link
Member

@meteorcloudy @Wyverald @SalmaSamy @fmeum There are some conflicts when I try to cherry-pick this to release-6.4.0

  1. scripts/bootstrap/compile.sh
link_file "${PWD}/src/MODULE.tools" "${BAZEL_TOOLS_REPO}/MODULE.bazel"
new_hash=$(shasum -a 256 "${BAZEL_TOOLS_REPO}/MODULE.bazel" | awk '{print $1}')
sed -i.bak "/\"bazel_tools\":/s/\"[a-f0-9]*\"/\"$new_hash\"/" MODULE.bazel.lock

is not in the release-6.4.0 branch. But it's in the master branch.

  1. src/test/shell/bazel/bazel_determinism_test.sh
# Set up the maven repository properly.
cp derived/maven/BUILD.vendor derived/maven/BUILD
# Update the hash of bazel_tools in lockfile to avoid rerunning module resolution.
new_hash=$(shasum -a 256 "src/MODULE.tools" | awk '{print $1}')
sed -i.bak "/\"bazel_tools\":/s/\"[a-f0-9]*\"/\"$new_hash\"/" MODULE.bazel.lock
# TODO: Temporary hack for lockfile version mismatch, remove these two lines after updating to 6.4.0

is not in the release-6.4.0 branch. But it's in the master branch.

  1. src/tools/bzlmod/utils.bzl does not exist in the releae-6.4.0 branch at all currently

cc: @bazelbuild/triage

@fmeum
Copy link
Collaborator Author

fmeum commented Oct 5, 2023

@meteorcloudy Could you resolve these conflicts? I'm not sure I understand which branch needs which version of this workaround.

@fmeum fmeum deleted the prettier-lockfile branch October 5, 2023 22:04
@meteorcloudy
Copy link
Member

Np, I can do it!

meteorcloudy pushed a commit to meteorcloudy/bazel that referenced this pull request Oct 6, 2023
String-valued tag and repo rule attributes in the `MODULE.bazel.lock` file have to be serialized in a way that distinguishes them from label-valued attributes. This commit uses the fact that the string representation of labels always starts with `@@` to escape strings only if they start with `@@` (or happen to be of the form of an escaped string). Since string-valued attributes rarely start with `@@`, in particular when specified by humans and not macros, this greatly reduces the need for escaping in the lockfile.

This commit raises the lockfile version to 3.

Along the way also disables HTML escaping for attributes as it isn't needed and results in less readable representations of certain characters.

Closes bazelbuild#19684.

PiperOrigin-RevId: 571037360
Change-Id: I04bb60d371cd09326780004122e729d78c99732a
@meteorcloudy
Copy link
Member

We just need to drop all three conflict changes: #19748

meteorcloudy added a commit that referenced this pull request Oct 6, 2023
String-valued tag and repo rule attributes in the `MODULE.bazel.lock`
file have to be serialized in a way that distinguishes them from
label-valued attributes. This commit uses the fact that the string
representation of labels always starts with `@@` to escape strings only
if they start with `@@` (or happen to be of the form of an escaped
string). Since string-valued attributes rarely start with `@@`, in
particular when specified by humans and not macros, this greatly reduces
the need for escaping in the lockfile.

This commit raises the lockfile version to 3.

Along the way also disables HTML escaping for attributes as it isn't
needed and results in less readable representations of certain
characters.

Closes #19684.

PiperOrigin-RevId: 571037360
Change-Id: I04bb60d371cd09326780004122e729d78c99732a

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 6.4.0 RC2. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

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.

5 participants