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

feat(action): do not depend on pipx in Nox action #768

Merged
merged 7 commits into from
Mar 29, 2024

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented Feb 17, 2024

pipx is being re-added in actions/runner-images#9474

blocked by actions/runner-images#9256 (temp: don't use pipx commit)
blocked by actions/setup-python#808 (temp: python 3.8 & 3.9 missing on macos-14 commit)

@henryiii
Copy link
Collaborator

Do we know if pipx is going to be re-added? We could use this mechanism (both here and in cibuildwheel) until it is. And I wouldn't be shocked if 3.8 was never added to GHA macOS AS, Python didn't officially support AS at that point, it was very experimental. The 3.8 that ships on (at least previous versions of) macOS AS was an Apple patched fork. I'm not sure why 3.9 is missing though, as that had pretty solid support for AS.

@henryiii
Copy link
Collaborator

The matching PR was merged and released in cibuildwheel. @mayeut can we take this out of draft?

@mayeut mayeut marked this pull request as ready for review March 16, 2024 08:17
@mayeut
Copy link
Contributor Author

mayeut commented Mar 16, 2024

I marked this PR as ready for review as requested by @henryiii

pipx is going to be re-added however this PR also deals with self-hosted runners on which pipx might not be available.
As for python 3.8/3.9, there's no timeline from upstream with issue/PR for a couple months.

Copy link
Collaborator

@cjolowicz cjolowicz left a comment

Choose a reason for hiding this comment

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

LGTM. Can we change the title to something like "do not depend on pipx in Nox action"?

@mayeut mayeut changed the title chore(ci): test action on macos-14 feat(action): do not depend on pipx in Nox action Mar 17, 2024
@henryiii
Copy link
Collaborator

AFAICT pipx is just awaiting deployment. I'm not nearly as worried about not using pipx if it's available on all the official runners - a self hosted runner could just add pipx. Maybe we should just add the tests when pipx becomes available? What do others think? I'm fine either way.

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated
with open(os.environ["GITHUB_PATH"], "at") as f:
f.write(f"{nox_bin_path}\n")
print("::endgroup::")
shell: "${{ steps.localpython.outputs.python-path }} -u {0}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
shell: "${{ steps.localpython.outputs.python-path }} -u {0}"
shell: "${{ steps.allpython.outputs.python-path }} -u {0}"

Not sure using the last activated one makes sense, but it’s what was done before. Seems like only activating 3.6 would be valid, but we must run from 3.7+, for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@henryiii, the last activated one is always 3.11:

# we want to install python 3.11 last to ease nox set-up
if "3.11" in cpython_versions_filtered:
index = cpython_versions_filtered.index("3.11")
index = versions.index(cpython_versions[index])
cpython_nox = versions.pop(index)
versions.append(cpython_nox)
else:
# add this to install nox
versions.append("3.11")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn’t we use localpython?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

at some point, and that's probably still the case, setup-python was allowed to delete some python install to install a more specific one: e.g. you have the latest 3.11.y pre-installed, but specify 3.11.x in the nox action then, 3.11.y is deleted and localpython won't work anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, makes sense. Should we bump to 3.12 now?

@henryiii henryiii merged commit efc17bb into wntrblm:main Mar 29, 2024
31 checks passed
@mayeut mayeut deleted the macos-14 branch March 30, 2024 09:07
@henryiii henryiii mentioned this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants