search

tulip-control / polytope

Geometric operations on polytopes of any dimension
https://pypi.org/project/polytope
Other
74 stars 19 forks source link

Moved git hash getting scripts from setup.py into the version module. #28

Closed carterbox closed 8 years ago

carterbox commented 8 years ago

The code that appends the git hash to the end of the version number was split between setup.py and polytope.version. This made it hard to copy into other projects. I have moved some of the git hash code into polytope.version such that it is more self-contained. I also think that it makes setup.py easier to read. The only functional difference you should notice is that now you specify the version number inside setup.py above the classifiers instead of in polytope.version.

johnyf commented 8 years ago

The version definition mechanism is planned to be replaced with that from dd, which has also been used in tulip, which was introduced there with https://github.com/tulip-control/tulip-control/pull/145. One difference is that gitpython is used in those versions

Also, I have to note that fetching the version from git seems simple, but I have found it extremely difficult to write it correctly. The reason is that there are a great number of cases (git present or not, gitpython present or not -- in the implementation mentioned above --, git in path or not, .git present or not, dirty working directory or not, tagged version or not, tag matched by version regular expression, number within tag matches number declared as version in setup.py or not, etc.). This situation is aggravated further by the bootstrapping nature of the environment in which this operation can possibly run (user deployment with almost nothing installed).

Having said this, there are a few comments on the changes:

  1. defining the version number in setup.py instead of version.py is a good change, and the same as in dd/setup.py and tulip/setup.py (flat better than nested, PEP 20). The main reason for doing so is to avoid importing the package (or using imp to import files from the package) at a stage when no dependency has yet been installed.
  2. the code that appends the version number is not intended to be made available to the user, i.e., to be imported to other projects. Making such facilities available in setuptools is a valid concern, but not to be addressed at the level of polytope. An example of a project that aims at providing such functionality is mentioned here.
  3. The proposed change does not define the variable version in version.py, which means that __init__.py will fail to import the version number, which is probably what causes the tests to fail.
carterbox commented 8 years ago

Cool. I like the way in which tulip creates _version.py during installation.