search

desihub / desiutil

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

install py or src not both; no silent installdir fallback #163

Closed sbailey closed 3 years ago

sbailey commented 3 years ago

Our two hybrid python/C++ packages (fiberassign and specex) have now converted to using python setup.py install for installations, including compiling the C++ code. This was creating a fake-failure problem for desiInstall which was noticing the src/ directory and trying to run a non-existent Makefile. This PR updates the installation code to use build type "py" OR "make" OR "src" but not "py" AND "src". I left the "make" and "src" logic in place even though we don't have any packages using them now, but if the setup.py file is found it uses that and then moves on.

This also fixes a bug where if the desiInstall --root ...directory didn't exist at NERSC, the code was silently falling back to the default NERSC install location instead of warning the user that their requested installation location doesn't exist and stop.

With current master:

[cori12 temp] desiInstall -r $SCRATCH/desi/temp fiberassign 2.1.1
CRITICAL:install.py:866:install:2021-02-11T15:15:02: Error during compile: make: *** src: No such file or directory.  Stop.

That produces a useable installation, albeit without the permissions fully corrected.

With this branch there are no errors, fully successful install.

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 67.893% when pulling a8c2d69b0c81c77f06f26fc3dd5782251f34180e on install_no_src into 80714f7932ac33d170282804786c1609e4701b5e on master.

weaverba137 commented 3 years ago

I think this is fine at first glance, but there is also the desiInstall.rst documentation that will need to be changed. Could you do that?

weaverba137 commented 3 years ago

Even though these are vestigial, is the remaining logic "make" XOR "src"? That is, all possible build types are mutually exclusive?

sbailey commented 3 years ago

My intension was to make all build types exclusive (make and src already were, but previously they could be combined with py). I'll update docs.

weaverba137 commented 3 years ago

Good, thank you. I'll do a formal review soon, but it might have to wait until tomorrow morning.

sbailey commented 3 years ago

@weaverba137 thanks for the comments / suggestions. I think I have addressed all of them. Please re-review.

weaverba137 commented 3 years ago

OK to merge now. I need to do an overhaul of the desiutil testing infrastructure anyway but I will punt that to a separate issue. I can create a tag after merge if you're ready for that.

sbailey commented 3 years ago

Thanks, merging now. I may have another PR or two for desiutil today prior to a tag for 21.2 / Cascades, so no tag yet.