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

feat: detect bzlmod files identifying workspace directory #697

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chickenandpork
Copy link

In #683 ibazel cannot find a workspace by WORKSPACE.bzlmod
In my own work, ibazel cannot find the workspace by MODULE.bazel

Generally, the workaround has been to create an empty file WORKSPACE.bazel (which appeared with #536, shipped in v0.25.3) to help ibazel but this is likely not the expected behaviour.

Solution is proposed in this PR:

  • looks for WORKSPACE.bzlmod, WORKSPACE.bazel, MODULE.bazel, falling back to WORKSPACE
  • unittests for all four
  • preserves the IsDir() check to protect against directories in path matching case-insensitive to these names
  • adds the WORKSPACE.bazel (Stream output when testing a single target #536) to the same case-insensitive-aware check and treats consistently to the other files
  • unittest for case-insensitive-matching directory: ignoring directory-matches in general

This solution is broader than proposed by @austinwoon (#684) -- but I only saw #684 after solving my own pain for myself. I would prefer @austinwoon be recognized for their contribution; I will happily rebase on #684 if it helps establish austinwoon in the contribution path.

Test

A simple test can be down with rules_gazebo, a small-ish ruleset that has workarounds:
0. assuming you're working in $HOME/src

  1. (cd ~/src && git clone git@github.com:bazelbuild/bazel-watcher.git)
  2. (cd ~/src && git clone https://github.com/gazebosim/rules_gazebo)
  3. (cd ~/src/bazel-watcher && bazel build //cmd/ibazel)
  4. sudo install -m0755 ~/src/bazel-watcher/bazel-bin/cmd/ibazel/ibazel_/ibazel /usr/local/bin/ (assuming it's on your $PATH)
  5. (cd ~/src/rules_gazebo && ibazel build //...) -- we expect the workarounds to allow this to work
  6. rm -f ~/src/rules_gazebo/WORKSPACE.b* -- delete the workarounds
  7. (cd ~/src/rules_gazebo && ibazel build //...) -- repaired ibazel still works here (due to MODULE.bazel)

Copy link

google-cla bot commented Jan 9, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@chickenandpork chickenandpork force-pushed the feat/detect-bzlmod-workspace-files branch from ccfc866 to f93b037 Compare January 9, 2025 20:43
@achew22
Copy link
Member

achew22 commented Jan 9, 2025

Hey @chickenandpork! Thanks so much for your contribution. Now all we have to do is get that CI passing and I'd be delighted to merge. Thanks again!

@chickenandpork chickenandpork force-pushed the feat/detect-bzlmod-workspace-files branch from 5195723 to 469d9fc Compare January 9, 2025 22:00
@chickenandpork
Copy link
Author

chickenandpork commented Jan 9, 2025

Hey @chickenandpork! Thanks so much for your contribution. Now all we have to do is get that CI passing and I'd be delighted to merge. Thanks again!

Awesome @achew22 -- problem is, I don't have anything running Windows to repro, but I think this will fix based on the log.

Hey @achew22 I see a bit of flake in the testing: things I didn't change passed before, failed now. The testing looks for certain text strings from bazel, but I don't think bazel promises to keep entropy away from text strings.

Has anyone looked at ignoring the output text that sometimes shows up?

@chickenandpork chickenandpork force-pushed the feat/detect-bzlmod-workspace-files branch from 469d9fc to de9fb40 Compare January 9, 2025 22:32
@achew22
Copy link
Member

achew22 commented Jan 9, 2025

Yeah, I don't have a Windows machine either. Sorry I can't be more helpful than to point you at the CI

@achew22
Copy link
Member

achew22 commented Jan 9, 2025

Sorry, my fingers are fat.

Hey @achew22 I see a bit of flake in the testing: things I didn't change passed before, failed now. The testing looks for certain text strings from bazel, but I don't think bazel promises to keep entropy away from text strings.

Has anyone looked at ignoring the output text that sometimes shows up?

I could be mistaken but I believe the strings it is looking for are emitted by iBazel. Give that we're in the same repo I'm ish-comfortable depending on them. If you see things that are from bazel and wanna rework the test to not depend on that, I'm very supportive.

@chickenandpork
Copy link
Author

Hey @achew22

I could be mistaken but I believe the strings it is looking for are emitted by iBazel. Give that we're in the same repo I'm ish-comfortable depending on them. If you see things that are from bazel and wanna rework the test to not depend on that, I'm very supportive.

This is a good sentiment; I appreciate that.

Bazel-watcher testing is looking for strings from Bazel. Exactly certain strings. No more, no less.

Unfortunately, bazel occasionally adds helpful INFO messages in some cases… inconsistently, in a way that makes “no more, no less” into “flaky unittest”. …I think that’s what’s happening.

I don’t think bazel expected that their stdout/stderr is suddenly an API, with specific static content, but Bazel-watcher unittests like it is.

I have frustration because Bazel has evolved and Bazel-watcher is not actually the same repo: one of the things I like about a monorepo is that the tests show what changes break other components. Ironic that in tools favoured for monorepos, we need to do a polyrepo pattern of catching up to the error from a while back.

having joined a new role, I have the time to fix the issue, but I worry on the unknown demands of fixing the unittest to permit the fix to merge and make ibazel usable again. I can try, but the new role may take priority.

do you use ibazel daily? If so , you likely have peppered the workaround in your repos. Would you consider running with this change, and stripping out the workaround?

@chickenandpork
Copy link
Author

In re-running the test, there is a change in behaviour from bazel, which causes some tests to fail.

Manually running bazel test //internal/e2e/error:error_test --action_env=HOME --cache_test_results=no causes ~every second test to fail.

Running with settings for flaky tests up to 5 shows 2 of 5, 3 of 5 to be failing but eventually a test will run without the extra output, and it passes.

Expanding the default timeout in internal/e2e/ibazel.go seems to get a better passing rate. I'm pushed that as de9fb40 on my side, or 166fc67

@achew22, is that too cowardly, to merely increase the timeout for this time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants