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

Enable allow-prereleases on all Python installs. #172

Merged
merged 11 commits into from
Oct 16, 2024
Merged

Conversation

freakboy3742
Copy link
Member

Enable the use of allow-prereleases when setting up Python.

This allows us to use a bare version number for pre-release Python versions, rather than needing to add (and often remove at runtime) the -dev suffix. Given we're about to do a big push of adding 3.14, and removing the -dev suffix from 3.13, it seemed opportune to get this in place.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Copy link
Member

@rmartin16 rmartin16 left a comment

Choose a reason for hiding this comment

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

👍🏼

.github/workflows/ci.yml Show resolved Hide resolved
Copy link
Member

@rmartin16 rmartin16 left a comment

Choose a reason for hiding this comment

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

I was initially confused reading some of the logic...but then I realized this is taking a bit more liberal view of "system" than what I was imagining.

When I think about "system Python", I am referring to the Python that is default and native to the environment. For Linux, this Python is governed by the distributors...but for macOS and Windows, this Python does not exist (or at least not meaningfully). Therefore, if we expand "system Python" to include macOS and Windows in CI, we're really just saying "whatever GitHub ships in the runner" is the "system Python". So, testing against a "system Python" on these platforms doesn't have any analogue in reality.

As another line of reasoning, this also creates a very different definition of "system" for non-Linux platforms. For Linux, we literally want to use the Python installed by the system. Elsewhere, though, we're saying we want a system Python...but really just install a version of GitHub's Python for us to use. Furthermore, we're depending on GitHub to not let this "system Python" be an EOL version at any point.

Philosophy aside...this implementation does have the advantage of simplifying the use of app-build-verify and app-create-verify insofar as allowing callers to just pass system for python-version regardless of the runner OS. If we take my imagined implementation, callers would need to conditionally set python-version to system only when testing native Linux packaging.

For instance, if we consider how Briefcase would need to use app-build-verify:

verify-apps:
  name: Build app
  needs: [ package, package-automation, unit-tests ]
  uses: beeware/.github/.github/workflows/app-build-verify.yml@main
  with:
    python-version: ${{ matrix.python-version }}
    runner-os: ${{ matrix.runner-os }}
    framework: ${{ matrix.framework }}
  strategy:
    fail-fast: false
    matrix:
      framework: [ "toga", "pyside6", "pygame", "console" ]
      runner-os: [ "macos-12", "macos-14", "ubuntu-latest", "windows-latest" ]
      include:
        - runner-os: "ubuntu-latest"
          python-version: "system"

So, long-winded as usual...but personally, I think I lean towards a more limited definition of "system" where we're literally just talking about /usr/bin/python3. Therefore, the value of system for python-version would only mean "do not run actions/setup-python"....and this is really only appropriate for the Ubuntu runners. Thoughts?

@freakboy3742
Copy link
Member Author

freakboy3742 commented Oct 13, 2024

So, long-winded as usual...but personally, I think I lean towards a more limited definition of "system" where we're literally just talking about /usr/bin/python3. Therefore, the value of system for python-version would only mean "do not run actions/setup-python"....and this is really only appropriate for the Ubuntu runners. Thoughts?

That's where I started; the problem is that it fall apart on matrix builds because you can't pass "system" into a macOS/Windows build in a matrix.

I guess the real question here is whether it makes more sense to raise an error, or fall back to a "recent version". I've used the fallback, using the python3 that is available in the CI platform; but there's an argument to be made that "3.x" might be a more predictable interpretation. I agree that it's a little weird in the intepretation of "system"; but it's a strict addition of behavior ("system" on macOS/Windows ~= "3.x"), and that interpretation doesn't not make sense given there's no other meaningful interpretation of system (unless you want to get into "Xcode ships with 3.9" and "Windows ships with a store shim that will install 3.x"... but a strict "3.x" makes more sense to me)

