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

Add MODULE.bazel #263

Open
jacky8hyf opened this issue Dec 8, 2023 · 9 comments
Open

Add MODULE.bazel #263

jacky8hyf opened this issue Dec 8, 2023 · 9 comments

Comments

@jacky8hyf
Copy link

It appears that on Bazel Central Registry (BCR), a patch is needed to add MODULE.bazel so this can work as a Bazel module.

See https://github.com/bazelbuild/bazel-central-registry/blob/main/modules/abseil-py/1.4.0/MODULE.bazel.

Can someone consider adding MODULE.bazel to this Git repository directly?

I did not issue a pull request yet because the latest version is 2.0.0, unlike 1.4.0 on the BCR. I am not sure whether the compatibility level should be 1 or 2.

Filing this issue for discussion.

@yilei
Copy link
Contributor

yilei commented Dec 11, 2023

Reading https://bazel.build/external/migration, looks like we can add a MODULE.bazel file in parellel with WORKSPACE. Once that's added, when someone registers the new version of abseil-py in BCR, patches like this is no longer needed in the new entry.

Reading https://bazel.build/external/module#compatibility_level, it should be 2 since we follow SemVer.

Mind send a PR?

@rickeylev
Copy link
Contributor

Ah, looks like someone on the Bazel team created this module to aid the android migration to Bazel.

Are you sure you want to use bzlmod to pull in absl-py? The alternative is to use e.g. rules_python pip.parse() to pull it in.

I am not sure whether the compatibility level should be 1 or 2.

For Yilei's context: the gist of what the MODULE compatibility level does is force an upgrade. It's basically saying that a version changed enough that you have to change your dependency specifications. The major version part of a module ("1" from "1.4.0", in this case) doesn't automatically become the compatibility level.

In requirements.txt language, it's like if pypi package resolution refused to cross a major version boundary. e.g. if one put absl-py >= 1.0, it was treated as absl-py >= 1.0, < 2.0. Or if when a locked requirements file was updated, it refused to go from foo=1.2.3 to foo=2.0.0 unless some more manual action was taken.

So, question to @yilei is then: Are the changes between 1.4 and 2.0 significant enough they should be treated as requiring an upgrade (i.e. any user module setting bazel_dep(name="absl_py", version="1.XXX" must change it to "2.XXX")? If so, compatibility level should be increased to 2. If not, leaving it at 1 is fine.

@yilei
Copy link
Contributor

yilei commented Dec 11, 2023

Hmm, the compatibility level seems tricky. The main compatibility issue is abseil-py 2.0.0 dropped support for Python 3.6. So if one is using Python 3.6, it requires absl-py >= 1.0, < 2.0. For Python 3.7+ users, compatibility_level can stay 1. But Python version isn't a concept in bzlmod.

@jacky8hyf
Copy link
Author

Are you sure you want to use bzlmod to pull in absl-py? The alternative is to use e.g. rules_python pip.parse() to pull it in.

The reason we don't want to use pip is that, for Kleaf, we want to build in an environment without Internet access. We always only want to use repo/git to pull the sources, then ideally stop using the Internet, and run bazel to start building.

If we have bazel to pull dependencies using pip, that means bazel will have to access the Internet, which is against Android / AOSP Gerrit's building mechanisms.

I'll leave the compatibility level question to you and the Bazel folks, since I also don't know how to solve the Python 3.6 problem you mention above :)

@metti
Copy link

metti commented Dec 13, 2023

Do you see harm in sending a pull request with the more conservative compatibility level 2 and possibly changing this to 1 later? (For me personally, the python minimum requirement would warrant level 2 as it could actively break existing users due to a change within absl-py introduced in a major version).

@david-crouse
Copy link

I'm inclined to agree in making it 2 as that can prevent downstream "gotchas" and encourages folks to think about whether the upgrade will break things.

@mering
Copy link

mering commented Feb 22, 2024

Python 3.6 (and also 3.7) are out of support. Bumping the compatibility level for every Python version change doesn't seem appropriate (imagine a lot of projects just drop unsupported versions, so they would at least once per year have to bump their compatibility level).

@david-crouse
Copy link

david-crouse commented Feb 23, 2024

Are there any other issues with backwards compat from 1.x.x to 2.x.x? I'm OK with setting a new compatibility level to match the major version number. That's not as world-breaking as changing it every time we drop a Python version and seems to make logical sense.

@yilei
Copy link
Contributor

yilei commented Feb 26, 2024

  1. Considering a Python 3.6 user: if they have a dependency that requires abseil-py to be 2.0+, then regardless of the compatibility_level set here, they are already in a bad state.
  2. For the rest of Python versions, the compatibility_level should really be 1 as it shouldn't require upgrading to abseil-py to 2.0.

Based on above reasoning, I think we should set compatibility_level = 1.

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

No branches or pull requests

6 participants