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

use explicit setup-python versions over implicit expose- #21568

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

cburroughs
Copy link
Contributor

The project is currently using a mix of the GitHub supplied setup-python action and pantsbuild/actions/expose-pythons. On GitHub managed runners they behave similarly:

  • setup-python will install a version of Python if missing, or add it to the PATH if already present at the expected location (managed runners historically have multiple Pythons baked into the image)
  • expose-pythons will add all Python's at the expected GitHub location to the PATH. (Which ones is up to GitHub.)

So today the invocation of setup-python followed by expose-pythons is redundant.

Consolidating on setup-python let's us be explicit about expected versions (more like the ARM image) and reduces the number of custom actions the project manages while still making multiple Python versions available.

NOTE: The awkward double newlines in the final yaml are a pre-existing issue
https://stackoverflow.com/questions/45004464/yaml-dump-adding-unwanted-newlines-in-multiline-strings

See #21552 for some history regarding the setup- vs expose- actions.

@cburroughs cburroughs added category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required PR doesn't require mention in release notes labels Oct 23, 2024
The project is currently using a mix of the GitHub supplied
`setup-python` action and `pantsbuild/actions/expose-pythons`.  On
GitHub managed runners they behave similarly:
 * `setup-python` will install a version of Python if missing, or add
 it to the PATH if already present at the expected location (managed
 runners historically have multiple Pythons baked into the image)
 * `expose-pythons` will add all Python's at the expected GitHub
 location to the PATH.  (Which ones is up to GitHub.)

So today the invocation of `setup-python` followed by `expose-pythons`
is redundant.

Consolidating on `setup-python` let's us be explicit about expected
versions (more like the ARM image) and reduces the number of custom
actions the project manages while still making multiple Python
versions available.

NOTE: The awkward double newlines in the final yaml are a pre-existing
issue
https://stackoverflow.com/questions/45004464/yaml-dump-adding-unwanted-newlines-in-multiline-strings

See pantsbuild#21552 for some history regarding the `setup-` vs `expose-` actions.
@cburroughs cburroughs marked this pull request as ready for review October 23, 2024 19:18
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Seems very sensible to be explicit about which Python versions are desirable!

@cburroughs cburroughs merged commit e18484d into pantsbuild:main Oct 25, 2024
25 checks passed
huonw added a commit that referenced this pull request Oct 29, 2024
This fixes the "Build wheels (macOS12-x86_64)" CI job, by installing
Python. It was previously failing with errors like:

```
> Run ./pants run src/python/pants_release/release.py -- build-wheels
pants: Failed to find a Python 3.9 interpreter
```

I think this was a small oversight in #21568 (in particular
https://github.com/pantsbuild/pants/pull/21568/files#diff-b980287839311482e79f8adac1fdd7768274cf1e677a1103d9444185e4aabbd1L890),
which wasn't caught by CI because the build-wheels jobs were skipped.
@huonw huonw added this to the 2.22.x milestone Nov 19, 2024
@huonw
Copy link
Contributor

huonw commented Nov 19, 2024

Marked for cherrypicking to unblock #21663 and #21664

@WorkerPants
Copy link
Member

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

❌ 2.22.x

I was unable to cherry-pick this PR to 2.22.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

  1. (Ensure your git working directory is clean)
  2. Run the following script to reproduce the merge-conflicts:
    git fetch https://github.com/pantsbuild/pants main \
      && git fetch https://github.com/pantsbuild/pants 2.22.x \
      && git checkout -b cherry-pick-21568-to-2.22.x FETCH_HEAD \
      && git cherry-pick e18484d94ae835ddd7407bfba67c04c472785fb4
  3. Fix the merge conflicts and commit the changes
  4. Run build-support/cherry_pick/make_pr.sh "21568" "2.22.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

❌ 2.23.x

I was unable to cherry-pick this PR to 2.23.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

  1. (Ensure your git working directory is clean)
  2. Run the following script to reproduce the merge-conflicts:
    git fetch https://github.com/pantsbuild/pants main \
      && git fetch https://github.com/pantsbuild/pants 2.23.x \
      && git checkout -b cherry-pick-21568-to-2.23.x FETCH_HEAD \
      && git cherry-pick e18484d94ae835ddd7407bfba67c04c472785fb4
  3. Fix the merge conflicts and commit the changes
  4. Run build-support/cherry_pick/make_pr.sh "21568" "2.23.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

❌ 2.24.x

I was unable to cherry-pick this PR to 2.24.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

  1. (Ensure your git working directory is clean)
  2. Run the following script to reproduce the merge-conflicts:
    git fetch https://github.com/pantsbuild/pants main \
      && git fetch https://github.com/pantsbuild/pants 2.24.x \
      && git checkout -b cherry-pick-21568-to-2.24.x FETCH_HEAD \
      && git cherry-pick e18484d94ae835ddd7407bfba67c04c472785fb4
  3. Fix the merge conflicts and commit the changes
  4. Run build-support/cherry_pick/make_pr.sh "21568" "2.24.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.


When you're done manually cherry-picking, please remove the needs-cherrypick label on this PR.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

