MesserLab / SLiM

SLiM is a genetically explicit forward simulation software package for population genetics and evolutionary biology. It is highly flexible, with a built-in scripting language, and has a cross-platform graphical modeling environment called SLiMgui.
https://messerlab.org/slim/
GNU General Public License v3.0
160 stars 30 forks source link

Ubuntu SLiM install error #441

Closed NikolayOskolkov closed 3 months ago

NikolayOskolkov commented 4 months ago

I am installing SLiM via DebianUbuntuInstall.sh script on Ubuntu 18, and getting this error:

Argument not separated from preceding token by whitespace.
This warning is for project developers.  Use -Wno-dev to suppress it.

-- The C compiler identification is GNU 7.5.0
-- The CXX compiler identification is GNU 7.5.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- CMAKE_BUILD_TYPE is Release
-- BUILD_SLIMGUI is ON
-- Found OpenGL: /usr/lib/x86_64-linux-gnu/libOpenGL.so   
CMake Error at CMakeLists.txt:352 (install):
  install DIRECTORY given no DESTINATION!

-- Configuring incomplete, errors occurred!
See also "/tmp/BUILD/CMakeFiles/CMakeOutput.log".
Build failed. Please see the output and make a post on the slim-discuss mailing list.
The output from this build is stored in '/var/log/' as SLiM-CMakeOutput-2024-04-29T10:17:43+02:00.log.

The log-file is attached SLiM-CMakeOutput-2024-04-29T10:17:43+02:00.log

NikolayOskolkov commented 4 months ago

I figured out that there was a problem in the CMakeLists.txt with this line:

install(DIRECTORY data/ TYPE DATA)

I commented this line out and the installation of the SLiMgui from source worked for me. Apparently this line is somehow broken or missing the DESTINATION variable. At least the error I got above was related to the missing DESTINATION variable, as far as I understand

bhaller commented 4 months ago

Thanks @NikolayOskolkov. @bryce-carson thoughts on this? I guess he is on Ubuntu18, which is rather old; perhaps it is not quite compatible with the install script?

bryce-carson commented 4 months ago

Thanks @NikolayOskolkov. @bryce-carson thoughts on this? I guess he is on Ubuntu18, which is rather old; perhaps it is not quite compatible with the install script?

Yes, the version of CMake is likely too old.

The function call is correct per this documentation.

bryce-carson commented 4 months ago

I am installing SLiM via DebianUbuntuInstall.sh script on Ubuntu 18, and getting this error:

Argument not separated from preceding token by whitespace.
This warning is for project developers.  Use -Wno-dev to suppress it.

-- The C compiler identification is GNU 7.5.0
-- The CXX compiler identification is GNU 7.5.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- CMAKE_BUILD_TYPE is Release
-- BUILD_SLIMGUI is ON
-- Found OpenGL: /usr/lib/x86_64-linux-gnu/libOpenGL.so   
CMake Error at CMakeLists.txt:352 (install):
  install DIRECTORY given no DESTINATION!

-- Configuring incomplete, errors occurred!
See also "/tmp/BUILD/CMakeFiles/CMakeOutput.log".
Build failed. Please see the output and make a post on the slim-discuss mailing list.
The output from this build is stored in '/var/log/' as SLiM-CMakeOutput-2024-04-29T10:17:43+02:00.log.

The log-file is attached SLiM-CMakeOutput-2024-04-29T10:17:43+02:00.log

The error is occurring because the version of CMake included with Ubuntu 18 is likely too old and doesn't support the DIRECTORY type installation method.

Please read the documentation I linked to in my reply to Ben, and identify what the value of the variable should be and use that as the destination argument (substituting it for type data appropriately [double check the syntax]).

At this late hour in which I reply, I'm not sure if the desktop integration target was the only thing that failed. If so, that's okay, it's not difficult to install the files manually. I'll provide you with a more detailed response/fix on the 1st.

