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

Fixed the stuff to use setuptools better. #320

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KOLANICH
Copy link

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.

Copy link
Member

@mmckerns mmckerns left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I agree, lots of good stuff here. Please see the inline comments.


__author__ = 'Mike McKerns'

__doc__ = """
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

Nice idea. I have a few issues, however:

  • load_types with all objects being loaded I believe will incur a speed hit which is fairly unacceptable for dill in use in parallel computing.
  • I'm not sure I like the name dill.tools. I think I've used scripts elsewhere 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

@mmckerns mmckerns Jun 26, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

The comments in the More Info section (also in README.md, I believe) are inconsistent with your changes.

Copy link
Author

@KOLANICH KOLANICH Jun 26, 2019

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@KOLANICH KOLANICH Jun 28, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

@mmckerns mmckerns Jun 29, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Author

@KOLANICH KOLANICH Jun 29, 2019

Choose a reason for hiding this comment

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

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.*
Copy link
Member

Choose a reason for hiding this comment

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

ctypes requirement is missing. It's needed for certain non-standard platforms, I believe like google cloud, if I remember correctly.

Copy link
Author

@KOLANICH KOLANICH Jun 26, 2019

Choose a reason for hiding this comment

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

isn't ctypes a part of python stdlib?

Copy link
Member

@mmckerns mmckerns Jun 26, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Thue problem is not with dill, but with its native dependencies which are meant to be a part of python interpreter.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. But, we have a practical problem here, about what to do about it.

platforms = Linux, Windows, Mac

[options]
packages = dill, dill.tools
Copy link
Member

Choose a reason for hiding this comment

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

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.
@mmckerns
Copy link
Member

there's been significant updates to dill's build procedure. It still might be nice to cherrypick some stuff from this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants