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

Declare rules_python deps #175

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Declare rules_python deps #175

merged 4 commits into from
Nov 29, 2023

Conversation

katre
Copy link
Member

@katre katre commented Oct 24, 2023

[Previous text moved to #179]

Add correct lxml dep in rules_python setup, fixing #179.

@katre
Copy link
Member Author

katre commented Oct 25, 2023

Okay, after some work and updates I at least have consistent failures: without bzlmod, enforce_min_sdk_floor can't run because the python toolchain can't find the binary dependencies on lxml (this is an assumption but seems justified after checking similar error reports in other programs).

I don't know why bzlmod works here, and what it's doing differently.

@katre katre changed the title enforce_min_sdk_version requires lxml, which isn't present Declare rules_python deps Oct 30, 2023
@katre
Copy link
Member Author

katre commented Oct 30, 2023

Managed to set up a docker image that mimics CI, and it looks like the (successful) bzlmod version is using python 3.11, and the (unsuccessful) non-bzlmod version is using python 3.8:

11c11
<     lxml/site-packages/lxml/_elementpath.cpython-311-x86_64-linux-gnu.so,
---
>     lxml/site-packages/lxml/_elementpath.cpython-38-x86_64-linux-gnu.so,
14c14
<     lxml/site-packages/lxml/builder.cpython-311-x86_64-linux-gnu.so,
---
>     lxml/site-packages/lxml/builder.cpython-38-x86_64-linux-gnu.so,

@katre
Copy link
Member Author

katre commented Oct 30, 2023

Okay, that solved it for linux, mac intel and mac arm apparently need separate vendored_requirements files because of the interpreter difference. I'll try a bit more and see if I can poke this.

@ted-xie
Copy link
Contributor

ted-xie commented Nov 13, 2023

Thanks for looking into this!

Managed to set up a docker image that mimics CI, and it looks like the (successful) bzlmod version is using python 3.11, and the (unsuccessful) non-bzlmod version is using python 3.8:

wat, there are two different versions of python used in two different build pipelines in BazelCI?


--extra-index-url https://pypi.python.org/simple/

lxml
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no specific version of lxml specified here, so pip will always try to pull the latest available one, right? In that case, wouldn't the requirements_lock.txt file be invalidated frequently?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it's locked to the latest version when the lockfile was last updated.

It'd probably be better to explicitly specify a version, but that's down the chain after "make the thing work cross-platform".

@katre
Copy link
Member Author

katre commented Nov 13, 2023

Yes, the Mac workers and the Ubuntu workers have different versions installed.

Ideally, we wouldn't have system python on either, and just use a hermetic python toolchain for everything.

To summarize the problem for rules_android:

  1. To use a hermetic python toolchain is easy with bzlmod
    1. We don't publish a bzlmod version of rules_android for users, even though it's possible to build inside rules_android with bzlmod
  2. To use a hermetic python toolchain without bzlmod, we'd need a third-stage WORKSPACE macro (after rules_android_prereqs and rules_android_workspace
    1. It's possible to vendor the needed deps files, except that they have architecture dependencies, so can't work for us.

This only works for bzlmod: non-bzlmod will break.

Trying to fix bazelbuild#179.
This only works for bzlmod: non-bzlmod will break.

Trying to fix bazelbuild#179.
This will work for linux but not Mac.
@copybara-service copybara-service bot merged commit de047db into bazelbuild:main Nov 29, 2023
1 of 2 checks passed
@katre katre deleted the fix-lxml branch December 14, 2023 15:11
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