Gazelle extension for Python #514

f0rmiga · 2021-08-20T17:31:56Z

Hi there! 👋

This is the initial Gazelle extension for Python.

A good starting point for reviewing is the added README file under gazelle/, the example workspace and the testdata directory.

More discussion on where Gazelle extensions should live here: bazel-contrib/bazel-gazelle#1030.

google-cla · 2021-08-20T17:32:00Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

alexeagle · 2021-08-20T17:32:53Z

@googlebot I consent.

I'm co-authoring this with Thulio.

thundergolfer · 2021-08-31T12:55:33Z

Thanks @f0rmiga can you add bazel-contrib/bazel-gazelle#1030 to the PR description?

Looks like something reviewers should read through given discussion in the bazelbuild Slack (note that Slack link will eventually die as the workspace has free-tier limited history).

thundergolfer

Reviewed the "starting point for reviewing" stuff so far. Good stuff. Will continue on tomorrow or day after.

Edit: Alright I'm back... 6 days later.

thundergolfer · 2021-09-01T13:49:04Z

gazelle/README.md

+modules_mapping(
+    name = "modules_map",
+    # This should point to wherever we declare our python dependencies
+    requirements = "//:requirements_lock.txt",


This is a 'lock' file. Does it require the requirements file to be transitively locked? If yes, it should be called out I think.

FWIW the public doc for the repo doesn't call this out (but maybe it should?)

The public doc also use "//path/to:requirements_lock.txt" so maybe we should use that here as well for consistency? (and down below in gazelle_python_manifest)

thundergolfer · 2021-09-01T13:53:13Z

gazelle/README.md

+
+Finally, you create a target that you'll invoke to run the Gazelle tool
+with the rules_python extension included. This typically goes in your root
+`/BUILD.bazel` file:


nit: Could be clearer to just say "target typically goes in your workspace root package, //:gazelle.

gazelle/README.md

thundergolfer

Had a scan through the Golang, and the testing looks nice and thorough. As I'm not the best person to do ongoing review of Golang code, can we get an ongoing reviewer added to .github/CODEOWNERS. They'll get assigned by Github as appropriate.

gazelle/kinds.go

examples/build_file_generation/BUILD

thundergolfer · 2021-09-14T09:00:35Z

.bazelversion

@@ -1 +1 @@
-3.3.1
+4.1.0


Do we need this change? We have #533 bumping the version.

that just landed, so no we shouldn't need this anymore

google-cla · 2021-09-30T21:20:33Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

joshua-cannon-techlabs

👋 Howdy! Very excited to see this PR, as someone who build-engineers at a company with way too much hand-maintained Python BUILD files 😁

In fact, ignoring all the other challenges we face with Bazel+Python, this is the largest IMO.

One high-level suggestion I have (possibly out of scope, but would love to see it): super-fine grained generation. This mode would be the default and essentially boils down to roughly one rule-per-file.

This would make builds/tests as fast as possible within Bazel. It'll also make the BUILD files noisy, but it's also generated so "meh".

To demonstrate it with an example:

Take this directory structure:

cheese_shop/
├─ __init__.py  # Just imports and re-exports from the _<cheese> files
├─ _caerphilly.py
├─ _caerphilly_test.py
├─ _emmental.py
├─ _emmental_test.py
├─ _japanese_sage_darby.py
├─ _japanese_sage_darby_test.py
├─ mister_wensleydale.py  # public subpackage
├─ mister_wensleydale_test.py

Ideally, there'd be:

5 py_librarys:
- One for each _<cheese>.py (Maybe this could even has it's visibility marked, due to the leading-underscore convention?)
- One for __init__.py. This would be the one that other libraries depend on.
- One for mister_wensleydale.py, a public subpackage
4 py_tests. One for each *_test.py

That way if I only edit _caerphilly.py, Ideally only the _caerphilly_test.py test rule will need to be run (and not the other tests).

joshua-cannon-techlabs · 2021-10-19T14:00:03Z

gazelle/README.md

+
+### Tests
+
+Python test files are those ending in `_test.py`.


I think it'd be prudent to also look for other standard patterns.

unittest's discovery's pattern is test* by default: https://docs.python.org/3/library/unittest.html#cmdoption-unittest-discover-p
pytest allows both test_ and _test: https://docs.pytest.org/en/6.2.x/goodpractices.html#test-discovery

And/or allow me to specify the pattern (through a directive?) 😁

This is already supported! https://github.com/bazelbuild/rules_python/pull/514/files#diff-56956604a1d66006e9135f231c19bf1799aa19370f382ccd94842a88856bce78R86

We only need to update the documentation to be clear. unittest should be supported if you write a template for it and use the target output as __test__.py and the generation target named __test__.

joshua-cannon-techlabs · 2021-10-19T14:02:37Z

gazelle/README.md

+modules_mapping(
+    name = "modules_map",
+    # This should point to wherever we declare our python dependencies
+    requirements = "//:requirements_lock.txt",


FWIW the public doc for the repo doesn't call this out (but maybe it should?)

The public doc also use "//path/to:requirements_lock.txt" so maybe we should use that here as well for consistency? (and down below in gazelle_python_manifest)

joshua-cannon-techlabs · 2021-10-19T14:14:27Z

gazelle/README.md

+
+Python test files are those ending in `_test.py`.
+
+A `py_test` target is added containing all test files as `srcs`.


This might be out of scope for this PR, but this might be a nonstarter for projects using a custom test rule (which is suuuuuuuper common, since rules_python lacks pytest support today and pytest is the ubiquitous Python test library/runner).

Is this possible to customize through directives?

I'll expand this section. It's more complicated than what we stated so far.

joshua-cannon-techlabs · 2021-10-19T14:40:10Z

gazelle/README.md

+    modules_mapping = "@modules_map//:modules_mapping.json",
+    # This is what we called our `pip_install` rule, where third-party
+    # python libraries are loaded in BUILD files.
+    pip_deps_repository_name = "pip",


The public doc for this repo uses my_deps, let's be consistent?

f0rmiga · 2021-10-27T20:59:12Z

@joshua-cannon-techlabs Thanks for your input. I considered the file-grained approach, but not as default. I shall change a bit the generation mode options before landing this. I won't add the file-grained mode before landing, but will leave space for it to be added in the near future. So the options supported now would be:

# gazelle:python_generation_mode package
# gazelle:python_generation_mode project

and then for the future:

# gazelle:python_generation_mode file

google-cla · 2021-10-27T22:50:38Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-10-27T23:14:26Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

f0rmiga · 2021-10-27T23:14:42Z

@googlebot I consent.

google-cla · 2021-10-27T23:14:51Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-10-27T23:34:10Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

f0rmiga · 2021-10-27T23:44:38Z

@thundergolfer The MacOS Buildkite runner is missing the os module from Python. Any idea why?

google-cla · 2021-11-10T00:22:48Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Co-authored-by: Jonathon Belotti <jonathon@canva.com>

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Co-authored-by: Jonathon Belotti <jonathon@canva.com>

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

With the recent change in pypa/setuptools#2769, some wheels started to fail build immediately with an unpinned setuptools in isolation mode. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Also add the python_generation_mode directive. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

hrfuller

LGTM. A few nits that can be addressed in a follow up.

hrfuller · 2021-11-16T22:49:09Z

gazelle/modules_mapping/builder.py

+
+
+def build(wheel):
+    print("{}: building {}".format(datetime.now(), wheel), file=sys.stderr)


prefer an f-string here

f-strings are Python 3.6 only. Don't we care about earlier Python versions?

I agree we should avoid depending on interpreter version where possible/convenient. I don't know what assertions we have to keep ourselves honest about it.

hrfuller · 2021-11-16T22:57:35Z

gazelle/modules_mapping/builder.py

+            print("STDOUT:", file=sys.stderr)
+            print(process.stdout, file=sys.stderr)
+            print("STDERR:", file=sys.stderr)
+            print(process.stderr, file=sys.stderr)


this could be turned into a more readable multi-line string

hrfuller · 2021-11-16T23:00:37Z

gazelle/modules_mapping/builder.py

+def build(wheel):
+    print("{}: building {}".format(datetime.now(), wheel), file=sys.stderr)
+    process = subprocess.run(
+        [sys.executable, "-m", "build", "--wheel", "--no-isolation"], cwd=wheel


Is assuming that .tar.gz packages have a build module a packaging standard? Do some need setup.py run?

I'm not sure I understand this question. The build module here is provided by https://packaging.python.org/key_projects/#build, which implements https://www.python.org/dev/peps/pep-0517/.

After it builds the wheel from the source, we dig the file with generator.py in this same Python package.
Is your question around needing to run setup.py instead of looking at the packaged wheel only?

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Performing the check last is more correct and yields better performance, noticeable on large repositories. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

f0rmiga requested review from andyscott and thundergolfer as code owners August 20, 2021 17:31

google-cla bot added the cla: no label Aug 20, 2021

google-cla bot added cla: yes and removed cla: no labels Aug 20, 2021

alexeagle force-pushed the gazelle branch 2 times, most recently from b548584 to b48b1c8 Compare August 20, 2021 17:41

pcj mentioned this pull request Aug 24, 2021

Build puller/loader from source for repository rules bazelbuild/rules_docker#1918

Open

12 tasks

thundergolfer requested review from hrfuller and removed request for andyscott August 31, 2021 12:52

thundergolfer reviewed Sep 1, 2021

View reviewed changes

thundergolfer reviewed Sep 7, 2021

View reviewed changes

gazelle/kinds.go Outdated Show resolved Hide resolved

examples/build_file_generation/BUILD Outdated Show resolved Hide resolved

f0rmiga force-pushed the gazelle branch 2 times, most recently from cb39cbc to a795f91 Compare September 14, 2021 05:34

thundergolfer reviewed Sep 14, 2021

View reviewed changes

alexeagle force-pushed the gazelle branch from 164e2a1 to 9025a11 Compare September 30, 2021 21:20

google-cla bot added cla: no and removed cla: yes labels Sep 30, 2021

joshua-cannon-techlabs reviewed Oct 19, 2021

View reviewed changes

f0rmiga force-pushed the gazelle branch from 9025a11 to 3df1413 Compare October 27, 2021 22:50

alexeagle force-pushed the gazelle branch from a3c22dd to d0f9b63 Compare November 10, 2021 00:22

google-cla bot added cla: yes and removed cla: no labels Nov 10, 2021

alexeagle and others added 13 commits November 16, 2021 13:57

Initial upstream of gazelle plugin

05d1f1f

Add new example to --deleted_packages

bc46fe9

Update examples/build_file_generation/BUILD

a4dc73a

Co-authored-by: Jonathon Belotti <jonathon@canva.com>

fix: gazelle:exclude on coarse-grained

0675500

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: comment on Kinds()

6b8dbda

Co-authored-by: Jonathon Belotti <jonathon@canva.com>

owner: f0rmiga

28b798b

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: build and setuptools pinned versions

68989b5

With the recent change in pypa/setuptools#2769, some wheels started to fail build immediately with an unpinned setuptools in isolation mode. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

refactor: use local_repository in examples

053f99d

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

bump: examples Bazel version

d0293e3

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: add missing .gitignore to example

323d740

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

refactor: remove python_coarse_grained_generation

6a4d1d1

Also add the python_generation_mode directive. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: gazelle spam from org_golang_x_tools

f60c229

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

revert: example .bazelversion

779e50e

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

f0rmiga force-pushed the gazelle branch from d0f9b63 to 779e50e Compare November 16, 2021 21:59

hrfuller approved these changes Nov 16, 2021

View reviewed changes

f0rmiga added 4 commits November 16, 2021 15:04

fix: simplify std_modules.py

201f470

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

feat: test py_library without __init__.py

530ae5e

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

feat: manifest generation tag manual

4b403c8

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: check std modules last

a58208c

Performing the check last is more correct and yields better performance, noticeable on large repositories. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

f0rmiga force-pushed the gazelle branch from 5021a63 to a58208c Compare November 16, 2021 23:27

alexeagle merged commit a5a7ffb into bazelbuild:main Nov 17, 2021

alexeagle deleted the gazelle branch November 17, 2021 03:10

stevebarrau mentioned this pull request Apr 25, 2022

Add gazelle extension for Java bazel-contrib/rules_jvm#19

Merged

dougthor42 mentioned this pull request Aug 5, 2024

Gazelle incorrectly ignores files called setup.py. #2108

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gazelle extension for Python #514

Gazelle extension for Python #514

f0rmiga commented Aug 20, 2021 •

edited

Loading

google-cla bot commented Aug 20, 2021

alexeagle commented Aug 20, 2021

thundergolfer commented Aug 31, 2021

thundergolfer left a comment •

edited

Loading

thundergolfer Sep 1, 2021

joshua-cannon-techlabs Oct 19, 2021

thundergolfer Sep 1, 2021

thundergolfer left a comment

thundergolfer Sep 14, 2021

alexeagle Sep 30, 2021

google-cla bot commented Sep 30, 2021

joshua-cannon-techlabs left a comment •

edited

Loading

joshua-cannon-techlabs Oct 19, 2021

joshua-cannon-techlabs Oct 19, 2021

f0rmiga Oct 27, 2021

joshua-cannon-techlabs Oct 19, 2021

joshua-cannon-techlabs Oct 19, 2021

f0rmiga Oct 27, 2021

joshua-cannon-techlabs Oct 19, 2021

f0rmiga commented Oct 27, 2021 •

edited

Loading

google-cla bot commented Oct 27, 2021

google-cla bot commented Oct 27, 2021

f0rmiga commented Oct 27, 2021

google-cla bot commented Oct 27, 2021

google-cla bot commented Oct 27, 2021

f0rmiga commented Oct 27, 2021

google-cla bot commented Nov 10, 2021

hrfuller left a comment

hrfuller Nov 16, 2021

f0rmiga Nov 16, 2021

alexeagle Nov 17, 2021

hrfuller Nov 16, 2021

hrfuller Nov 16, 2021

f0rmiga Nov 16, 2021


		Python test files are those ending in `_test.py`.

		A `py_test` target is added containing all test files as `srcs`.



		def build(wheel):
		print("{}: building {}".format(datetime.now(), wheel), file=sys.stderr)

		@@ -1 +1 @@
		3.3.1
		4.1.0

Gazelle extension for Python #514

Gazelle extension for Python #514

Conversation

f0rmiga commented Aug 20, 2021 • edited Loading

google-cla bot commented Aug 20, 2021

alexeagle commented Aug 20, 2021

thundergolfer commented Aug 31, 2021

thundergolfer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thundergolfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cla bot commented Sep 30, 2021

joshua-cannon-techlabs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0rmiga commented Oct 27, 2021 • edited Loading

google-cla bot commented Oct 27, 2021

google-cla bot commented Oct 27, 2021

f0rmiga commented Oct 27, 2021

google-cla bot commented Oct 27, 2021

google-cla bot commented Oct 27, 2021

f0rmiga commented Oct 27, 2021

google-cla bot commented Nov 10, 2021

hrfuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0rmiga commented Aug 20, 2021 •

edited

Loading

thundergolfer left a comment •

edited

Loading

joshua-cannon-techlabs left a comment •

edited

Loading

f0rmiga commented Oct 27, 2021 •

edited

Loading