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 the PEP 621 requires-python setting #2016

Closed
freakboy3742 opened this issue Oct 6, 2024 · 3 comments · Fixed by #2029
Closed

Honor the PEP 621 requires-python setting #2016

freakboy3742 opened this issue Oct 6, 2024 · 3 comments · Fixed by #2029
Labels
enhancement New features, or improvements to existing features. good first issue Is this your first time contributing? This could be a good place to start!

Comments

@freakboy3742
Copy link
Member

What is the problem or limitation you are having?

Although Briefcase is able to target a wide range of Python versions, the app being published may not be equally flexible. As an example, the Toga testbed requires the use of Python 3.10 as a result of the specific dependencies that have been pinned.

Describe the solution you'd like

Briefcase should honor the PEP 612 requires-python configuration item. If a project defines requires-python setting, and the currently active Python doesn't meet the requirement defined by that setting, Briefcase should raise an error.

Describe alternatives you've considered

Do nothing, and rely on project documentation to define acceptable deployment platforms.

Additional context

The BaseCommand.verify_app() method would be a reasonable place for this check.

@freakboy3742 freakboy3742 added enhancement New features, or improvements to existing features. good first issue Is this your first time contributing? This could be a good place to start! labels Oct 6, 2024
@sarayourfriend
Copy link
Contributor

sarayourfriend commented Oct 14, 2024

Hi @freakboy3742, I'd like to work on this issue. I hope to have a PR up for it later today, but if you see this before I manage to get that up, can you confirm that the best place for me to add the check would be in merge_pep621_config, in other words, when the PEP621 properties are parsed in the first place? If I've followed the code correctly, the config is parsed every time a Briefcase command runs, so that would prevent running any command with a Python version that does not satisfy requires-python (when defined).

The thing I'm somewhat unsure of is whether adding the check there mixes concerns. Perhaps merge_pep621_config should just place the requires-python setting into the global config and the check/exception should happen somewhere else (not sure where though)?

@freakboy3742
Copy link
Member Author

@sarayourfriend No - merge_pep621_config isn't the right place, essentially for the reason you've identified.

There's two parts to the process of reading a configuration. The first is reading the configuration file; this involves a series of merges so that all the various parts of a configuration are pulled together into a single "flat" configuration dictionary for each app, no matter how complex your app's configuration is. This is where the merge_pep621_config comes in - it pulls known PEP 621 configuration items into the app's configuration. This is doing the parsing, which will sometimes involve some checks that the values are reasonable (e.g, that you've actually provided a string for a value that needs to be a a string); but it doesn't do anything to verify that the app configuration itself makes sense in the context of the app.

Then, when you run a command, there's the verify_app(app) method. This is defined on the base command; but other commands extend this implementation to do other checks, once more context about what has been requested is available. That is where any validations should be performed.

There might be a need to modify the merge_pep621_config implementation to ensure that the requires-python option is parsed and merged into the app configuration, but it shouldn't be doing anything other than ensuring the key is in the configuration. The actual validation should be done in the verify_app() method.

@sarayourfriend
Copy link
Contributor

Brilliant, I hoped something like verify_app existed, glad I asked! 🙂

I'll have a PR up shortly, and will ask any further questions or clarification there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features. good first issue Is this your first time contributing? This could be a good place to start!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants