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 PBS Python provider #19462

Closed
wants to merge 3 commits into from
Closed

Add PBS Python provider #19462

wants to merge 3 commits into from

Conversation

thejcannon
Copy link
Member

This change adds Python Build Standalone (aka PBS) as another "Python Provider". Additionally, a script is provided that scrapes the GitHub API for release information.

@thejcannon
Copy link
Member Author

Marked as draft, since #19379 adds the PyGithub requirement to the repo, and I'm too lazy to also add it here 😄

Otherwise everything is ready for showtime.

@thejcannon thejcannon requested review from stuhood, benjyw and huonw July 14, 2023 18:03
@thejcannon
Copy link
Member Author

Also note there's significant overlap between this and the Pyenv implementation. After this PR I plan on shifting some of the duplicated code to the generic code to allow the various implementations to just define whats necessary.

pass


def _choose_python(
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 think the pyenv backend has a bug where it doesn't respect patch version constraints. This code likely could be shared.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR with fix for the pyenv backend #21139

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

Putting this here might make include-listing for #19458 more fiddly. Thoughts on putting this in (some subdirectory of) pants_release?

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 really like the locality, but also I sympathize with the visibility pain.
We do something similar for terraform here so standardizing (and maybe wiring up to update-lockfiles?) would be a good idea.

Comment on lines 19 to 26
def _github():
token = os.environ.get("GH_TOKEN")
if not token:
token = subprocess.run(
["gh", "auth", "token"], check=True, text=True, capture_output=True
).stdout.strip()

return github.Github(auth=github.Auth.Token(token))
Copy link
Contributor

Choose a reason for hiding this comment

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

Pending #19379 merging and moving this script to pants_release (or something), this could be shared with the existing code there.

In addition, based on #19379 (comment), if we keep the reimplementation here, the subprocess.run can happen unconditionally and simplify the code: gh auth token handles GH_TOKEN itself.

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'll update now that the other PR is merged.

src/python/pants/backend/python/providers/pbs/rules.py Outdated Show resolved Hide resolved
src/python/pants/backend/python/providers/pbs/rules.py Outdated Show resolved Hide resolved
@cburroughs
Copy link
Contributor

For the specific case of in-repo plugins it would be really spiffy if this provider could be enabled conditionally. (per target? per python version?)

That would make the "hermetic python" stories a big easier to explain since it would not have a "except if you have plugins" asterisk. I know that's also scope-creepy and need not block merging.

jonasrauber pushed a commit to jonasrauber/pants that referenced this pull request Jul 7, 2024
huonw added a commit that referenced this pull request Jul 26, 2024
fixes #20175

fix inspired by #19462

---------

Co-authored-by: Jonas Rauber <jrauber@apple.com>
Co-authored-by: Huon Wilson <huon@exoflare.io>
cburroughs added a commit that referenced this pull request Sep 20, 2024
Add Python Build Standalone (aka PBS) as another "Python Provider".
Additionally, a script is provided that scrapes the GitHub API for
release information.

Original work (most of it!) in #19462 by @thejcannon

---------

Co-authored-by: Joshua <joshdcannon@gmail.com>
@cburroughs
Copy link
Contributor

I am gong to close this in favor of #21422 Thank you for doing almost all of the work!

@cburroughs cburroughs closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants