-
-
Notifications
You must be signed in to change notification settings - Fork 473
Initial pep639 support #2307
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
base: main
Are you sure you want to change the base?
Initial pep639 support #2307
Conversation
Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>
Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>
Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good compromise approach - it's not a full migration to PEP 639, but it's enough of a port that we will be able to stop raising errors for a valid PEP 639 configuration, and doesn't box us into a corner for a full PEP 639 conversion.
I've flagged a couple of minor areas for cleanup inline.
src/briefcase/config.py
Outdated
| ) | ||
| if cwd is None: | ||
| cwd = Path() | ||
| all_globs = (cwd.glob(pattern) for pattern in config["license-files"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An edge case here: license-files = "LICENCE" is invalid PEP 639 syntax, but it fails here in an unexpected way - it looks for a glob match on each letter in the word LICENSE. It would probably be worth adding a if isinstance(config["license-files"], str) check.
src/briefcase/config.py
Outdated
| return {"file": str(license_file)} | ||
|
|
||
|
|
||
| def merge_pep621_config(global_config, pep621_config, console, *, cwd=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that we need to have cwd to be able to process pep639 configurations; but I'm wondering if it would be better to treat them as 2 separate steps, and keep the prototype of merge_pep621_config simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have moved the PEP639 parsing out of merge_pep621_config (which might make sense since PEP639 isn't in fact PEP621...), is that more in line with what you imagined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially, yes; but I think we can take this a bit further and clean it up even more.
As I see it, ignoring license processing for the moment, the config parsing comes in 3 phases:
- The
tool.briefcasesection is read as the base "global" config - The
projectblock is then merged in, yielding a complete "global" config - Each app configuration is then read, formed from format config, merged over the platform config, merged over the app config, merged over the global config.
Ideally, (1) and (2) would be the other way around; (2) would be the true "global" config, and the tool.briefcase section would be merged over the top of it. The existing ordering exists because of the differences in PEP621 key names; if/when we ever complete migrating to completely PEP621 naming and formats, we can likely reverse this.
However, regardless of the approach, license isn't a renamed key - we're using (or trying to use) the PEP621 license definition. In introducing PEP639 support, the same thing is true - we're trying to use the official key everywhere.
The edge case that made me think of this is that, in theory, you could define a license details at each level - project, tool.briefcase, tool.briefcase.app.helloworld, tool.briefcase.app.helloworld.macOS, ... - and all those configurations, especially the merged app-level ones, could clash. In addition, license-files should be a cumulative setting.
So - it seems to me that we should be treating license-files as a normal cumulative setting (like requires), and then post processing the validation/conversion into license.file format after all merging has been done. The post-validation loop is the same as the one that is currently being used to report the deprecation of license - but should be done after the settings merge (arguably the deprecation check should have been in the same place, for the same reason)
In the future, when we have full PEP639 support, we replace the "transform to license.file" with "transform into a list of actual file names".
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense. We made an attempt at it this weekend at least
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>
Without this change, the code will work even if the license-files field is just a single string. However, in that case, the string will be iterated over and the code will look for a glob match for each character independently. This fix makes it explicitly fail in that circumstance, as the PEP639 standard specifies that license-files is an array of strings. Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor cleanups, plus the bigger design issue flagged in a comment reply.
| Found license-files in pyproject.toml, but it is a string, while it should be a list. | ||
| Did you mean | ||
| license-files = ['{config["license-files"]}'] | ||
| instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Found license-files in pyproject.toml, but it is a string, while it should be a list. | |
| Did you mean | |
| license-files = ['{config["license-files"]}'] | |
| instead? | |
| Found license-files in pyproject.toml, but it is defined as a string, not a list. | |
| Did you mean: | |
| license-files = [{config["license-files"]}'] | |
| instead? |
| license_file = next(all_licenses, None) | ||
| if license_file is None: | ||
| raise BriefcaseConfigError( | ||
| "No license matching the glob patterns in `project.license-files`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "No license matching the glob patterns in `project.license-files`" | |
| "The patterns specified by `project.license-files` do not match any files." |
src/briefcase/config.py
Outdated
| project_config = pyproject.get("project", {}) | ||
| if pep639_info := get_pep639_license_info(project_config, console=console, cwd=cwd): | ||
| pyproject["project"]["license"] = pep639_info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this block needed? The global config will be processed later (currently L668) as part of the iteration over all project + app targets - won't having it here as well result in double processing?
src/briefcase/config.py
Outdated
| for name, config in [("project", global_config)] + list(all_apps.items()): | ||
| if pep639_info := get_pep639_license_info(config, console, cwd=cwd): | ||
| config["license"] = pep639_info | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is at least partially a pre-existing problem - but should this validation block be at after the app/platform/format merge? At least in theory, a user could put license definitions at every level of the file, and we should be incorporating all that data into the final evaluation of whether the license definition is valid.
src/briefcase/config.py
Outdated
| return {"file": str(license_file)} | ||
|
|
||
|
|
||
| def merge_pep621_config(global_config, pep621_config, console, *, cwd=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially, yes; but I think we can take this a bit further and clean it up even more.
As I see it, ignoring license processing for the moment, the config parsing comes in 3 phases:
- The
tool.briefcasesection is read as the base "global" config - The
projectblock is then merged in, yielding a complete "global" config - Each app configuration is then read, formed from format config, merged over the platform config, merged over the app config, merged over the global config.
Ideally, (1) and (2) would be the other way around; (2) would be the true "global" config, and the tool.briefcase section would be merged over the top of it. The existing ordering exists because of the differences in PEP621 key names; if/when we ever complete migrating to completely PEP621 naming and formats, we can likely reverse this.
However, regardless of the approach, license isn't a renamed key - we're using (or trying to use) the PEP621 license definition. In introducing PEP639 support, the same thing is true - we're trying to use the official key everywhere.
The edge case that made me think of this is that, in theory, you could define a license details at each level - project, tool.briefcase, tool.briefcase.app.helloworld, tool.briefcase.app.helloworld.macOS, ... - and all those configurations, especially the merged app-level ones, could clash. In addition, license-files should be a cumulative setting.
So - it seems to me that we should be treating license-files as a normal cumulative setting (like requires), and then post processing the validation/conversion into license.file format after all merging has been done. The post-validation loop is the same as the one that is currently being used to report the deprecation of license - but should be done after the settings merge (arguably the deprecation check should have been in the same place, for the same reason)
In the future, when we have full PEP639 support, we replace the "transform to license.file" with "transform into a list of actual file names".
Does that make sense?
| "license": {"text": "BSD License"}, | ||
| "requires-python": ">=3.9", | ||
| }, | ||
| console=Mock(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these Mocks still required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! No, they are not
Remove unneccessary mocks and improve handling of license info for platform-specific configs. Co-authored-by: Marie Roald <roald.marie@gmail.com>
Co-authored-by: Marie Roald <roald.marie@gmail.com>
Co-authored-by: Marie Roald <roald.marie@gmail.com>
Co-authored-by: Marie Roald <roald.marie@gmail.com>
This PR (co-authored with @yngvem) adds initial support for PEP639-style license metadata (#2146). This means that the
briefcase createcommand will work with PEP639-style metadata (for users that follow the happy path of only one license file). However, full support for is still lacking. Specifically, this still need to be implemented:briefcase convert-commandCurrently, in this PR, support is implemented by converting PEP639-style metadata to the PEP621-style license metadata internally. We think this can make sense in an initial effort to add support, as it keeps backwards compatibility. However, when full PEP639-style metadata is implemented, it might make sense to "switch it around" so PEP621-style metadata is transformed into the PEP639-style.
PR Checklist: