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

Respect REUSE spec when including licenses #57

Merged
merged 14 commits into from
Aug 30, 2020
Merged

Conversation

MrJohz
Copy link
Contributor

@MrJohz MrJohz commented Aug 10, 2020

  • The REUSE spec states that a LICENSES folder should exist with copies of all of the licenses used in this project.
  • Previously, poetry tried to include all files like LICENSE*, but this didn't respect directories, meaning that poetry would fail if a LICENSES folder existed. (This is the Pip local install (via PEP 517) fails if a folder named LICENSES is present in the source code poetry#2565 bug.)
  • Now poetry is a bit more specific about the "correct" license files to add
    • LICENSE or COPYING
    • LICENSE.* or COPYING.* (i.e. with any extension)
    • LICENSES/** (i.e. all files within the LICENSES directory)
  • In addition, if a suggested licensing file doesn't exist, or isn't a file, poetry will now simply ignore it, rather than throwing an error.

Resolves: python-poetry/poetry#2565

  • Added tests for changed code.
  • Updated documentation for changed code. (I don't think this is necessary, as there isn't any documentation for the original behaviour? I can add some if that would be preferred.)

This is a copy of python-poetry/poetry#2573, which I will now close given that the relevant code has been moved to poetry-core.

* REUSE spec states that a LICENSES folder should exist with copies of
  all of the licenses used in this project.
* Previously, poetry tried to include all files like LICENSE*, but this
  didn't respect directories, meaning that poetry would fail if a
  LICENSES folder existed.
* Now poetry is a bit more specific about the "correct" license files to
  add
  * LICENSE or COPYING
  * LICENSE.* or COPYING.*
  * LICENSES/**
* In addition, if a suggested licensing file doesn't exist, or isn't a
  file, poetry will simply ignore it.
@milliams
Copy link

As nothing more than a user who has been hit by python-poetry/poetry#2565, this looks like it would solve my issue which is currently preventing me from releasing.

Also, indeed I can't see anywhere in the documentation that would need updating.

@finswimmer finswimmer requested a review from a team August 12, 2020 06:49
license_files_to_add.append(self._path / base)
license_files_to_add.extend(self._path.glob(base + ".*"))

license_files_to_add.extend((self._path / "LICENSES").glob("**" + os.sep + "*"))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
license_files_to_add.extend((self._path / "LICENSES").glob("**" + os.sep + "*"))
license_files_to_add.extend(self._path.glob("LICENSES/**/*"))

pathlib is smart enough to handle OS diferences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Useful to know, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that this isn't the case when using the .glob method. I'm not sure why it isn't normalised, but you can see in this run when this change was added that the only failure was in the Windows builds.

I've reverted this change as a result.

Copy link
Member

Choose a reason for hiding this comment

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

I'll check shortly but this might be related to https://bugs.python.org/issue31202

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it is. In this case; can you try this suggestion? This should also confirm that the issue is as identified above or if pathlib does not handle os diferences as advertised.

Suggested change
license_files_to_add.extend((self._path / "LICENSES").glob("**" + os.sep + "*"))
license_files_to_add.extend(self._path.joinpath("LICENSES").glob("**/*"))

poetry/core/masonry/builders/wheel.py Show resolved Hide resolved
poetry/core/masonry/builders/wheel.py Outdated Show resolved Hide resolved
poetry/core/masonry/builders/wheel.py Outdated Show resolved Hide resolved
@MrJohz
Copy link
Contributor Author

MrJohz commented Aug 17, 2020

@abn - I worked through your suggestions, but they didn't all work out so successfully because of various weird reasons - most notably, the .glob method in Windows doesn't normalise path separators.

I replied to your comments inline - could you advise whether the new changes are sufficient, or whether I should make other changes?

Thanks!

@abn abn merged commit a522953 into python-poetry:master Aug 30, 2020
@sdispater sdispater mentioned this pull request Sep 18, 2020
stephanlukasczyk added a commit to se2p/pynguin that referenced this pull request Oct 29, 2020
The introduction of REUSE compatibility breaks the build of the wheel
file.  python-poetry/poetry-core#57 closed the
issue on the poetry side, although `poetry-core` has not yet been
updated.  Nevertheless, I still do this change now, to have it available
for future releases.
@stephanlachnit
Copy link

Debian Developer here, is there an option to disable this behavior? Distributions usually handle the location of the licensing information themselves, so it would be nice if this could be changed with an environment variable

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.

Pip local install (via PEP 517) fails if a folder named LICENSES is present in the source code
4 participants