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

Move non-library package data outside of Python package #72

Merged
merged 1 commit into from
Jan 17, 2018
Merged

Move non-library package data outside of Python package #72

merged 1 commit into from
Jan 17, 2018

Conversation

jdufresne
Copy link
Contributor

@jdufresne jdufresne commented Jul 23, 2017

This avoids having the files themselves installed as Python files used by the package. As these files are not used by the actual Python code, no need to install alongside the library. All files remain a part of the
source package, are just not installed.

The license file is now installed using the wheel package's supported method. Added to [metadata] to setup.cfg with the property license-file, with this, the license is now installed in to the dist-info directory.

This change helps move towards the goal of distributing olefile as a single module instead of a package of one module. Once this can be achieved, there will be less indirection and unnecessary nested layers
in the distribution. Moves toward a Python community conventions.

@coveralls
Copy link

coveralls commented Jul 23, 2017

Coverage Status

Coverage remained the same at 63.712% when pulling 1f962cd on jdufresne:manifest-in into e7eb5aa on decalage2:master.

@coveralls
Copy link

coveralls commented Jul 23, 2017

Coverage Status

Coverage remained the same at 63.712% when pulling 1f962cd on jdufresne:manifest-in into e7eb5aa on decalage2:master.

setup.cfg Outdated
@@ -1,2 +1,5 @@
[bdist_wheel]
universal = True

[metadata]
license-file = LICENSE.txt
Copy link
Owner

Choose a reason for hiding this comment

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

what does this license-file does exactly? (could not find it in the distutils documentation yet)

@jdufresne
Copy link
Contributor Author

what does this license-file does exactly? (could not find it in the distutils documentation yet)

Unfortunately, it isn't well documented. Best "documentation" I can find is the related PR to the wheel package, here.

With this, building the wheel package adds the LICENSE.txt file to the the wheel. For example:

$ python setup.py bdist_wheel
... [wheel output] ...
$ unzip -v dist/olefile-0.45.dev1-py2.py3-none-any.whl
Archive:  dist/olefile-0.45.dev1-py2.py3-none-any.whl
 Length   Method    Size  Cmpr    Date    Time   CRC-32   Name
--------  ------  ------- ---- ---------- ----- --------  ----
    1019  Defl:N      574  44% 07-05-2017 01:39 c4ad339c  OleFileIO_PL.py
    1194  Defl:N      673  44% 07-05-2017 01:40 6c6d0fbd  olefile/__init__.py
  100721  Defl:N    27511  73% 07-05-2017 01:39 290ea087  olefile/olefile.py
    9139  Defl:N     3775  59% 07-24-2017 00:12 8f3b1bee  olefile-0.45.dev1.dist-info/DESCRIPTION.rst
    2846  Defl:N     1335  53% 07-24-2017 00:12 0b60746d  olefile-0.45.dev1.dist-info/LICENSE.txt
    1595  Defl:N      656  59% 07-24-2017 00:12 f4eab8d1  olefile-0.45.dev1.dist-info/metadata.json
      21  Defl:N       21   0% 07-24-2017 00:12 bf7d2a40  olefile-0.45.dev1.dist-info/top_level.txt
     113  Defl:N       98  13% 07-24-2017 00:12 7b96df94  olefile-0.45.dev1.dist-info/WHEEL
   10677  Defl:N     4172  61% 07-24-2017 00:12 a1ee8367  olefile-0.45.dev1.dist-info/METADATA
     845  Defl:N      510  40% 07-24-2017 00:12 60ed45b8  olefile-0.45.dev1.dist-info/RECORD
--------          -------  ---                            -------
  128170            39325  69%                            10 files

Notice the file olefile-0.45.dev1.dist-info/LICENSE.txt. If I install this wheel, this file is also installed:

$ pip install dist/olefile-0.45.dev1-py2.py3-none-any.whl
... [pip output] ...
$ cat ~/.venv/olefile/lib/python3.6/site-packages/olefile-0.45.dev1.dist-info/LICENSE.txt
... [olefile LICENSE.txt content] ...

So, this is an established feature for bundling LICENSE files when installing Python packages.

@jdufresne
Copy link
Contributor Author

what does this license-file does exactly? (could not find it in the distutils documentation yet)

This is now documented in the Wheel documentation:

https://wheel.readthedocs.io/en/stable/index.html#including-the-license-in-the-generated-wheel-file

Including the license in the generated wheel file

Several open source licenses require the license text to be included in every distributable artifact of the project. Currently, the only way to to do this with “wheel” is to specify the license_file key in the [metadata] section of the project’s setup.cfg:

[metadata]
license_file = LICENSE.txt

The file path should be relative to the project root. The file will be packaged as LICENSE.txt (regardless of the original name) in the .dist-info directory in the wheel

@coveralls
Copy link

coveralls commented Dec 10, 2017

Coverage Status

Coverage remained the same at 65.298% when pulling 79c0def on jdufresne:manifest-in into dadafb8 on decalage2:master.

@jdufresne
Copy link
Contributor Author

I've updated the MANIFEST.in file so the moved files will continue to be distributed with the source distribution.

@coveralls
Copy link

coveralls commented Dec 10, 2017

Coverage Status

Coverage remained the same at 65.298% when pulling ea60c4a on jdufresne:manifest-in into dadafb8 on decalage2:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 65.298% when pulling ea60c4a on jdufresne:manifest-in into dadafb8 on decalage2:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 65.298% when pulling ea60c4a on jdufresne:manifest-in into dadafb8 on decalage2:master.

@coveralls
Copy link

coveralls commented Dec 10, 2017

Coverage Status

Coverage remained the same at 65.298% when pulling ea60c4a on jdufresne:manifest-in into dadafb8 on decalage2:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 65.298% when pulling ea60c4a on jdufresne:manifest-in into dadafb8 on decalage2:master.

@decalage2
Copy link
Owner

OK, thanks @jdufresne. I will accept this PR once I have checked all the files and resolved the conflict.

I do not plan to make olefile a simple module as it was the case in the past, just because I might split the code into several files in the future. So it's easier if we already have a package structure. I know it looks a bit strange to have a package for a single module, but I'd prefer to keep it like this.

Unless it creates actual issues, of course. Is that a problem?

@jdufresne
Copy link
Contributor Author

Nope no problem at all. Thanks for the response and considering. And thanks for maintaining the project.

Rebased to resolve merge conflicts.

@coveralls
Copy link

coveralls commented Dec 10, 2017

Coverage Status

Coverage remained the same at 65.298% when pulling 746f976 on jdufresne:manifest-in into a844c0f on decalage2:master.

This avoids having the files themselves installed as Python files used
by the package. As these files are not used by the actual Python code,
no need to install alongside the library. All files remain a part of the
source package, are just not installed.

The license file is now installed using the wheel package's supported
method. Added to [metadata] to setup.cfg with the property license-file,
with this, the license is now installed in to the dist-info directory.
@coveralls
Copy link

coveralls commented Jan 4, 2018

Coverage Status

Coverage remained the same at 65.298% when pulling 6c9fda5 on jdufresne:manifest-in into a75f539 on decalage2:master.

@jdufresne
Copy link
Contributor Author

Rebased to resolve merge conflicts.

@decalage2 decalage2 merged commit 587203b into decalage2:master Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants