spacetelescope / pysynphot

Synthetic Photometry.
http://pysynphot.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
23 stars 21 forks source link

Consolidate relic initialization #85

Closed jhunkeler closed 6 years ago

jhunkeler commented 6 years ago
  1. Use pkgutil to determine if relic has been installed (i.e. pip). If so, fall through to import relic.release.
  2. Check whether relic exists and is an empty directory (90% chance it's a submodule). If so, initialize the relic submodule.
  3. Check whether relic does not exist. If not, clone relic
  4. If either the submodule initialization or clone operation succeed, insert the local relic directory into sys.path

In all cases import relic.release occurs only once.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 85.363% when pulling 8f3e573031dbe40d9daf485283c5c485b30857c0 on jhunkeler:relic-init-submodule into 91976c9fda59fcfb2217505d29c55055371fb450 on spacetelescope:master.

pllim commented 6 years ago

Doesn't this add git itself as an installation dependency when someone does pip install? I am not sure how I feel about that.

pllim commented 6 years ago

Note: This is an alternative to #84 to address #83.

jhunkeler commented 6 years ago

If #83 handles installing relic as a dependency, the logic in this PR here would import relic.release from site-packages rather than the current directory and skip over the git calls completely.

It's Friday and apparently I can't read!

pllim commented 6 years ago

Currently, relic is not a declared dependency, but a sub-module, so as-is, git logic would be used.

If we want to make relic a formal third-party dependency, then there is no longer a need to have it as a submodule. Right? Or am I confusing myself?

jhunkeler commented 6 years ago

The general approach when not using a submodule is to clone relic with git and use it at runtime to support setup.py. Pretty much every package using relic right now has git as an implicit dependency whether you're compiling against an sdist tarball, or pip/easy_install/pypi.

As far I am aware, we haven't received any tickets about it causing issues in the wild.

PS - Outside of this PR, unless the user clones the repo with git clone --recursive or manually runs git submodule update --init --recursive their relic directory will be empty, and the installation will fail ambiguously.

jhunkeler commented 6 years ago

OK, I tested this PR against a modified MANIFEST.in:

$ cat MANIFEST.in
include README.rst
include LICENSE.md

include RELIC-INFO

recursive-include doc *
recursive-include relic *
prune relic/.git
prune relic/tests

prune build
prune doc/build

global-exclude *.pyc *.o

The relic bundled with the sdist tarball is imported first and the logic in setup.py makes no calls to git. In any case, I believe this PR will still help people that are building with pip install git+https://..., or cloning the repository manually and building from source.

pllim commented 6 years ago

Are you saying that this PR now complements #84 (with the expanded manifest) instead of replacing it? That is, I am to merge both this one and the other one?

jhunkeler commented 6 years ago

Correct... I think this PR is a decent compromise. The sdist bundle of relic will work as intended, and users cloning the repo directly will benefit from not having to do anything special to get it to compile.