Fixed the stuff to use setuptools better. #320
Conversation
|
|
||
| __author__ = 'Mike McKerns' | ||
|
|
||
| __doc__ = """ |
There was a problem hiding this comment.
The __init__.__doc__ needs to stay, as that is what is seen in the docs at http://dill.rtfd.io/.
| @@ -28,7 +28,7 @@ | |||
| load_types(pickleable=True,unpickleable=True) | |||
There was a problem hiding this comment.
Nice idea. I have a few issues, however:
load_typeswith all objects being loaded I believe will incur a speed hit which is fairly unacceptable fordillin use in parallel computing.- I'm not sure I like the name
dill.tools. I think I've usedscriptselsewhere for similar conversions of scripts to functions.
setup.cfg
Outdated
| packages = dill, dill.tools | ||
| zip_safe = False | ||
| python_requires = >=2.5, !=3.0.* | ||
| setup_requires = setuptools; wheel; setuptools_scm |
There was a problem hiding this comment.
While, in the next release or so, I will be trimming off support for some older versions of python, using setuptools_scm will force a drop of several old versions. Old versions need to be supported longer in dill as compared to other packages, as people need a way to open old stored pickled object files.
| zip_safe = False | ||
| python_requires = >=2.5, !=3.0.* | ||
| setup_requires = setuptools; wheel; setuptools_scm | ||
|
|
There was a problem hiding this comment.
These two cfg are some nice modernization.
| @@ -6,314 +6,5 @@ | |||
| # License: 3-clause BSD. The full license text is available at: | |||
| # - https://github.com/uqfoundation/dill/blob/master/LICENSE | |||
|
|
|||
| import os | |||
|
|
|||
| # set version numbers | |||
There was a problem hiding this comment.
Not sure I like the change in version numbering. I need to ponder that a bit...
| from distutils.core import setup | ||
| has_setuptools = False | ||
|
|
||
| # generate version number |
There was a problem hiding this comment.
Admittedly ugly (and hackish code from an old version of numpy) primarily to manage version number and docs. I agree, it does need a change.
| with open('LICENSE') as file: | ||
| license_text = file.read() | ||
|
|
||
| # generate the readme text |
There was a problem hiding this comment.
This shouldn't go away. I need these docs for dill.__init__.__doc__. Unfortunately they are similar but different than in README.md. Not sure how to best resolve that.
There was a problem hiding this comment.
The comments in the More Info section (also in README.md, I believe) are inconsistent with your changes.
There was a problem hiding this comment.
Of course it should go away. If we need __init__.__doc__, you can probably fetch the long_description using pkg_resources (I have never done that though).
There was a problem hiding this comment.
You mean shouldn't, right? dill.__init__.__doc__ is the module doc, and is used for the rtfd entry page. I typically see the long_description be similar if not the same as this doc. Indeed that's what I've been doing. Why remove this doc, and if you remove it, what do you put on the intro page for the sphinx docs?
There was a problem hiding this comment.
I mean the stuff bloating the module should go away - it is ugly and makes it uarder to read. And generating code in setup.py should go away. And custom setup.py for the most of projects should also go away. I mean we defilitely want to keep setup.py minimalist and clean and have everything in setup.cfg. This if sufficient count of projects adopted such minimalistic the same setup.py it should be possible to create a whitelist of hashes of such setup.py files and execute them only when they match the hash. This way it may be possible to get rid of remote code execution when installing python packages.
So IMHO if that large __doc__ is really needed, it should be retrieved dynamically from metadata.
There was a problem hiding this comment.
I agree setup.py could be cleaned up. It has a lot of artifacts that were the way things were done idk maybe 3-5 years ago. Further, having the doc in setup.py and having the doc in __init__ are indeed two different things. However, the __doc__ is needed for the module documentation. You've made it go away in this PR. I'm not going to accept a PR that removes functionality unless it's really warranted, and I'm not at all convinced that removing module documentation is a good thing. I'll consider your PR if you decide you want to handle the review comments.
There was a problem hiding this comment.
I am not going to fix anything in this PR. The main motivation was to clean the disgusting mess in setup.py because I hate the mess and because it is not easy to verify that setup.py doesn't contain malware if it has dynamically-constructed or dynamically-executed code. Every eval or exec is a big red flag meaning potential backdoor (so are pickle, well-known modules doing unpickling, and especially preshipped pickled files. I feel like we need to redesign the language itself to address these problems). And there were real backdoors exploiting that. So before installing a lib I do some static code analysis. Feel free to create an own commit based on this one doing all the fixes you like.
| [options] | ||
| packages = dill, dill.tools | ||
| zip_safe = False | ||
| python_requires = >=2.5, !=3.0.* |
There was a problem hiding this comment.
ctypes requirement is missing. It's needed for certain non-standard platforms, I believe like google cloud, if I remember correctly.
There was a problem hiding this comment.
isn't ctypes a part of python stdlib?
There was a problem hiding this comment.
Yes... or more precisely... believe it or not... most of the time it is. The STL contents are not all the same on all platforms and distributions, and last I checked, it was one of a few libraries that isn't always shipped as part of the STL.
For example: bz2 and sqlite3 aren't installed on Ubuntu by default, and ctypes doesn't come installed by default with pypy or if using MacPorts.
There was a problem hiding this comment.
and ctypes doesn't come installed by default with pypy or if using MacPorts.
I guess that in this case it should be installed using system package manager, like apt and brew, not pip.
There was a problem hiding this comment.
I can't control how people install dill -- the fact is sometimes the STL is slightly different due to how and where it was installed.
There was a problem hiding this comment.
Thue problem is not with dill, but with its native dependencies which are meant to be a part of python interpreter.
There was a problem hiding this comment.
I agree. But, we have a practical problem here, about what to do about it.
| platforms = Linux, Windows, Mac | ||
|
|
||
| [options] | ||
| packages = dill, dill.tools |
There was a problem hiding this comment.
I had dill.tests as a module. While it might not be 100% desirable to include tests as a module, it at least was necessary for tests to run in some cases. Needs investigation.
…ing versions into README.md and adding a file with metainfo has been removed. If one needs a version info or content of README, he should use pkg_resources. Releases are the versions built from git tags. Development releases have git tags with "-dev" :) Also added some goodies, like .editorconfig and ignores.
KOLANICH
force-pushed
the
fixes_by_KOLANICH
branch
from
2d0a3be
to
3d22013
Compare
|
there's been significant updates to dill's build procedure. It still might be nice to cherrypick some stuff from this PR. |
Low-value stuff like inserting versions into README.md and adding a file with metainfo has been removed. If one needs a version info or content of README, he should use
pkg_resources. Releases are the versions built from git tags. Development releases have git tags with "-dev" :) Also added some goodies, like .editorconfig and ignores.