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

Honor PEP 621 requires-python setting. #2029

Merged

Conversation

sarayourfriend
Copy link
Contributor

@sarayourfriend sarayourfriend commented Oct 14, 2024

Fixes #2016

While I believe this changeset addresses the issue as described, I have some lingering questions about the approach I took. Namely, the Linux system command mixins already include some Python interpreter version validation to ensure the running interpreter is compatible with Briefcase, and I haven't found a good way to integrate that into this change. I wondered for a bit whether a basic definition of LinuxSystemMostlyPassiveMixin::verify_python should be hoisted to the BaseCommand, and then super() called from those Linux system commands instead of creating an entirely new method on the BaseCommand. I chose against trying to do that because it seemed the concerns were getting mixed a bit, and I wanted to see what the maintainers thought before making any bigger changes.

If I were going to fully integrate this new bit of interpreter version validation with the existing bits, I think I'd like to rename verify_python to verify_interpreter_briefcase_compatibility (or something along those lines). Then name the BaseCommand method just verify_python and call to verify_interpreter_briefcase_compatibility from an override of that base command method in the mixin.

Speaking generally, it would probably be a good idea to rename verify_python in that mixin anyway, to at least nominally clarify the difference between its responsibility and that of the base command's own Python interpreter verification. But, like I said, I wasn't too confident in any of the approaches I could come up with, and wanted to avoid creating a bigger changeset than absolutely necessary. That being said, I'm happy to keep working on this to create a more cohesive solution, just let me know 🙂.

Also: thank you for the excellent contributor documentation. It was easy to get started contributing to BeeWare! And a pleasant codebase to navigate around too.

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

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Initial quick review looks really good - will do a full review when CI is passing.

From the look of it, the issue is that config.py hasn't yet acquired the from __future__ import annotations that enables PEP 563 deferred annotation handling. This has been added to most files in Briefcase;l but for some reason, config.py hasn't been updated yet.

@sarayourfriend sarayourfriend force-pushed the add/requires-python-consideration branch from 9dd79d7 to 0982628 Compare October 14, 2024 09:21
@sarayourfriend
Copy link
Contributor Author

Aha! Thanks, I previously pushed a commit to just remove that annotation, but have replaced it with the future import 👍

@sarayourfriend sarayourfriend force-pushed the add/requires-python-consideration branch from 9846238 to aee137f Compare October 14, 2024 09:35
@sarayourfriend
Copy link
Contributor Author

Okay, seems like the CI is good aside from the problem fixed in #2028 🎉

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This looks really good! A couple of small suggestions inline; we'll also need to wait until #2028 lands to confirm we're good to merge.

Regarding your comments about the Linux version checks - those are slightly different, because they are quite often verifying the version of Python in a Docker container that Briefcase is managing, rather than Briefcase itself. To that end, they're a slightly different verification to what is being validated here.

However, I think your instinct about renaming those validations from verify_python to something else is sound; verify_interpreter_briefcase_compatibility is a little more verbose than I'd prefer. Something like verify_system_python would be a little more palatable.

src/briefcase/commands/base.py Outdated Show resolved Hide resolved
tests/commands/base/test_verify_requires_python.py Outdated Show resolved Hide resolved
tests/commands/base/test_verify_requires_python.py Outdated Show resolved Hide resolved
@freakboy3742
Copy link
Member

FYI - #2028 has now landed, so if you merge with main, CI should now pass.

Draws a clearer distinction between verifying the interpreter in the container, vs on the host (handled in BaseCommand::verify_required_python), and system Python verification in verify_system_python
@sarayourfriend
Copy link
Contributor Author

You may be partial to verify_system_python because it already exists 😅

def verify_system_python(self):

After looking at the usage of the existing verify_python method a bit, it seems to only be used for validating the Docker container's interpreter. The method's docstring claims this, and it is called only in this branch guarded by if self.use_docker:

if self.use_docker:
DockerAppContext.verify(
tools=self.tools,
app=app,
image_tag=self.docker_image_tag(app),
dockerfile_path=self.bundle_path(app) / "Dockerfile",
app_base_path=self.base_path,
host_bundle_path=self.bundle_path(app),
host_data_path=self.data_path,
python_version=app.python_version_tag,
extra_build_args=self.extra_docker_build_args,
)
# Check the system Python on the target system to see if it is
# compatible with Briefcase.
if verify_python:
self.verify_python(app)

I've renamed it to verify_docker_python, which matches the verbosity of the other methods, and (hopefully) accurately reflects the method's scope. Let me know what you think, happy to use any name you'd like or leave this change out of this PR altogether.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I've made a small tweak to the parameterized cases to (hopefully) make them a little more clear, and be a little less reliant on the specific Python version - but otherwise, this looks great - Thanks for the contribution!

As luck has it, you've wrapped this up just before I was about to push a release, so this feature should be in the wild in a couple of hours as part of Briefcase 0.3.20.

(On an unrelated note - I notice from your bio that you're (a) looking for work, and (b) based in naarm; if you're not already planning to be at PyCon AU 2024, that should be some companies looking to hire there - and, for bonus points, you'll be able to claim your BeeWare challenge coin, because I'll be there :-))

@sarayourfriend
Copy link
Contributor Author

(On an unrelated note - I notice from your bio that you're (a) looking for work, and (b) based in naarm; if you're not already planning to be at PyCon AU 2024, that should be some companies looking to hire there - and, for bonus points, you'll be able to claim your BeeWare challenge coin, because I'll be there :-))

Thanks! The timing of this contribution may or may not be related to my presence at PyCon AU next month and having seen you speak at KiwiPycon last month 🙂. I very much enjoyed your talk in Wellington, and suddenly have a lot of time on my hands to explore contributions to projects other than Openverse, so thought I'd take your advice from the Q&A at that talk and look at BeeWare.

I'll see you at PyCon, and around GitHub 😁

@freakboy3742 freakboy3742 merged commit 053a0db into beeware:main Oct 15, 2024
52 checks passed
@sarayourfriend sarayourfriend deleted the add/requires-python-consideration branch October 15, 2024 03:59
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.

Honor the PEP 621 requires-python setting
2 participants