Skip to content

Commit

Permalink
Don't attempt installing pixmaps and launchers on macOS -> breaks git…
Browse files Browse the repository at this point in the history 
…hub test runs etc

Also, I think the best would be to honour CMAKE_INSTALL_PREFIX
${CMAKE_INSTALL_PREFIX}/share/pixmaps ought to give /usr/share/pixmaps on Debian
  • Loading branch information
@willend
willend committed Jul 19, 2024
1 parent 728fd5e commit 31e70b0
Showing 1 changed file with 6 additions and 5 deletions.
11 changes: 6 additions & 5 deletions tools/Python/mcgui/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,12 @@ if(NOT WINDOWS)
PROGRAMS "${WORK}/${P}gui"
DESTINATION ${DEST_BINDIR}
)

# Install desktop entry
install(FILES "${WORK}/${FLAVOR}-${MCCODE_VERSION}-py.desktop" DESTINATION /usr/share/applications )

# Install icon
install(FILES "${FLAVOR}-py.png" DESTINATION /usr/share/pixmaps/)
if (NOT DARWIN)
# Install desktop entry
install(FILES "${WORK}/${FLAVOR}-${MCCODE_VERSION}-py.desktop" DESTINATION /usr/share/applications )

# Install icon
install(FILES "${FLAVOR}-py.png" DESTINATION /usr/share/pixmaps/)
endif()
endif()

3 comments on commit 31e70b0

@willend
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@farhi the previous install destination broke GitHub runners for macOS, see e.g. https://github.com/McStasMcXtrace/McCode/actions/runs/10009267813

For now I have added if (NOT DARWIN), but I further think we ought to use something like ${CMAKE_INSTALL_PREFIX}/share/pixmaps instead of a hardcoded path to /usr

@willend
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@farhi I have implemented ${CMAKE_INSTALL_PREFIX} now since the above, half-baked solution did not work anyway. :)

(Note to self, I really should enable PR-only commits to main)

@farhi
Copy link
Contributor

@farhi farhi commented on 31e70b0 Jul 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Peter,

Thanks for the fix.
However the new target

  • {CMAKE_INSTALL_PREFIX}/share/applications

boils down to something like:

  • /usr/share/share/applications

for the desktop launcher, which is not the expected /usr.share/applications/.

Regarding commits, in the end, if we converge on a working solution, as we always did, then all is fine in my view. I'll push a fix, via a PR ;-)

Please sign in to comment.