Drop Python 3.8 support and bump macOS/iOS template epoch #1934
Conversation
| """Return classes that subclass Tool in a module in ``briefcase.integrations``, e.g. | ||
| {"android_sdk": AndroidSDK}.""" | ||
| return dict( | ||
| inspect.getmembers( | ||
| sys.modules[f"briefcase.integrations.{tool_module_name}"], | ||
| lambda klass: ( | ||
| inspect.isclass(klass) | ||
| and not inspect.isabstract(klass) | ||
| and issubclass(klass, (Tool, ManagedTool)) |
There was a problem hiding this comment.
This is the only substantive logic change required by the version upgrade.
With the move of Sequence from typing.Sequence to collections.abc.Sequence, SubprocessArgsT now returns True from inspect.isclass(), but raises an error if you try to use it in issubclass. Adding an additional check for inspect.isabstract() avoids the error, and also allows removing the explicit reference to Tool and ManagedTool classes.
|
Extracted from #1943 (comment):
Yeah; I've definitely encountered this - even as recently as #1929. The solution I've been thinking about is basically the same approach we resolved this in other similar situations. That is, when an app template repo needs to test against a specific version of Briefcase, you can specify So, we could just extend this pattern to the build format templates for any use of Maybe something like:
So, for example:
This would also probably be a good time to refactor all the regex logic on PR Body in to its own GitHub Action instead of continuing to duplicate it. (see beeware/.github#159) |
The change you're suggesting would address the issue of running CI for a PR on Briefcase prior to landing the PRs for the template - but I'm not sure that's actually a problem that is causing us the problem here. We've got a reasonably well established pattern for landing template changes - rely on CI in the template (pointing at a branch of Briefcase) to validate the template works; merge the template PR; then re-run CI on the Briefcase PR; then merge the Briefcase PR. I'm not sure we gain a lot by further complicating our CI tooling just so that we can have a clean CI run on the Briefcase side prior to merging the template. The problem in the case of this specific PR is that the branch redirection only applies on the PR - and so when merged to main, the template's merge CI run will fail. That's usually short lived because you can (almost) immediately merge the Briefcase PR, but in this case we can't because we need to retag the stub binary on the Xcode template before we can land the app template, which means the Xcode template's mainline needs to use a branch of Briefcase. I guess one approach in this case would be to tag the new binary on the PR branch. That breaks the usual convention of tagging stub binaries on the main branch... but I guess the tagged commit will become part of the mainline as soon as the template is merged to main, so maybe that doesn't matter. Even then, that wouldn't address the issue with the TODOs on the beeware/Python-Apple-support#191 - again, because the changes need to exist on the main branch (or, the 3.X branches in that case). So - while this change would fix a problem, I'm not sure it's a problem we actually have in practice, and the complication to the CI workflows is such that I'm not sure it's worth it. |
|
I see. Out of curiosity, is there another example of this happening? At least where the CI for Another thought is GitHub supports variables that can be configured outside of the CI workflows themselves. So, for these temporary periods where CI on |
Yes, it's rare - hence why we've been willing to live with some janky landing sequences on the rare occasion it happens. Other than template and epoch changes, the only example I can think of where it has come up recently is where a template adds a dependency on a new template filter. This currently requires adding (and landing) the filter in one PR, then updating the template to use that filter, then updating briefcase to do whatever change needed the new template. Again, we have a workaround; it might be nice to be able to temporarily tell the template to use a branch of Briefcase when running on main.
I hadn't thought of using GitHub environment variables for this. The UX for setting this up is a bit messy, but I guess it would work, and it wouldn't be that much more complicated than the current "regex over PR body" approach. We'd probably also want a safety catch to make sure that we accidentally release Briefcase with the environment variable set on a template. |
|
@mhsmith FYI - I had to revert the merge of the macOS Xcode template because having that template merged was breaking all the dependabot runs. When we do merge that template, we'll need to follow the remaining steps in rapid succession because until this PR is merged, any other PRs on Briefcase or a template will fail. |
|
OK, then I'll approve the new PRs but leave them unmerged. |
|
@mhsmith I've tagged and published the updated stub binaries on the That means the macOS-app template and this PR are ready for review. Assuming you're OK with both, this template PRs can be merged, the CI re-run on this PR, and then merge this PR. If you want to hold off on the merging, I'm happy to do that in the morning. |
Python 3.8 is EOL in October, so the PEP 730 patches for iOS support won't be back ported to that release. There's also a need for a template epoch change because of the change in binary format on both iOS and macOS.
PR Checklist: