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

build: do not run pip install outside the bazel build framework #4647

Merged
merged 11 commits into from
Oct 31, 2024

Conversation

jiceatscion
Copy link
Contributor

I was sick and tired of stepping on that Lego every time I built on a new machine...

make mocks, was running mocks.py directly instead of calling bazel run. That used whatever ambient python and libraries to match instead of the python env specified by WORKSPACE, which meant that we had to include pip install in the tools/install_deps process, which often enough would fail or require no-end of messy work-arounds (e.g. create a python venv and remember to activate it before building - but just the mocks).

along with the dependdencies.
This should also allow us to do away with pip_install as a pre-build
step (and get rid of the associated shenanigans).
…The only

reason to do this was "make mocks" which bypassed bazel. Bazel is bypassed
no-more.
@jiceatscion
Copy link
Contributor Author

This change is Reviewable

@jiceatscion jiceatscion requested review from JordiSubira and a team and removed request for lukedirtwalker October 29, 2024 10:02
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)

scion.sh was still using togen.py, topodot.py and set_ipv6_addr.py
directly; with the consequence that any significant deviation between the
python environment in the build tree and that of the global environment could
cause these tools to fail.

Instead, we now produce py_binaries for these three tools, install them in
bin/, and use them from there.

To make them work when not invoked from bazel, we have to package them as self
contained executables, which is achieved by adding the --build_python_zip
command line flag. Suprisingly, this is the only method available. There isn't
even an equivalent parameter to the py_binary rule. All other workarounds turn
out to do crazy things (e.g. pkg_tar with the "include_run_files" option
produces non-sense). The tools are requested as part of make all.

Another case of calling python scripts outside bazel was
router_benchmark/benchmark.py. For that one, I arranged so it can be invoked
through bazel (like its twin brother test.py) instead of only from the command
line.
For some very mysterious reasons, flake8 gets broken by build_python_zip.
I have never been able to find how exactly flake8 is made as a py binary
because its creations is mediated by 6 layers of bazel code generation. But
it is indeed made during the build.

If I turn build_python_zip ON for the "build" config only, then flake8 is still
broken, but if I turn it ON everywhere except for the "test:lint" config,
then flake8 works. That makes no sense to me, but I'll be content with that.
Life is short and I am sick of spending mine trying to deconstruct bazel
and python frivolous and broken magic.
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)

jiceatscion and others added 3 commits October 30, 2024 15:36
Deal with more bazel/python non-sense. py_binary cannot be made with an
imported entry point. There has to be a redundant local wrapper.
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @JordiSubira)


tools/supervisorctl.py line 5 at r5 (raw file):

# This is exact replica of supervisord's entry point because I haven't
# found an intelligible way of creating a py_binary target that doesn't

have you seen https://rules-python.readthedocs.io/en/latest/api/rules_python/python/entry_points/py_console_script_binary.html#py_console_script_binary.binary_rule ?

Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JordiSubira and @lukedirtwalker)


tools/supervisorctl.py line 5 at r5 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

have you seen https://rules-python.readthedocs.io/en/latest/api/rules_python/python/entry_points/py_console_script_binary.html#py_console_script_binary.binary_rule ?

Had seen the page but not recognized this section as relevant to my case. (I was also seriously running out of patience with the whole thing). I'll give that a try. See if I can make it work.

Copy link
Contributor Author

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 13 files reviewed, 1 unresolved discussion (waiting on @JordiSubira and @lukedirtwalker)


tools/supervisorctl.py line 5 at r5 (raw file):

Previously, jiceatscion wrote…

Had seen the page but not recognized this section as relevant to my case. (I was also seriously running out of patience with the whole thing). I'll give that a try. See if I can make it work.

seems to work. so, done

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)

@jiceatscion jiceatscion merged commit bae8d39 into scionproto:master Oct 31, 2024
5 checks passed
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