-
Notifications
You must be signed in to change notification settings - Fork 542
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
docs(examples): Add example of using a src
dir and separate tests
dir with gazelle
#1842
base: main
Are you sure you want to change the base?
Conversation
I can't seem to figure out why builds are failing. Any tips? If I run $ cd examples/bzlmod_python_src_dir_with_separate_tests_dir/
$ bazel test //...
INFO: Analyzed 10 targets (4 packages loaded, 24 targets configured).
INFO: Found 7 targets and 3 test targets...
INFO: Elapsed time: 20.303s, Critical Path: 18.58s
INFO: 13 processes: 7 internal, 2 local, 4 processwrapper-sandbox.
INFO: Build completed successfully, 13 total actions
//:gazelle_python_manifest.test PASSED in 0.2s
//:requirements_test PASSED in 16.2s
//tests:test_my_python_module PASSED in 0.6s
Executed 3 out of 3 tests: 3 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are. If I run from the git repo root, I get the same failures seen in CI ( $ cd -
/c/dev/rules_python
$ bazel test //...
ERROR: Skipping '//...': error loading package under directory '': error loading package 'examples/bzlmod_python_src_dir_with_separate_tests_dir': Unable to find package for @@[unknown repo 'pypi' requested from @@]//:requirements.bzl: The repository '@@[unknown repo 'pypi' requested from @@]' could not be resolved: No repository visible as '@pypi' from main repository.
ERROR: error loading package under directory '': error loading package 'examples/bzlmod_python_src_dir_with_separate_tests_dir': Unable to find package for @@[unknown repo 'pypi' requested from @@]//:requirements.bzl: The repository '@@[unknown repo 'pypi' requested from @@]' could not be resolved: No repository visible as '@pypi' from main repository.
INFO: Elapsed time: 25.549s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Couldn't start the build. Unable to run tests From what I can tell, the only differences between this example and
But I tried making both of those edits locally and still got the same error. Note: I'm ignoring the MacOS and Windows CI failures for now. |
Please run the pre-commit hooks that will update .bazelignore, which should fix the errors seen in the tests running from the root. Edit: or the .bazelrc, can't remember which one in this case. |
Thanks. Turns out it's both I also noticed that the pre-commit hook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought I had submitted these review comments, but it seems that I had not done this. Happy to move this forward.
I think we may want to add a bazel-in-bazel
integration test somewhere here.
@@ -0,0 +1,69 @@ | |||
# Define metadata about this repository/project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having an empty WORKSPACE
file may fix some of the issues you saw with the pre-commit hook, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... still no dice. Same issue.
Given that CI is passing, I'm not too concerned about the pre-commit check. My assumption is that it's something specific to my computer/setup rather than specific to this branch, as pre-commit run --all-files
also fails on the latest main
commit 3730803.
# In the old WORKSPACE file, this would be 4 items: | ||
# 1. `load` the http_archive rule | ||
# 2. run the http_archive rule, grapping rules_python from github | ||
# 3. load the py_repositories target from rules_python | ||
# 4. execute py_respositories() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just link to the relevant example (e.g. pip_parse
or pip_parse_vendored
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relative file references added, but I kept the original list because I find it easier to follow if I don't have to jump around to various other examples. LMK if you feel strongly about removing the list.
) | ||
|
||
# Install rules_python, which allows us to define how bazel should work with python files. | ||
# See https://github.com/bazelbuild/rules_python/blob/main/examples/bzlmod/MODULE.bazel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use relative file references? At least in vim
users could use gf
to go to the file instantly and I hope other editors might support something similar.
# See https://github.com/bazelbuild/rules_python/blob/main/examples/bzlmod/MODULE.bazel | |
# See ../bzlmod/MODULE.bazel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, updated here and for gazelle/README.md below.
# Use the bazel downloader for pulling pypi packages. | ||
experimental_index_url = "https://pypi.org/simple", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
requirements_lock = "//:requirements.lock", | ||
) | ||
|
||
# Same as WORKSPACE install_deps() - actually install the python deps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not technically correct. Here we just expose the repo to be used by the module and the install happens lazily by bazel. I do think the intention is good - to draw parallels between WORKSPACE
and MODULE.bazel
, but this example might be something that people may read without any WORKSPACE
knowledge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good to know! Updated, PTAL and check for correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I had submitted these review comments
I do the same things all the time. I really wish GitHub had some "you have an unsubmitted review pending" notification or something.
But anyway, thanks for the comments! And don't worry about the delay! You've already been super responsive elsewhere, and adding a new example is a low priority.
I think we may want to add a bazel-in-bazel integration test somewhere here.
Can you elaborate on this?
@@ -0,0 +1,69 @@ | |||
# Define metadata about this repository/project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... still no dice. Same issue.
Given that CI is passing, I'm not too concerned about the pre-commit check. My assumption is that it's something specific to my computer/setup rather than specific to this branch, as pre-commit run --all-files
also fails on the latest main
commit 3730803.
) | ||
|
||
# Install rules_python, which allows us to define how bazel should work with python files. | ||
# See https://github.com/bazelbuild/rules_python/blob/main/examples/bzlmod/MODULE.bazel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, updated here and for gazelle/README.md below.
# In the old WORKSPACE file, this would be 4 items: | ||
# 1. `load` the http_archive rule | ||
# 2. run the http_archive rule, grapping rules_python from github | ||
# 3. load the py_repositories target from rules_python | ||
# 4. execute py_respositories() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relative file references added, but I kept the original list because I find it easier to follow if I don't have to jump around to various other examples. LMK if you feel strongly about removing the list.
requirements_lock = "//:requirements.lock", | ||
) | ||
|
||
# Same as WORKSPACE install_deps() - actually install the python deps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good to know! Updated, PTAL and check for correctness.
I've been thinking about this PR and how do we organise it together with other gazelle docs that we have. We are running short on CI executors so just adding this example into the CI is not so straight forward. For running What is more, if we merge this PR, does it trump the other example on Given that this example may be more complex than the existing gazelle example I would be +1 on replacing the existing example with this or merging them in some way. Sorry that this is dragging so long, but I am a bit lost with how we teach our users to use Am I overthinking it here? What are your thoughts? We can have a call to chat this because I do think that having an async messaging is slowing us down. Shall we get in touch on the bazel slack? |
I don't have a slack account, sorry. I could do Google Meet if that works for you. No, you're not overthinking this at all - you have great concerns and know a bunch more about the project than I do. For example, I didn't know about
I would say that there should be a single "main" example to follow, ideally with a limited feature set consisting of the most common use cases (how many projects need IMO the feature set for the main example should be:
We don't need the requirement locking (
I'd say that depends on the goals of Do people want Or is
Same. When we make a decision I can work on that.
+100 on that. Incorrect docs can be worse than no docs, haha. |
Decision from our video chat: integrate this example into the existing
|
Add an example of using a
src
dir and separatetests
dir andhaving gazelle generate targets correctly so that tests can be run.
Fixes #1775.