Thanks for the issue report.

bhaller commented 4 months ago

OK. Perhaps the correct fix here is to update the required CMake version. At the top of CMakeLists.txt is cmake_minimum_required (VERSION 2.8.12 FATAL_ERROR). What CMake version is required for this functionality to work correctly? If we required that version, people on, e.g., Ubuntu18 with an older CMake would get an error, and would do whatever they need to do to get the required CMake. What do you think of that, @bryce-carson? 2.8.12 is probably ancient by now; I think that requirement has not been pushed since SLiM started using CMake. How many people do you think would be inconvenienced by this?

The other obvious solution is to find a way to do what we want to do in CMake in a way that works even in 2.8.12. I have no idea, regarding that. :-> Thoughts?

bryce-carson commented 4 months ago

I can't remember the lowest version number of Ubuntu that we support, given its shipped version of the Qt5 libraries. Whatever the version of CMake that ships is likely a good target, if it's not a lower version than what we already support.

Given the differences between various Linux distributions, we might be best to simply instruct any user with unexpected errors to manually install the package, following their distribution's instructions. Some users won't know how to find those instructions because they're meant for system administrators (those who install software!).

Emulating future CMake version features with its scripting capabilities is bound to be too fragile to support, so I'll instead whip together a feature in the BASH script to default to the older, hard-coded installation of "data" files when the version of CMake is too old. That should be robust, and handles the installation for users rather than requiring them to swim in documentation they probably don't want to read.

bhaller commented 4 months ago

I can't remember the lowest version number of Ubuntu that we support given its shipped version of the Qt5 libraries. Whatever the version of CMake that ships is likely a good target, if it's not a lower version than what we already support.

I think we support 18.04, actually? I'm not sure what version of CMake it ships with, but I think the CMake build works fine on 18.04 except for this install step...?

Given the differences between various Linux distributions, we might be best to simply instruct amy user with unexpected errors to manually install following their distributions instructions. Some users won't know how to find those instructions because they're meant for system administrators (those who install software!).

Emulating future CMake version features with its scripting capabilities is bound to be too fragile to support, so I'll instead whip together a feature in the BASH script to default to the older, hardcoded installation of "data" files when the version of CMake is too old. That should be robust, and handles the installation for users rather than requiring them to swim in documentation they probably don't want to read.

That sounds like a good solution to me. So on Ubuntu18 it will avoid the CMake step that fails, and instead just do that part of the install directly. Sounds good, less fighting with CMake. :-> Thanks @bryce-carson.

bryce-carson commented 4 months ago

OK. Perhaps the correct fix here is to update the required CMake version. At the top of CMakeLists.txt is cmake_minimum_required (VERSION 2.8.12 FATAL_ERROR). What CMake version is required for this functionality to work correctly?

3.14 🥧

bhaller commented 4 months ago

OK. Perhaps the correct fix here is to update the required CMake version. At the top of CMakeLists.txt is cmake_minimum_required (VERSION 2.8.12 FATAL_ERROR). What CMake version is required for this functionality to work correctly?

3.14 🥧

Hmm. That's be a big jump in required version number. Since some people are apparently still on Ubuntu18, it'd break people. I like the suggestion you made above for a workaround, without bumping the required version.

bryce-carson commented 4 months ago

OK. Perhaps the correct fix here is to update the required CMake version. At the top of CMakeLists.txt is cmake_minimum_required (VERSION 2.8.12 FATAL_ERROR). What CMake version is required for this functionality to work correctly?

3.14 🥧

Hmm. That's be a big jump in required version number. Since some people are apparently still on Ubuntu18, it'd break people. I like the suggestion you made above for a workaround, without bumping the required version.

I'm thinking we bump the required version, since it is required for the presently used syntax/semantics in the file, and using the work-around in DebianUbuntuInstall.sh.

Essentially,

The shell script for Debian and Ubuntu, since it requires root and builds the software anyways, may as well build a binary package and install that: it'd allow users to uninstall the software cleanly, and provide for better quality control by allowing previous versions to be replaced in a more structured way than simply replacing the binaries.

I'll be considering these further and doing some tests, but I'll push a quick fix for the installation shell script sooner than other things.

bhaller commented 4 months ago

Hi @bryce-carson. I'm not sure that I understand all the ins and outs here, sorry. But. Right now the overall CMakeLists.txt requires 2.8.12 overall, and requires 3.1.0 if and only if SLiMgui is being built (inside if(BUILD_SLIMGUI)). Can we bump that 3.1.0 requirement to 3.14, for SLiMgui, and leave the overall 2.8.12 requirement alone? What I want to avoid is breaking a whole bunch of people who build slim, but not SLiMgui, on their computing cluster that has an archaic software stack (as many/most clusters seem to have). As I understand it, this version requirement is only involved with installing SLiMgui into the desktop environment, correct? In fact, can the higher version requirement be imposed only for a make install of SLiMgui, where 3.14 is in fact needed? I'm aiming to break as few people as possible with this change.

And then there is the part of your comment above where you say:

The shell script for Debian and Ubuntu, since it requires root and builds the software anyways, may as well build a binary package and install that: it'd allow users to uninstall the software cleanly, and provide for better quality control by allowing previous versions to be replaced in a more structured way than simply replacing the binaries.

Is this related to this issue, CMake versioning, etc., or is this a separate, orthogonal proposal? It sounds like an improvement, but I wonder what the downsides might be...? Will it be more fragile, or harder to maintain, or work on fewer platforms, or anything like that? And is anything strongly driving the necessity of it, or is it just an optional improvement that you thought of?

I'm in Europe for the next two months, teaching SLiM workshops and such, and it is difficult to deal with fallout from issues over here (my build machines are back in NY, so dealing with new releases requires remote access), and it is crucial not to break anything until the workshops are done – if installation gets screwed up just before a workshop, I end up with a whole workshop full of people who can't install the software. So until I'm back in the U.S., I would like to keep changes to the minimum necessary. So if this idea is indeed separate/orthogonal, how about we open a new issue for it, and handle that later when the timing is more favorable?

bryce-carson commented 4 months ago

@grahamgower Hey, I could use your input on a shell command I'm needing in a shell script to fix this issue. Reading on SO, I saw that cp -rf a/ b/A might do what I want, which is to merge similar directory trees. I want to take the new files, which are somewhere in this tree, and install them into the correct locations in a directory tree with the same main branches. If a is a source tree mirroring the directory layout of /usr/local/share (it contains directories with the same names as b/A, and has additional files below the directories) then only the new files should be copied to their appropriate locations.

I did do a test of this, but because I'm manually installing files into a sensitive area I don't want to "clobber" anyone's system and leave it in an irreparable state. The --no-clobber option might be sufficient, but I'm unsure.

For context, the goal is to merge the files/directories in SLiM/data with /usr/local/share.

Regards and thanks.

grahamgower commented 4 months ago

I didn't read through the thread, but it sounds like cp --no-clobber should work. I haven't used this before, but I did a quick test and it seems to work ok. I also tried cp --update=none which does the same thing without any messages (e.g. cp --no-clobber says cp: not replacing b/A, but cp --update=none has no output). If you're looking for an alternate command, maybe some incantation with rsync will do what you want too?

grahamgower commented 4 months ago

Another thought: --no-clobber is probably a GNU extension because I don't see it listed at https://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html. So you might want to satisfy yourself that the --no-clobber feature will always be available on the systems where this script will run. E.g. maybe busybox cp doesn't support this?

bryce-carson commented 4 months ago

