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.

Sign up for GitHub

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

Jump to bottom

Apply buildifier to MODULE.bazel. #19796

Closed
wants to merge 1 commit into from
Closed

Apply buildifier to MODULE.bazel. #19796

wants to merge 1 commit into from

Conversation

katre
Copy link
Member

@katre katre commented Oct 11, 2023 •
edited
Loading

Also add a note about how to update the lockfile.

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Oct 11, 2023
@fmeum
Copy link
Collaborator

fmeum commented Oct 11, 2023

DO NOT MERGE: why does this break tests?

@katre I think that's because the MODULE.bazel.lock file is now out of date (e.g. Starlark Locations changed). Could you run bazel mod deps --lockfile_mode=update and then bazel run //src/test/tools/bzlmod:update_default_lock_file and commit the result?

@katre
Copy link
Member Author

katre commented Oct 11, 2023

I had no idea this was needed. Added a NOTE to MODULE.bazel and an error message in bazel_bootstrap_distfile_test that might help someone (probably me) in the future.

@iancha1992 iancha1992 added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Oct 11, 2023
fmeum
fmeum reviewed Oct 12, 2023
View reviewed changes
MODULE.bazel Outdated
# NOTE: When editing this file, also update the lockfile.
# bazel mod deps --lockfile_mode=update
# bazel build //src/test/tools/bzlmod:update_default_lock_file
# bazel-bin/src/test/tools/bzlmod/update_default_lock_file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this just be bazel run instead of build plus manual run? This may also only be needed when MODULE.tools is changed - cc @meteorcloudy .

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is only needed when MODULE.tools is changed.

And you actually have to use bazel run //src/test/tools/bzlmod:update_default_lock_file since it needs BUILD_WORKING_DIRECTORY to be set.

Copy link
Member Author

Choose a reason for hiding this comment

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

When I tried bazel run:

$ bazel run //src/test/tools/bzlmod:update_default_lock_file
INFO: Analyzed target //src/test/tools/bzlmod:update_default_lock_file (0 packages loaded, 1 target configured).
INFO: Found 1 target...
Target //src/test/tools/bzlmod:update_default_lock_file up-to-date:
  bazel-bin/src/test/tools/bzlmod/update_default_lock_file
INFO: Elapsed time: 0.381s, Critical Path: 0.01s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: bazel-bin/src/test/tools/bzlmod/update_default_lock_file
Running: /usr/local/google/home/jcater/.cache/bazel/_bazel_jcater/a85a0dacd9e20e18c47928db0f1530e0/execroot/_main/bazel-out/k8-fastbuild/bin/src/bazel mod deps
Extracting Bazel installation...
Starting local Bazel server and connecting to it...
WARNING: --enable_bzlmod is set, but no MODULE.bazel file was found at the workspace root. Bazel will create an empty MODULE.bazel file. Please consider migrating your external dependencies from WORKSPACE to MODULE.bazel. For more details, please refer to https://github.com/bazelbuild/bazel/issues/18958.
<root> (@_)

But it did update src/test/tools/bzlmod/MODULE.bazel.lock.

When I split into bazel build .. && ...:

 bazel build //src/test/tools/bzlmod:update_default_lock_file && bazel-bin/src/test/tools/bzlmod/update_default_lock_file
INFO: Analyzed target //src/test/tools/bzlmod:update_default_lock_file (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //src/test/tools/bzlmod:update_default_lock_file up-to-date:
  bazel-bin/src/test/tools/bzlmod/update_default_lock_file
INFO: Elapsed time: 0.343s, Critical Path: 0.01s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

No error, but src/test/tools/bzlmod/MODULE.bazel.lock wasn't changed.

This is incredibly confusing.

Copy link
Member

@meteorcloudy meteorcloudy Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The bash runfile libraries seems to fail with no error message when directly running the bash binary. @fmeum Am I using the runfiles library correctly? https://github.com/bazelbuild/bazel/blob/013683f6364dd15209aee83a71df58e3bfb4aff1/src/test/tools/bzlmod/update_default_lock_file.sh

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are, this is a bug. I will submit a fix.

Copy link
Member

Choose a reason for hiding this comment

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

A fix for this issue has been included in Bazel 7.0.0 RC2. Please test out the release candidate and report any issues as soon as possible. Thanks!

@katre
Copy link
Member Author

katre commented Oct 12, 2023

Pushed a few more commits to clarify messages and format more files.

meteorcloudy
meteorcloudy approved these changes Oct 13, 2023
View reviewed changes
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! I'll try to make update_default_lock_file less confusing.

@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 13, 2023
@fmeum fmeum mentioned this pull request Oct 13, 2023
Closed
@iancha1992
Copy link
Member

@katre Can you please resolve the conflicts?

@katre
Copy link
Member Author

katre commented Oct 13, 2023

Resolved and pushed.

@katre
Copy link
Member Author

katre commented Oct 13, 2023

Argh, import is breaking because this changes third_party/googleapis/MODULE.bazel and third_party/remoteapis/MODULE.bazel along with non-third_party files.

I'll try and fix this.

@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 16, 2023
@katre katre deleted the clean-modules branch October 16, 2023 17:33
copybara-service bot pushed a commit that referenced this pull request Oct 17, 2023
@fmeum @copybara-github
Fix Bash runfiles calling repository lookup for directly run scripts
58ce112 
When an `sh_binary` is run directly from `bazel-bin`, its path won't be contained in the manifest and will not match a `bazel-out` regex. Instead, make the path absolute and then match after the `bazel-bin` to extract the binaries repository name.

Also improve the error message when `runfiles_current_repository` can find neither the runfiles manifest nor the runfiles directory.

Work towards fixing #19796 (comment)

Closes #19810.

PiperOrigin-RevId: 574208158
Change-Id: I45d6b503e8f34e13177d57ca64c202640306cfb8
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Oct 17, 2023
@fmeum @bazel-io
Fix Bash runfiles calling repository lookup for directly run scripts
c8eea75 
When an `sh_binary` is run directly from `bazel-bin`, its path won't be contained in the manifest and will not match a `bazel-out` regex. Instead, make the path absolute and then match after the `bazel-bin` to extract the binaries repository name.

Also improve the error message when `runfiles_current_repository` can find neither the runfiles manifest nor the runfiles directory.

Work towards fixing bazelbuild#19796 (comment)

Closes bazelbuild#19810.

PiperOrigin-RevId: 574208158
Change-Id: I45d6b503e8f34e13177d57ca64c202640306cfb8
@bazel-io bazel-io mentioned this pull request Oct 17, 2023
Merged
meteorcloudy pushed a commit that referenced this pull request Oct 18, 2023
@bazel-io @fmeum
[7.0.0] Fix Bash runfiles calling repository lookup for directly run …
da1a6d7 
…scripts (#19857)

When an `sh_binary` is run directly from `bazel-bin`, its path won't be
contained in the manifest and will not match a `bazel-out` regex.
Instead, make the path absolute and then match after the `bazel-bin` to
extract the binaries repository name.

Also improve the error message when `runfiles_current_repository` can
find neither the runfiles manifest nor the runfiles directory.

Work towards fixing
#19796 (comment)

Closes #19810.

Commit
58ce112

PiperOrigin-RevId: 574208158
Change-Id: I45d6b503e8f34e13177d57ca64c202640306cfb8

Co-authored-by: Fabian Meumertzheim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iancha1992 iancha1992 iancha1992 left review comments

@fmeum fmeum fmeum left review comments

@meteorcloudy meteorcloudy meteorcloudy approved these changes

Assignees
No one assigned
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@katre @fmeum @iancha1992 @meteorcloudy