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

Pull rules_pycross into rules_python #1360

Closed
4 tasks
rickeylev opened this issue Aug 3, 2023 · 6 comments
Closed
4 tasks

Pull rules_pycross into rules_python #1360

rickeylev opened this issue Aug 3, 2023 · 6 comments

Comments

@rickeylev
Copy link
Collaborator

rickeylev commented Aug 3, 2023

A recurring source of issues in rules_python is the underlying support for how wheels are handled, mainly that support for multiple platforms and versions doesn't work very well because a lot of activity happens at repo-rule time. Several of us have been looking at different parts of this problem, and we've generally come to the conclusion that a more idealized implementation is more akin to having simple http_file download's of the pypi artifacts, and moving any subsequent logic out of repo-rule time and into the regular build phase. This is a pretty big change in how things are done.

Luckily for us, @jvolkman's rules_pycross has done much of this already, and he's said its OK for us to take that code.

Per Google's rules, there are two ways we can do this:

  1. jvolkman@ signs the Google CLA (done already, can check internal signcla page for jvolkman) and then sends a pull request contributing his code. This is treated like any other pull request, and rules_python then owns the code.
  2. Copy the rules_pycross code in a top-level third_party directory. Any existing LICENSE and copyright headers must be preserved. If we edit a file, we must modify the copyright header to include Google LLC.

(1) is preferred because it's just less overhead in the long run.

I suggest we ask Jeremy to simply do a one-time code-dump it into //python/private/pycross_staging, then we can start picking it apart as necessary. This avoids the extra overhead of license and copyright management from (2), and having to ask Jeremy to send a PR for some new piece of pycross we want to reuse.

After its ingested, we can rename things to drop the "pycross" prefixes, figure out appropriate APIs we wish to expose, how to integrate with our existing code, etc

  • jvolkman@ sends PR contributing code
  • Rename to remove "pycross" prefixes
  • Figure out public API surface
  • Integrate with existing workspace/bzlmod build styles
@chrislovecnm chrislovecnm mentioned this issue Aug 5, 2023
10 tasks
@chrislovecnm
Copy link
Collaborator

Who is working on this?

@groodt
Copy link
Collaborator

groodt commented Aug 6, 2023

Option 1 LGTM if LGTY @jvolkman

@jvolkman
Copy link
Contributor

jvolkman commented Aug 6, 2023

I assume it's me working on this, but we're camping this week so I won't be able to get to it until after.

philsc added a commit to philsc/rules_python that referenced this issue Sep 4, 2023
This patch imports a few files from jvolkman/rules_pycross at
757033ff8afeb5f7090b1320759f6f03d9c4615c.

I would like to re-use this rule for the `pypi_install` repo rule that
I'm working on. This rule extracts a downloaded wheel and generates an
appropriate `PyInfo` provider for it.

All the non-BUILD files are taken as-is without modification. A
followup patch will make tweaks so that the code can be used from
within rules_python.

References: bazelbuild#1360
philsc added a commit to philsc/rules_python that referenced this issue Sep 8, 2023
Since I couldn't figure out how to disable buildifier from third_party
code, I let buildifier do its thing. This necessitated adding the
copyright headers as per bazelbuild#1360.
github-merge-queue bot pushed a commit that referenced this issue Sep 8, 2023
This patch imports a few files from jvolkman/rules_pycross at
757033ff8afeb5f7090b1320759f6f03d9c4615c.

I would like to re-use this rule for the `pypi_install` repo rule that
I'm working on. This rule extracts a downloaded wheel and generates an
appropriate `PyInfo` provider for it.

All the .pyfiles are taken as-is without modification. I had to run
buildifier
on all the bazel-related files. As per #1360,
that
meant that I had to add copyright headers.

A followup patch will make tweaks so that the code can be used from
within rules_python.

References: #1360
philsc added a commit to philsc/rules_python that referenced this issue Sep 13, 2023
This patch changes the name of the rule to reflect the fact that it's
not exactly the same as the one in rules_pycross.

I also took this opportunity to delete the superfluous
`namespace_pkgs.py` library (plus test) since we have a nearly
identical version already in the main repo.

I added a test to validate that the rule functions at a basic level.

References: bazelbuild#1360
philsc added a commit to philsc/rules_python that referenced this issue Sep 13, 2023
This patch changes the name of the rule to reflect the fact that it's
not exactly the same as the one in rules_pycross.

I also took this opportunity to delete the superfluous
`namespace_pkgs.py` library (plus test) since we have a nearly
identical version already in the main repo.

I added a test to validate that the rule functions at a basic level.

References: bazelbuild#1360
github-merge-queue bot pushed a commit that referenced this issue Sep 27, 2023
This patch changes the name of the rule to reflect the fact that it's
not exactly the same as the one in rules_pycross.

I also took this opportunity to delete the superfluous
`namespace_pkgs.py` library (plus test) since we have a nearly
identical version already in the main repo.

I added a test to validate that the rule functions at a basic level.

References: #1360
philsc added a commit to philsc/rules_python that referenced this issue Sep 28, 2023
This patch adds a few arguments to `py_wheel_library` to simulate how
`http_archive` accepts patch-related arguments.

I also amended the existing test to validate the behaviour at a very
high level.

References: bazelbuild#1360
github-merge-queue bot pushed a commit that referenced this issue Dec 19, 2023
This patch adds a few arguments to `py_wheel_library` to simulate how
`http_archive` accepts patch-related arguments.

I also amended the existing test to validate the behaviour at a very
high level.

References: #1360
Copy link

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

@github-actions github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Feb 18, 2024
@martis42
Copy link
Contributor

@rickeylev

Since the robots are gonna close this soonish, are we still aiming to do this?
While I can't contribute on this, I would be interested in this being available in rules_python eventually.

@github-actions github-actions bot removed the Can Close? Will close in 30 days if there is no new activity label Feb 25, 2024
ewianda added a commit to ewianda/rules_python that referenced this issue Jun 20, 2024
ewianda added a commit to ewianda/rules_python that referenced this issue Jun 20, 2024
@groodt
Copy link
Collaborator

groodt commented Jun 21, 2024

Code was vendored here https://github.com/bazelbuild/rules_python/tree/main/third_party/rules_pycross

Closing this as the code is ported which completes the main motivators here. We have yet to really use or take advantage of the code. Plans have changed as well so perhaps we won't end up using the code after all.

@groodt groodt closed this as completed Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants