patriciogonzalezvivo / glslViewer

Console-based GLSL Sandbox for 2D/3D shaders
BSD 3-Clause "New" or "Revised" License
4.57k stars 352 forks source link

CMakeLists.txt: Avoid hardcoding installation dirs #315

Closed ZhongRuoyu closed 1 year ago

ZhongRuoyu commented 1 year ago

Hi! Submitting this to fix build in Homebrew/homebrew-core#118048 (log). Homebrew's test bot won't allow writes to system directories like /usr/bin. That could be resolved if installation directories are customizable, which is usually the case.

For details, please see the commit message. Thanks!

Signed-off-by: Ruoyu Zhong zhongruoyu@outlook.com

patriciogonzalezvivo commented 1 year ago

Hi @ZhongRuoyu, thanks for brining this up. This seams to change most of linux-only paths... which (because of how I'm integrating to windows managers needs to be fixed). Wondering if instead we can do this changes only for Apple. Something like:

        if (APPLE)
            include(GNUInstallDirs)
            install(TARGETS glslViewer DESTINATION ${CMAKE_INSTALL_BINDIR})
        else
            install(TARGETS glslViewer DESTINATION bin)
            target_link_libraries(glslViewer PRIVATE atomic)
            install(FILES "${PROJECT_SOURCE_DIR}/assets/glslViewer.png" DESTINATION /usr/share/pixmaps)
            install(FILES "${PROJECT_SOURCE_DIR}/assets/glslViewer.desktop" DESTINATION /usr/share/applications)
            ...
ZhongRuoyu commented 1 year ago

Hi @patriciogonzalezvivo, thanks for your attention. However, the build actually failed on Linux (Homebrew not only supports macOS), so I'm afraid these changes are necessary.

Build failure aside: I'm not familiar with how software should be installed on Windows, but normally on Unix systems, software installation isn't done this way. Firstly, not all users want, or have enough privilege, to install to /usr/share, /usr/bin, etc. And also, you probably want to prefer /usr/local/share over /usr/share, because on Linux, /usr is usually managed by the system package manager. That's also why CMAKE_INSTALL_PREFIX defaults to /usr/local. (And that also means, if you don't explicitly change the prefix, glslViewer will still be found under /usr/local/bin -- so there's some inconsistency here).

If you still want to preserve the original behaviour, you may set CMAKE_INSTALL_PREFIX to /usr. Then all assets will still be installed to /usr/share, and glslViewer will be installed to /usr/bin as well.

I hope that clarifies!

carlocab commented 1 year ago

Hard-coding install locations is not a nice thing to do, and negates one of the significant benefits of using a cross-platform build system like CMake.

If you'd like to retain the existing behaviour of installing into /usr by default, you can do something like

option(CMAKE_INSTALL_PREFIX "Location to install" "/usr")

However, it is important to respect user configuration of CMAKE_INSTALL_PREFIX, as many (all?) users who set this will find it quite surprising to have CMake install something elsewhere despite their having set it (to, e.g., their HOME directory).

patriciogonzalezvivo commented 1 year ago

Good points, thank you @ZhongRuoyu and @carlocab. I will test it today locally, solve the .desktop and .thumbnail dependencies

patriciogonzalezvivo commented 1 year ago

So here is the pickle, there is no `/usr/local/share/mime/package/ folder'. It would be ok, hard_coding only this path?

Install the project...
-- Install configuration: ""
-- Up-to-date: /usr/local/bin/glslViewer
-- Installing: /usr/local/share/pixmaps/glslViewer.png
-- Installing: /usr/local/share/applications/glslViewer.desktop
-- Installing: /usr/local/share/mime/packages/glslViewer-types.xml
xdg-mime: file 'share/mime/packages/glslViewer-types.xml' does not exist
Directory 'share/mime/packages' does not exist!
The databases in [share/applications] could not be updated.
-- Installing: /usr/local/share/thumbnailers/glslViewer.thumbnailer
-- Installing: /usr/local/bin/glslThumbnailer
-- Installing: /usr/local/bin/glslScreenSaver
-- Installing: /usr/local/share/glslViewer/glslScreenSaver.frag
-- Installing: /usr/local/share/glslViewer/glslScreenSaver.yaml
ZhongRuoyu commented 1 year ago

So here is the pickle, there is no `/usr/local/share/mime/package/ folder'.

Sorry, I must have missed that, since xdg-mime didn't fail the installation.

It would be ok, hard_coding only this path?

Actually, we can use the CMAKE_INSTALL_FULL_<dir> variants. Can you try again with the new patch that I just pushed?

patriciogonzalezvivo commented 1 year ago

Thank you so much @ZhongRuoyu ! that worked perfectly! I added a couple of lines to solve the issues finding GlslViewer png icon through scripting the it on the cmake file. If you can double check that this is ok with brew I will make a new tag for this bug fix. Thank you again

ZhongRuoyu commented 1 year ago

I've tested this locally, and I think it's good to go. Thanks, @patriciogonzalezvivo!

patriciogonzalezvivo commented 1 year ago

Sweet! I'm making the tag and release soon

patriciogonzalezvivo commented 1 year ago

https://github.com/patriciogonzalezvivo/glslViewer/releases/tag/v3.1.1

ZhongRuoyu commented 1 year ago

Thanks, @patriciogonzalezvivo !