desihub / desiutil

General DESI utilities, shell scripts, desiInstall, etc.
BSD 3-Clause "New" or "Revised" License
3 stars 9 forks source link

desiInstall of specex/main and fiberassign/main, which need compilation too #186

Closed sbailey closed 2 years ago

sbailey commented 2 years ago

desiInstall supports "in-place" installations of python repos that adds $PRODUCT_DIR/py to $PYTHONPATH and $PRODUCT_DIR/bin to $PATH so that any changes to the repo are automatically available without having to do a separate installation step. Good.

specex and fiberassign, however, are hybrid repos that have python code with compiled extensions. An in-place install is handy when making changes to any of the python code, but if any of the C++ code changes it still has to be compiled using:

python setup.py build_ext --inplace

desiInstall doesn't know this, and this pattern doesn't fit any of the build types listed at https://desiutil.readthedocs.io/en/latest/desiInstall.html#determine-build-type .

What's the best way to get desiInstall to know that it needs to run this extra step for these two repos?

A somewhat hacky solution that may not require changing desiInstall is to leverage its special case of looking for an etc/{productname}_data.sh script and executing that, e.g. as used by desimodel to get the data from svn. specex and fiberassign could add their own etc/*_data.sh scripts to run python setup.py build_ext --inplace, but that is somewhat cryptically using a data-download hook for other purposes.

It might be better to define another hook similar to etc/*_data.sh, and if desiInstall detects that it will run it for in-place branch installations, but not for regular installations. That requires an update to both desiInstall and the specex+fiberassign repos, but it might be more obvious and maintainable in the future.

For context, both specex and fiberassign used to have a Makefile that desiInstall knew to run, but both have migrated to a python-first approach with compiled extensions without a Makefile. Current master (now main) installations have bootstrapped the python setup.py build_ext --inplace upon first installation, after which the desitest nightly update cronjob re-runs that every night after git pull. The point of this ticket is so that the end-user doesn't have to remember to do special steps whenever they make a fresh main installation.

@weaverba137 thoughts?

weaverba137 commented 2 years ago

The implementation of this will not be hard, but there is a potential gotcha: the use of python setup.py ... is generally deprecated, so we don't want to come up with a mechanism that binds us into that command in a way that is difficult to subsequently change.

Analogous to e.g. etc/desimodel_data.sh I suggest a etc/package_compile.txt file that itself contains python setup.py build_ext --inplace, but which could be replaced by a different compile command either in the future, or if the package is already set up that way.

weaverba137 commented 2 years ago

After further discussion we will support a etc/package_compile.sh script that will be run by desiInstall and only for branch installs. The script will accept a command-line argument that is the path to the Python executable, which desiInstall knows. For example:

#!/bin/bash
# Additional documentation...
py=$1
${py} setup.py build_ext --inplace