#1350 - Improvements to handling of License file(s)

[techdragon]

• Fixes License Files listing in Metadata #1350
• Added support for specifying a set of license files as part of the project config.
• Moved the license file config into the package configuration. (Factory.create_poetry()) this matches how similar configurations such as “readme” are configured.
• Updated the builders to use the project level license file list instead of searching for their own (previously different) lists of license files.
• Moved the existing auto-discovery behaviours to become the default case during create_poetry() if nothing specific was configured.
• Added tests for the new features
• Documented the new configuration field.
• Added the “license-files” field to the poetry-schema.json
• Updated pre-commit config to use the correct black source repo, and set the default language for better consistency with any other pre-commit checks that will be added in the future.
• Fix Generated setup.py should use setuptools.setup instead of distutils.core.setup #866 by modifing the base template used by SdistBuilder. It now uses setuptools, since as documented by @seifertm in issue Generated setup.py should use setuptools.setup instead of distutils.core.setup #866 pip uses setuptools anyway and this clears the way to immediately supporting Add support for license_files option in metadata pypa/setuptools#1767 once this is merged and released from setuptools. Until setuptools updates to support multiple license files, this will result in a single log line in the debug output from setuptools so this is ok to merge before the change to setuptools is merged.

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

• Added tests for changed code.
• Updated documentation for changed code.

Note: If your Pull Request introduces a new feature or changes the current behavior, it should be based
on the develop branch. If it's a bug fix or only a documentation update, it should be based on the master branch.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

[techdragon]

Yay, everything passes now. 🎉

[bjoernpollex]

Hey, just dropping in to help with reviewing a bit, hope this is useful, otherwise, just tell me to shut up :)

[techdragon]

The rebase to keep on top of #1349 and the other changes was a little tricky, but I feel like the changes still fit pretty well and it was worth it since new tests found an easily fixed mistake with how I was using Path objects in sdist.py 😄

This PR is once again ready for a merge! 🎉

[sdispater]

I am not sure what this PR tries to solve. Instead of using a new license_files property, isn't it possible to use the include property?

[techdragon]

T.L.D.R. Version = Yes poetry can use include for license files, but it doesn't know which files it included in the package are the license files for the package. This change makes it so poetry knows which included files are license files so the package builder classes can use that info.

@sdispater You are right about being able to achieve some of this just by using include but the point of the change is to better handle the metadata that is represented by the new field.

At the moment you can include a license file using include, but this just includes them the same as any other file and does not associate them as special in any way.

There is currently an evolving discussion about the next set of changes to the package metadata format. https://discuss.python.org/t/improving-license-clarity-with-better-package-metadata/2154 and while the discussion isn't finalised yet the idea of adding a specific field to the package metadata to specify which included files are license related seems to have no one arguing against it so it is likely to be part of the next revision.

The changes in this PR, put poetry in a position to easily adapt to this potential future revision by keeping track of which included files were license related so that this can be passed on in the package metadata.

Case in point, I mentioned the open PR on setuptools, which when merged will make it possible to pass a list into the license_files keyword argument of the setup() function. I've added this functionality into the sdist builder class since setuptools is pretty tolerant of keyword arguments it doesn't know how to handle, as outlined in the linked PR.

[funkyfuture]

i'm not sure whether the stdlib's globbing implementation supports this, but {COPYING*,LICENSE*} should be enough.

[techdragon]

The standard library globbing doesnt support multiple patterns unfortunately. https://docs.python.org/3/library/glob.html#glob.glob

I've just tried a few variations but none of them are particularly nice looking, and black has a tendency to reformat them over more lines than the code they would replace.

            for glob_pattern in ("LICENSE*", "COPYING*"):
                for license_file in poetry_file.parent.glob(glob_pattern):
                    package.license_files.add(
                        license_file.relative_to(poetry_file.parent)
                    )

The code above was the nicest result and I'm happy to change it but it is at the end of the day... really just a fancier way of doing the same thing as the original code below.

            for license_file in poetry_file.parent.glob("LICENSE*"):
                package.license_files.add(license_file.relative_to(poetry_file.parent))
            for license_file in poetry_file.parent.glob("COPYING*"):
                package.license_files.add(license_file.relative_to(poetry_file.parent))

[techdragon]

@funkyfuture Any more thoughts on this?

[funkyfuture]

i made some remarks.

[sdispater]

I think we should keep this line if there are no license_files specified in the pyproject.toml file, so that people relying on the current behavior are not surprised and published packages are not suddenly missing license files.

[techdragon]

@sdispater - I agree this should remain the "default path" and I made sure to retain this behaviour as part of my changes. This functionality was consolidated into the package setup stuff (which is now in factory.py) and is currently being discussed above: #1367 (comment) & #1367 (comment)

The specific lines for automatically adding LICENSE* and COPYING* is in factory.py here:

• https://github.com/techdragon/poetry/blob/issue-1350-license-metadata-improvements/poetry/factory.py#L84-L92

And I also added a test case for this existing/default behaviour when no license_files have been specified. 😃https://github.com/techdragon/poetry/blob/issue-1350-license-metadata-improvements/tests/masonry/builders/test_complete.py#L394-L416

[techdragon]

@sdispater Since the functionality is retained as desired, but powered by new code in a different place, if you don't mind, id like to mark this as resolved. 😄

[sdispater]

As an optional feature I think this is a good idea. It will likely not make it to the 1.0.0 version since it's now pretty much feature-frozen but it will be integrated in a future minor version.

[techdragon]

Back for a new year and I see that pypa/setuptools#1767 has been merged and poetry 1.0.0 has shipped! So I'm going to try and find a little time this week to update this PR.

[Secrus]

@techdragon are you still interested in bringing this to Poetry? If so, please merge master and change the target branch to master. This will allow for review against the current codebase.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.