@WorkerPants WorkerPants added the auto-cherry-picking-failed Auto Cherry-Picking Failed label Nov 19, 2024
huonw pushed a commit that referenced this pull request Nov 19, 2024
The project is currently using a mix of the GitHub supplied
`setup-python` action and `pantsbuild/actions/expose-pythons`. On GitHub
managed runners they behave similarly:
* `setup-python` will install a version of Python if missing, or add it
to the PATH if already present at the expected location (managed runners
historically have multiple Pythons baked into the image)
* `expose-pythons` will add all Python's at the expected GitHub location
to the PATH. (Which ones is up to GitHub.)

So today the invocation of `setup-python` followed by `expose-pythons`
is redundant.

Consolidating on `setup-python` let's us be explicit about expected
versions (more like the ARM image) and reduces the number of custom
actions the project manages while still making multiple Python versions
available.

NOTE: The awkward double newlines in the final yaml are a pre-existing
issue

https://stackoverflow.com/questions/45004464/yaml-dump-adding-unwanted-newlines-in-multiline-strings

See #21552 for some history regarding the `setup-` vs `expose-` actions.
huonw added a commit that referenced this pull request Nov 19, 2024
This fixes the "Build wheels (macOS12-x86_64)" CI job, by installing
Python. It was previously failing with errors like:

```
> Run ./pants run src/python/pants_release/release.py -- build-wheels
pants: Failed to find a Python 3.9 interpreter
```

I think this was a small oversight in #21568 (in particular
https://github.com/pantsbuild/pants/pull/21568/files#diff-b980287839311482e79f8adac1fdd7768274cf1e677a1103d9444185e4aabbd1L890),
which wasn't caught by CI because the build-wheels jobs were skipped.
huonw pushed a commit that referenced this pull request Nov 19, 2024
The project is currently using a mix of the GitHub supplied
`setup-python` action and `pantsbuild/actions/expose-pythons`. On GitHub
managed runners they behave similarly:
* `setup-python` will install a version of Python if missing, or add it
to the PATH if already present at the expected location (managed runners
historically have multiple Pythons baked into the image)
* `expose-pythons` will add all Python's at the expected GitHub location
to the PATH. (Which ones is up to GitHub.)

So today the invocation of `setup-python` followed by `expose-pythons`
is redundant.

Consolidating on `setup-python` let's us be explicit about expected
versions (more like the ARM image) and reduces the number of custom
actions the project manages while still making multiple Python versions
available.

NOTE: The awkward double newlines in the final yaml are a pre-existing
issue

https://stackoverflow.com/questions/45004464/yaml-dump-adding-unwanted-newlines-in-multiline-strings

See #21552 for some history regarding the `setup-` vs `expose-` actions.
huonw added a commit that referenced this pull request Nov 19, 2024
This fixes the "Build wheels (macOS12-x86_64)" CI job, by installing
Python. It was previously failing with errors like:

```
> Run ./pants run src/python/pants_release/release.py -- build-wheels
pants: Failed to find a Python 3.9 interpreter
```

I think this was a small oversight in #21568 (in particular
https://github.com/pantsbuild/pants/pull/21568/files#diff-b980287839311482e79f8adac1fdd7768274cf1e677a1103d9444185e4aabbd1L890),
which wasn't caught by CI because the build-wheels jobs were skipped.
huonw added a commit that referenced this pull request Nov 20, 2024
… of #21568, #21582) (#21667)

The project is currently using a mix of the GitHub supplied
`setup-python` action and `pantsbuild/actions/expose-pythons`. On GitHub
managed runners they behave similarly:
* `setup-python` will install a version of Python if missing, or add it
to the PATH if already present at the expected location (managed runners
historically have multiple Pythons baked into the image)
* `expose-pythons` will add all Python's at the expected GitHub location
to the PATH. (Which ones is up to GitHub.)

So today the invocation of `setup-python` followed by `expose-pythons`
is redundant.

Consolidating on `setup-python` let's us be explicit about expected
versions (more like the ARM image) and reduces the number of custom
actions the project manages while still making multiple Python versions
available.

NOTE: The awkward double newlines in the final yaml are a pre-existing
issue

https://stackoverflow.com/questions/45004464/yaml-dump-adding-unwanted-newlines-in-multiline-strings

See #21552 for some history regarding the `setup-` vs `expose-` actions.

---------

Co-authored-by: cburroughs <chris.burroughs@gmail.com>
huonw added a commit that referenced this pull request Nov 21, 2024
… of #21568, #21582) (#21668)

The project is currently using a mix of the GitHub supplied
`setup-python` action and `pantsbuild/actions/expose-pythons`. On GitHub
managed runners they behave similarly:
* `setup-python` will install a version of Python if missing, or add it
to the PATH if already present at the expected location (managed runners
historically have multiple Pythons baked into the image)
* `expose-pythons` will add all Python's at the expected GitHub location
to the PATH. (Which ones is up to GitHub.)

So today the invocation of `setup-python` followed by `expose-pythons`
is redundant.

Consolidating on `setup-python` let's us be explicit about expected
versions (more like the ARM image) and reduces the number of custom
actions the project manages while still making multiple Python versions
available.

NOTE: The awkward double newlines in the final yaml are a pre-existing
issue

https://stackoverflow.com/questions/45004464/yaml-dump-adding-unwanted-newlines-in-multiline-strings

See #21552 for some history regarding the `setup-` vs `expose-` actions.

---------

Co-authored-by: cburroughs <chris.burroughs@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-cherry-picking-failed Auto Cherry-Picking Failed category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required PR doesn't require mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants