Skip to content

Conversation

ZhongRuoyu
Copy link
Contributor

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
Copy link
Owner

patriciogonzalezvivo commented Dec 13, 2022

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Owner

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

@patriciogonzalezvivo
Copy link
Owner

patriciogonzalezvivo commented Dec 14, 2022

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

This allows the user to specify alternative directories when
`/usr/share` is not a suitable option.

Also, remove `sudo` invocations. It should be more preferable to let the
user do `sudo cmake --install <build-directory>` instead, if needed.
This is safer, and allows users without root privileges to build and
install to custom locations.

See:
- https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html
- https://www.gnu.org/prep/standards/html_node/Directory-Variables.html

Signed-off-by: Ruoyu Zhong <zhongruoyu@outlook.com>
@ZhongRuoyu
Copy link
Contributor Author

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 patriciogonzalezvivo merged commit aee2346 into patriciogonzalezvivo:main Dec 14, 2022
@patriciogonzalezvivo
Copy link
Owner

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
Copy link
Contributor Author

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

@ZhongRuoyu ZhongRuoyu deleted the cmake-install branch December 14, 2022 20:16
@patriciogonzalezvivo
Copy link
Owner

Sweet! I'm making the tag and release soon

@patriciogonzalezvivo
Copy link
Owner

@ZhongRuoyu
Copy link
Contributor Author

Thanks, @patriciogonzalezvivo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants