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

Setuptools will install licenses if included in setup.cfg #1536

Merged
merged 8 commits into from Jan 27, 2019

Conversation

5 participants
@dtaneli
Copy link
Contributor

dtaneli commented Oct 27, 2018

python setup.py sdist now includes the license file if license_file
is included in setup.cfg unless it is explicitly excluded in MANIFEST.in.

Co-Authored-By: Poyzan Nur Taneli 31743851+ptaneli@users.noreply.github.com

Summary of changes

Closes #357

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details

@dtaneli dtaneli force-pushed the dtaneli:license-fix-357 branch from 930f20f to 372623a Oct 27, 2018

Setuptools will install licenses if included in setup.cfg
Addressing #357

`python setup.py sdist` now includes the license file if `license_file`
is included in `setup.cfg` unless it is explicitly excluded in `MANIFEST.in`.

Co-Authored-By: Poyzan Nur Taneli <31743851+ptaneli@users.noreply.github.com>

@dtaneli dtaneli force-pushed the dtaneli:license-fix-357 branch from 372623a to ca0760a Oct 27, 2018

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Oct 27, 2018

@dtaneli Can you add some tests for this?

Unit tests for installing licenses from setup.cfg (#357)
Co-Authored-By: Poyzan Nur Taneli <31743851+ptaneli@users.noreply.github.com>
@dtaneli

This comment has been minimized.

Copy link
Contributor Author

dtaneli commented Oct 27, 2018

Hi @pganssle, I added some tests but I am not sure if it is better to merge these four test cases into a single test or not. Any preferences?

Also, does this change warrant a changelog update?

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Oct 27, 2018

@dtaneli It is generally preferable to have separate tests, or parametrized tests, but I think in the spirit of DRY (don't repeat yourself), it might be better to refactor these a single parametrized test.

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Oct 27, 2018

And yes, this is a behavior change, it definitely needs a changelog.

@dtaneli

This comment has been minimized.

Copy link
Contributor Author

dtaneli commented Oct 28, 2018

Made the tests parametrized, added a changelog entry and updated thesetup.cfg documentation.

@pganssle, during the London sprint, we discussed re-using license = file:<path> instead of license_file = <path> since the usage of these attributes kind of overlap. Or maybe we would like to support both?

@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Oct 28, 2018

@dtaneli The license field is something completely different, it's used for the name of the license. You'd put something like Apache 2.0 in that field, not the text of the license, or the license file.

@dtaneli

This comment has been minimized.

Copy link
Contributor Author

dtaneli commented Oct 28, 2018

Ok, I just wanted to make sure because we found that you could actually pass a file according to https://setuptools.readthedocs.io/en/latest/setuptools.html#metadata


Is there anything else that is missing for the scope of this issue?

Show resolved Hide resolved setuptools/command/sdist.py Outdated

@pganssle pganssle referenced this pull request Oct 28, 2018

Closed

license field in setup.cfg should not accept file: #1551

4 of 4 tasks complete
Show resolved Hide resolved setuptools/command/sdist.py Outdated
Show resolved Hide resolved setuptools/command/sdist.py Outdated
Show resolved Hide resolved setuptools/tests/test_egg_info.py Outdated
Show resolved Hide resolved setuptools/tests/test_egg_info.py

dtaneli and others added some commits Nov 10, 2018

@jaraco

jaraco approved these changes Jan 27, 2019

@jaraco jaraco merged commit 97e8ad4 into pypa:master Jan 27, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
deploy/netlify Deploy preview ready!
Details

PyPA Sprint Weekend at Bloomberg (2018) automation moved this from Submitted PRs to Merged PRs Jan 27, 2019

@jakirkham

This comment has been minimized.

Copy link
Contributor

jakirkham commented Jan 27, 2019

Thanks for doing this @dtaneli and @jaraco for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment