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

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

Closed
sbailey opened this issue Aug 16, 2022 · 2 comments · Fixed by #188
Closed

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

sbailey opened this issue Aug 16, 2022 · 2 comments · Fixed by #188
Assignees

Comments

@sbailey
Copy link
Contributor

sbailey commented Aug 16, 2022

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

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

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

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

Successfully merging a pull request may close this issue.

2 participants