-
Notifications
You must be signed in to change notification settings - Fork 9
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
install py or src not both; no silent installdir fallback #163
Conversation
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? |
Even though these are vestigial, is the remaining logic "make" XOR "src"? That is, all possible build types are mutually exclusive? |
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. |
Good, thank you. I'll do a formal review soon, but it might have to wait until tomorrow morning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a few minor issues to discuss.
I accidentally hit the "approve" button. I should have hit "Request changes"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my other review. This is just to ensure that a "Request changes" result is returned.
@weaverba137 thanks for the comments / suggestions. I think I have addressed all of them. Please re-review. |
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. |
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. |
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 thesrc/
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 thesetup.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:
That produces a useable installation, albeit without the permissions fully corrected.
With this branch there are no errors, fully successful install.