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

support MODULE.bazel file #647

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

klandergren
Copy link
Contributor

addresses #646

  • verified that ibazel with this patch can correctly watch and run the target in the linked issue
  • verified that //internal/ibazel/workspace:workspace_test passes

@achew22
Copy link
Member

achew22 commented Mar 7, 2024

I think this is a great change! Would you be willing to add a new e2e test based on the simple test case but instead of having the test change BUILD files, having it change the MODULE.bazel file? I'd be very happy to merge as soon as something like that was in

@klandergren
Copy link
Contributor Author

Thanks @achew22! What specific changes to a MODULE.bazel should the new e2e test exercise?

I ask because I noticed that none of the tests in simple_test.go test modifications to WORKSPACE, which is the closest equivalent file to what MODULE.bazel replaces. My understanding is that both WORKSPACE and MODULE.bazel are used to do things like load and resolve external dependencies which—I think—would involve external network requests which would not be desired for testing.

I considered loading a macro via load but after getting an error found that only a subset of methods are available for use in MODULE.bazel files.

In light of this my opinion is that an e2e test exercising MODULE.bazel may not be necessary but of course I defer to you.

@klandergren
Copy link
Contributor Author

I did some experimentation and investigation and found that sourceQuery and buildQuery are the two queries that control which files ibazel pays attention to when detecting changes. While it does pass the parent directories of these outputs to fsevents it filters events (by path) against the results of these two queries. Neither result contains WORKSPACE or MODULE.bazel, so to support an end-to-end test that detects changes in those files would be more involved and probably need its own separate PR.

@farhaven
Copy link

@achew22 Is there a way to get this PR merged?

@achew22 achew22 enabled auto-merge (rebase) May 12, 2024 17:37
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.

None yet

3 participants