Hi @bryce-carson. I'm not sure that I understand all the ins and outs here, sorry. But. Right now the overall CMakeLists.txt requires 2.8.12 overall, and requires 3.1.0 if and only if SLiMgui is being built (inside if(BUILD_SLIMGUI)). Can we bump that 3.1.0 requirement to 3.14, for SLiMgui, and leave the overall 2.8.12 requirement alone? What I want to avoid is breaking a whole bunch of people who build slim, but not SLiMgui, on their computing cluster that has an archaic software stack (as many/most clusters seem to have). As I understand it, this version requirement is only involved with installing SLiMgui into the desktop environment, correct? In fact, can the higher version requirement be imposed only for a make install of SLiMgui, where 3.14 is in fact needed? I'm aiming to break as few people as possible with this change.

And then there is the part of your comment above where you say:

The shell script for Debian and Ubuntu, since it requires root and builds the software anyways, may as well build a binary package and install that: it'd allow users to uninstall the software cleanly, and provide for better quality control by allowing previous versions to be replaced in a more structured way than simply replacing the binaries.

Is this related to this issue, CMake versioning, etc., or is this a separate, orthogonal proposal? It sounds like an improvement, but I wonder what the downsides might be...? Will it be more fragile, or harder to maintain, or work on fewer platforms, or anything like that? And is anything strongly driving the necessity of it, or is it just an optional improvement that you thought of?

I'm in Europe for the next two months, teaching SLiM workshops and such, and it is difficult to deal with fallout from issues over here (my build machines are back in NY, so dealing with new releases requires remote access), and it is crucial not to break anything until the workshops are done – if installation gets screwed up just before a workshop, I end up with a whole workshop full of people who can't install the software. So until I'm back in the U.S., I would like to keep changes to the minimum necessary. So if this idea is indeed separate/orthogonal, how about we open a new issue for it, and handle that later when the timing is more favorable?

Hi! Yep, it was orthogonal, but I'm abandoning the idea. Eventually a Debian junkie will adopt what I've done in the debian/ folder, but until then I think the manual shell scripts current incarnation is the best route for those systems.

I'll update you on the plan tomorrow, after work, but I'm not suggesting a huge bump in the script now. I have limited the requirement to exactly the line that uses it, and otherwise nothing is changed about CMake.

Further, the change only exists in a patch to the CMake file embedded inside the upgraded version of the Debian and Ubuntu install script. I'm working on the final step of the upgrade and doing tests, but it should be a favourable improvement to the script which fixes this issue and does nothing more.

bhaller commented 4 months ago

Sounds great @bryce-carson, thanks!

bryce-carson commented 4 months ago

I didn't read through the thread, but it sounds like cp --no-clobber should work. I haven't used this before, but I did a quick test and it seems to work ok. I also tried cp --update=none which does the same thing without any messages (e.g. cp --no-clobber says cp: not replacing b/A, but cp --update=none has no output). If you're looking for an alternate command, maybe some incantation with rsync will do what you want too?

Thanks. rsynx was suggested in the SO thread I read, but it isn't installed by default on Ubuntu 18, and I imagine isn't installed by default on older Debian systems either.

I'll do more testing with cp, but there's only five files to install, so just using install with appropriate options may be more prudent than trying to abstract too much. "Move A to B, no more, no less." KISS, right?

bryce-carson commented 4 months ago

Sounds great @bryce-carson, thanks!

The conditional use of the syntax I'm currently using will be in a PR, however, as other systems may need protection. Right now it is unprotected; the PR will fix this issue when merged alongside a PR for SLiM-Extras.

2 PRs, 1 fix. 😀

bryce-carson commented 3 months ago

@bhaller We're done with this issue, right? I'm not sure myself.

We've merged two PRs, and closed another issue, and that's what I said previously.

The conditional use of the syntax I'm currently using will be in a PR, however, as other systems may need protection. Right now it is unprotected; the PR will fix this issue when merged alongside a PR for SLiM-Extras.

2 PRs, 1 fix. 😀

bhaller commented 3 months ago

I think so. Closing.