Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for macOS app bundles for GUI apps #2739

Merged
merged 16 commits into from
Jun 7, 2024
Merged

Conversation

bjeurissen
Copy link
Member

This can absorb some of the logic around this currently found in https://github.com/MRtrix3/macos-installer/ and/or https://github.com/MRtrix3/mrtrix3/tree/app_bundles

@bjeurissen
Copy link
Member Author

In the current binary installer for macos, we add wrapper scripts in the bin folder that point to the GUI apps (mrview and shview) inside their app bundle folders, e.g. https://github.com/MRtrix3/macos-installer/blob/master/mrview, to ensure seamless operation of mrview and shview from both the macOS command-line and macOS GUI.

We probably have to do something similar in CMake or come up with a more elegant solution.

cmd/CMakeLists.txt Outdated Show resolved Hide resolved
cmd/CMakeLists.txt Outdated Show resolved Hide resolved
@bjeurissen
Copy link
Member Author

bjeurissen commented Oct 26, 2023

I have cleaned up the generation of and completed the contents of the plist files by using custom Info.plist template support provided by CMake (this is the only way one can generate plist files for app bundles to liking).

What is currently still missing is:

  • copying the icons in icons/macos into the appropriate Resource folders of the application bundles
  • copying the wrappers that point mrview to mrview.app/Contents/MacOS/mrview (e.g. https://github.com/MRtrix3/macos-installer/blob/master/mrview). This can't be a simple symlink, but needs to be a wrapper script, otherwise CLI and GUI integration breaks. Note that this should also fix the current CI failure when building the docs for mrview and shview (which breaks since these files are not present when building as app bundles).

I don't know CMake well enough to do this in the appropriate way...

@daljit46
Copy link
Member

Unfortunately, CMake's documentation is rather lacking on this front. For GUI use cases, I always recommend checking Qt's documentation, which is often excellent.

For setting the icon, something like this should work:

    set(mrtrix_icon_macos "mrtrix.icns")
    set_source_files_properties(${mrtrix_icon_macos} PROPERTIES 
        MACOSX_PACKAGE_LOCATION "Resources"
    )
    target_sources(mrview PRIVATE ${mrtrix_icon_macos})

    set_target_properties(mrview PROPERTIES
        MACOSX_BUNDLE_ICON_FILE "mrtrix.icns"
    )

@bjeurissen
Copy link
Member Author

Noticed that while the app bundles work fine in the build directory, they break when installed to the installation directory. Cmake correctly replaces the absolute library paths to relative ones on installation, but appears to apply the exact same logic to both the regular binaries and the ones in a bundle.

As a result, in the current form we need to run:

install_name_tool -rpath @loader_path/../lib @loader_path/../../../../lib ~/mrtrix3_installation/bin/mrview.app/Contents/MacOS/mrview
install_name_tool -rpath @loader_path/../lib @loader_path/../../../../lib ~/mrtrix3_installation/bin/shview.app/Contents/MacOS/shview

Copy link

clang-tidy review says "All clean, LGTM! 👍"

@daljit46
Copy link
Member

OK, so I have had a look at this in more detail. I think it'd be good to move cmake/bundle to packaging/macos/bundle. Also, to handle the logic in cmd/CMakeLists.txt, it'd be sensible to create a new CMake module cmake/MacOSBundle.cmake which provides some helper functions to achieve what we need.

To solve the issue with the RPATH for the installation directory, it should be sufficient to set the INSTALL_RPATH property to something like @executable_path/../../../../lib instead of relying on the global CMAKE_INSTALL_RPATH which is by default set to @loader_path/../lib.

Copy link

clang-tidy review says "All clean, LGTM! 👍"

@bjeurissen
Copy link
Member Author

Currently nothing seems to trigger the creation of the app bundle (at least not on my machine).

Copy link

github-actions bot commented Jun 3, 2024

clang-tidy review says "All clean, LGTM! 👍"

Copy link

github-actions bot commented Jun 3, 2024

clang-tidy review says "All clean, LGTM! 👍"

@daljit46
Copy link
Member

daljit46 commented Jun 3, 2024

Currently nothing seems to trigger the creation of the app bundle (at least not on my machine).

It should work now.

@bjeurissen
Copy link
Member Author

bjeurissen commented Jun 3, 2024

Currently nothing seems to trigger the creation of the app bundle (at least not on my machine).

It should work now.

It does! Thanks!

A few outstanding things that need to be fixed:

  • <key>LSMinimumSystemVersion</key> <string>${CMAKE_OSX_DEPLOYMENT_TARGET}</string> does not seem to be working, we need to either set this variable or use an alternative.
  • copying the wrappers that point mrview to mrview.app/Contents/MacOS/mrview (e.g. https://github.com/MRtrix3/macos-installer/blob/master/mrview). This can't be a simple symlink, but needs to be a wrapper script, otherwise CLI and GUI integration breaks.
  • When the app bundles are used, the CI binary tests for mrview and shview are skipped.
  • Fix inclusion of app icon for mrview

Copy link

github-actions bot commented Jun 4, 2024

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
Copy link

github-actions bot commented Jun 4, 2024

clang-tidy review says "All clean, LGTM! 👍"

Copy link

github-actions bot commented Jun 4, 2024

clang-tidy review says "All clean, LGTM! 👍"

@daljit46
Copy link
Member

daljit46 commented Jun 5, 2024

When the app bundles are used, the CI binary tests for mrview and shview are skipped.

What do you mean by this? We don't have any binary tests for those commands.

@bjeurissen
Copy link
Member Author

When the app bundles are used, the CI binary tests for mrview and shview are skipped.

What do you mean by this? We don't have any binary tests for those commands.

Never mind. It was the docs generation that was failing in the CI before when the mrview/shview wrappers were not present, not the binary tests (which are indeed not present).

@bjeurissen
Copy link
Member Author

One more thing about the CLI-wrappers: they are currently only added in the "installation" phase. Is it possible to already do this in the "build" stage? So that a developer can use the build without installing?

@daljit46
Copy link
Member

daljit46 commented Jun 6, 2024

One more thing about the CLI-wrappers: they are currently only added in the "installation" phase. Is it possible to already do this in the "build" stage? So that a developer can use the build without installing?

Done.

@MRtrix3 MRtrix3 deleted a comment from github-actions bot Jun 6, 2024
@MRtrix3 MRtrix3 deleted a comment from github-actions bot Jun 6, 2024
@MRtrix3 MRtrix3 deleted a comment from github-actions bot Jun 6, 2024
@bjeurissen bjeurissen changed the title Initial support for macOS app bundles for GUI apps Support for macOS app bundles for GUI apps Jun 7, 2024
@bjeurissen bjeurissen merged commit 399cf16 into dev Jun 7, 2024
6 checks passed
@bjeurissen bjeurissen deleted the cmake_app_bundles branch June 7, 2024 11:55
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.

2 participants