Having a fallback doesn't preclude the sort of usage you've described, either. You could still define a default python version of "3.12", and override the Python to "system" for a specific Linux build. Your specific example workflow needs one more detail, as the macOS/Windows cases won't define a value for python-version, but that's easy to add with either a default includes: value, or a ``${{ matrix.python-version || '3.12' }}` when defining the python version.

The other detail worth flagging; app-create-verify does need to install Python, even on Linux, because otherwise it tries to install dependencies into the system python, falls back to a user install and fails. Open to suggestions to any fixes or workarounds on this one; but that's also the pre-existing behavior.

So - do we keep the "working fallback", or go back to "raise an error if you use 'system' on macOS/Windows", and modify the handful of matrix test cases to avoid the error?

@rmartin16
Copy link
Member

So - do we keep the "working fallback", or go back to "raise an error if you use 'system' on macOS/Windows", and modify the handful of matrix test cases to avoid the error?

I mostly just want to avoid any behavior where passing system for python-version has a valid meaning for macOS and Windows; so, I think that should be a failure mode.

Your specific example workflow needs one more detail, as the macOS/Windows cases won't define a value for python-version, but that's easy to add with either a default includes: value, or a ${{ matrix.python-version || '3.12' }} when defining the python version.

In my example, I was intending the choice of Python version be deferred to app-build-verify since it should default to 3.x in absence of a value.

The other detail worth flagging; app-create-verify does need to install Python, even on Linux, because otherwise it tries to install dependencies into the system python, falls back to a user install and fails. Open to suggestions to any fixes or workarounds on this one; but that's also the pre-existing behavior.

I think your solution is fine...but as with app-build-verify, I don't think system should have a meaning on macOS and Windows.

As an alternative, I suppose, we could expand a bit what we're trying to do. My initial thought was "let's embed this whole Python version pinning within the beeware/.github workflows" because the callers don't actually care. However, we could just take this a step further and use a value like resolve for python-version; in this way, the callers would then be explicitly deferring to the beeware/.github workflows to figure out the correct Python to run the workflow with.

@freakboy3742
Copy link
Member Author

So - do we keep the "working fallback", or go back to "raise an error if you use 'system' on macOS/Windows", and modify the handful of matrix test cases to avoid the error?

I mostly just want to avoid any behavior where passing system for python-version has a valid meaning for macOS and Windows; so, I think that should be a failure mode.

👍

I can live with that, especially considering...

Your specific example workflow needs one more detail, as the macOS/Windows cases won't define a value for python-version, but that's easy to add with either a default includes: value, or a ${{ matrix.python-version || '3.12' }} when defining the python version.

In my example, I was intending the choice of Python version be deferred to app-build-verify since it should default to 3.x in absence of a value.

... this. I forgot that the workflow had a fallback value at the input level.

The other detail worth flagging; app-create-verify does need to install Python, even on Linux, because otherwise it tries to install dependencies into the system python, falls back to a user install and fails. Open to suggestions to any fixes or workarounds on this one; but that's also the pre-existing behavior.

I think your solution is fine...but as with app-build-verify, I don't think system should have a meaning on macOS and Windows.

I've modified the implementation to take this approach.

As an alternative, I suppose, we could expand a bit what we're trying to do. My initial thought was "let's embed this whole Python version pinning within the beeware/.github workflows" because the callers don't actually care. However, we could just take this a step further and use a value like resolve for python-version; in this way, the callers would then be explicitly deferring to the beeware/.github workflows to figure out the correct Python to run the workflow with.

I think I'd rather stick to the side of "explicit is better than implicit" - I'm not sure we reliably have enough context on the .github side of things to pick the "right" Python version reliably, unless we just fall back to "3.x"... which will be the default value if unspecified anyway. All we'd be encoding is an explicit value which is the implicit value of 3.x, except for linux, which is "system", which I don't think is enough benefit to be worth it.

Copy link
Member

@rmartin16 rmartin16 left a comment

Choose a reason for hiding this comment

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

Some final details to work out.

.github/workflows/app-build-verify.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/app-build-verify.yml Show resolved Hide resolved
.github/workflows/app-build-verify.yml Show resolved Hide resolved
uses: actions/[email protected]
with:
python-version: ${{ inputs.python-version }}
python-version: ${{ inputs.python-version || '3.x' }}
Copy link
Member Author

Choose a reason for hiding this comment

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

By way of explanation: It appears "with: ''" is passed through as a literal empty string, rather than falling back to a default, so we need to provide the default as a fallback here, as well (same for the create-verify analog).

Copy link
Member

@rmartin16 rmartin16 left a comment

Choose a reason for hiding this comment

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

Looks good. Feel free to add me to the downstream PRs.

@freakboy3742
Copy link
Member Author

Looks good. Feel free to add me to the downstream PRs.

Will do... however, I don't think there's as many as I originally thought. Briefcase and the linux system template, obviously... but the other Linux templates don't need it... what else am I missing?

@rmartin16
Copy link
Member

I think the explicit use of ubuntu-22.04 should be a good signal:

~/github/beeware took 6s 
❯ grep -rF ubuntu-22.04 | grep 'yml\|yaml'
briefcase/.readthedocs.yaml:  os: ubuntu-22.04
briefcase/.github/workflows/ci.yml:        runner-os: [ "macos-12", "macos-14", "ubuntu-22.04", "windows-latest" ]
briefcase/.github/workflows/ci.yml:      # Ubuntu version used to run Linux tests. We use a fixed ubuntu-22.04
briefcase/.github/workflows/ci.yml:        runner-os: [ "macos-12", "macos-14", "ubuntu-22.04", "windows-latest" ]

toga-chart/.readthedocs.yaml:  os: ubuntu-22.04

toga/.readthedocs.yaml:  os: ubuntu-22.04
toga/.github/workflows/ci.yml:          runs-on: "ubuntu-22.04"
toga/.github/workflows/ci.yml:          runs-on: "ubuntu-22.04"
toga/.github/workflows/ci.yml:          runs-on: "ubuntu-22.04"

.github/.github/workflows/ci.yml:        runner-os: [ macos-latest, windows-latest, ubuntu-22.04 ]
.github/.github/workflows/ci.yml:        runner-os: [ macos-latest, windows-latest, ubuntu-22.04 ]
.github/.github/workflows/ci.yml:      runner-os: ubuntu-22.04

rubicon-objc/.readthedocs.yml:  os: ubuntu-22.04

beeware/.readthedocs.yaml:  os: ubuntu-22.04

briefcase-template/.github/workflows/ci.yml:        runner-os: [ "macos-latest", "ubuntu-22.04", "windows-latest" ]
briefcase-template/.github/workflows/ci.yml:      # ubuntu-22.04, which is needed to test local Debian packages. We use
briefcase-template/.github/workflows/ci.yml:      # ubuntu-22.04 explicitly rather than ubuntu-latest because when
briefcase-template/.github/workflows/ci.yml:        runner-os: [ "macos-latest", "ubuntu-22.04", "windows-latest" ]

So, looks like just briefcase-template.

@freakboy3742
Copy link
Member Author

So, looks like just briefcase-template.

Good catch - I've pushed a PR for that one as well.

The others all look like either explicit RTD configs, explicit configs in this repo, PR's I've already submitted, or Toga configs that need to be explicitly 22.04.

@freakboy3742 freakboy3742 merged commit e6da492 into main Oct 16, 2024
71 checks passed
@freakboy3742 freakboy3742 deleted the auto-dev-python branch October 16, 2024 01:08
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

Successfully merging this pull request may close these issues.

2 participants