search

villano-lab / nrCascadeSim

calculating the NR spectrum resulting from neutron-capture cascades.
MIT License
0 stars 1 forks source link

[JOSS Review] Improve `make install` to work for local installation #85

Closed nuclearGoblin closed 2 years ago

nuclearGoblin commented 2 years ago

From the review thread:

Change the Makefile to remove targeting /usr/local as a default. In many situations this is an unwritable location for users. If #64 is enacted this can be easily achieved with setting the CMAKE_INSTALL_PREFIX option during cmake. Remove recommendations of use of sudo to install software. Anyone who needs to use sudo does not need to be told to use sudo and all software should also be able to be installed and used locally.

matthewfeickert commented 2 years ago

From https://github.com/openjournals/joss-reviews/issues/3993#issuecomment-1015075303:

The install Make target

https://github.com/villano-lab/nrCascadeSim/blob/bf4795d83f8be6c9c1d737f5f51741def32cceb5/Makefile#L80-L82

hardcodes the install target directory to /usr/local/bin. Install locations can have defaults, but should allow for user input — there are many situations on remotes (example: CERN's LXPLUS) where users do not have access to /usr/local/ and the users would need to manually edit the Makefile.

Also, the clean Make target should for the same reasons not try to rm -f anything under /usr/local.

https://github.com/villano-lab/nrCascadeSim/blob/bf4795d83f8be6c9c1d737f5f51741def32cceb5/Makefile#L95-L96

This is also probably never what a user wants to have happen when make clean is run — clean targets are not uninstallers. Similarly, advocating directly for the use of sudo for installation of local software should be avoided. Anyone who needs to use sudo should not need to be told to use sudo and all software should also be able to be installed and used locally without sudo powers.

A better alternative default is to assume the user is NOT root, and to attempt to install in the user's ~/bin where they will be assured write access.

As mentioned, if Issue #64 is able to be resolved you can get this one basically for free, as you can specify the installation path with CMAKE_INSTALL_PREFIX

e.g.

cmake \
  -DCMAKE_INSTALL_PREFIX=/home/physicist/bin \
  -S . \
  -B ../build
villaa commented 2 years ago

Migrated to CMake in PR #91 which has a consistent and standard way to specify an install directory whether it's local or system-wide.