-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
CPack: Refactor AppImage and Apple DMG Generation #7252
base: master
Are you sure you want to change the base?
Conversation
I was able to get the CPack DMG generator to work with Edit: For simplicity, I've checked in the |
Hmm... the AppImage is missing... investigating... |
The AppImage is missing because the CPack feature for running custom scripts through variable |
Perhaps I have a hint for this: I use Ubuntu 20.04 that comes with cmake version 3.16 BUT Qt (5.15.2 installed from Qt site) comes with cmake 3.27 in its tools directory. This is the one I use. |
Thanks! Any recommendation for getting this into a CI (command line?) |
Not sure of what I will suggest below since I build using Qt Creator(so all is set automagically) but there are environment variables related to cmake:
|
FYI, CMake homepage lists the official PPA and pip package as alternative installation methods. |
@tresf I could update the Linux image with a newer CMake using the official PPA if you want. That might be easiest and also won't impact build times |
If parties agree on this CPack strategy, yes please (or from snap). Tested on Ubuntu 20.04: sudo snap install cmake --classic
alias cmake='snap run cmake'
# cmake --version
# cmake version 3.29.3
#
# CMake suite maintained and supported by Kitware (kitware.com/cmake). ... I had reservations about whether or not the snap version of cmake would play nicely... and it seems to work just fine. |
can we use the PPA for mingw ci image too? |
I'll try to do it sometime today
Yes, I think so, though I don't think it will be needed for this PR |
Actually, we can justify it here since bad79a8. |
I've marked this ready for review because I think it's in pretty good shape. (artifacts should start generating once cmake is updated). ARM64.AppImage.mp4 |
- Refactor BashCompletion.cmake - Add initial support for Linux AppImages (untested) - Add debug flags for cpack
Testing on Discord shows the following error, investigating... - ./lmms-1.3.0.run: /home/ubuntu/lmms2/build/cmake/linux/setup.sh: inaccessible or not found |
Done via ddca4e7. |
Due to unreliability with FUSE (availability, compatibility), I'm changing this PR to always extracting AppImages before running them. Quoting
At a glance, this may be a bit confusing, because the build system will create a symlink with the AppImage name. e.g. |
On Linux, Carla Rack and Carla Patchbay crash when I try to use them, and this is not the case on master EDIT: Here's the command-line output:
|
if(BASHCOMP_PKG_PATH) | ||
# TODO: CMake 3.21 Use "file(COPY_FILE ...)" | ||
install(CODE " | ||
execute_process(COMMAND ${CMAKE_COMMAND} -E copy \"${SCRIPT_NAME}\" \"${BASHCOMP_PKG_PATH}\" ERROR_QUIET RESULT_VARIABLE result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it install SCRIPT_NAME
twice, once in line 47 and once in line 51?
Also, where is the BASHCOMP_PKG_PATH
variable from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it install
SCRIPT_NAME
twice, once in line 47 and once in line 51?
It's due to cmake's lack of conditional INSTALL(...)
logic. For some reason, I removed the comment that describes this. It's here: 00d4ddd#diff-7960a523ea4d93951917fb0445e1145899f31c5a796e123ee06c805095e37221R59-R71.
In short:
- Always install to user-space
- Attempt to install system-wide, but don't fail the build if it doesn't work.
We did have some talks about this "system wide" install previously because this is the first time that we've had a component to install in a prefix that's outside of our own install prefix. I suppose we could instead detect a system-wide prefix and handle this in a more standard fashion. If that's the case, we can fix it as part of this PR if needed. I know some environments (such as Apple's Homebrew) ARE user-writable, so perhaps that was my motivation for this but that does NOT make sense to try when building a DMG or AppImage, so this logic is likely flawed.
Furthermore, I think there's a typo in the status message though.... I think this should say BASHCOMP_PKG_PATH
not BASHCOMP_USER_PATH
.
- message(STATUS "Unable to install bash-completion support system-wide: ${BASHCOMP_USER_PATH}/${SCRIPT_NAME})
+ - message(STATUS "Unable to install bash-completion support system-wide: ${BASHCOMP_PKG_PATH}/${SCRIPT_NAME})
Also, where is the
BASHCOMP_PKG_PATH
variable from?
It's from #4604. This is not a newly introduced variable with this PR. I believe its intended to be the completionsdir
component, found by pkg-config. My memory is foggy from back then and the conversations seem to have largely occurred on Discord so decisions are not all obvious. Anyway, we also re-use this variable name for cmake's find_package(bash-completion)
portion as well. It's probably a bad name, but I do not plan on changing it as part of this PR but can if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe its intended to be the completionsdir component, found by pkg-config.
Indeed! The top of the file actually explains it. You can even set this variable using -DBASHCOMP_PKG_PATH=...
.
Furthermore, I think there's a typo in the status message though.... I think this should say BASHCOMP_PKG_PATH not BASHCOMP_USER_PATH.
Agreeing. Please feel free to fix.
Attempt to install system-wide, but don't fail the build if it doesn't work.
IMO, the natural behavior would be: If the user does a local install, install bashcomp locally. If they do a system install ("root"), install it system-wide. I assume that back then, we thought: "An install should always try to be system-wide, because local installed bashcomp is not sourced by your bashrc". However, you could give the same argument for the PATH
variable (a local install of bin/lmms
will never be in your PATH
by default). In both cases, you can extend your bashrc to source a local bashcomp or to extend your PATH
to a local directory. All in all, I would favor to do the install exactly one, and always in the normal install tree. However, it is just my basic opinion, and also it might be considered out of scope of this PR. Tagging @PhysSong because they were also active in that bashcomp PR in 2018.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. I'll put some time into fixing this.
Thanks! Should be fixed via aab4cc5. |
Nevermind, still crashes. Investigating... |
@tresf I'm not sure why there are so many more, but of those extra files, there were |
@messmerd thank you. I'm fully aware of the cause of the crash. lmms/cmake/linux/LinuxDeploy.cmake Lines 244 to 250 in 22f17e0
There's a new |
Apologies for the whitespace changes in the latest commit 508f430, but spaces and tabs were inconsistent. The relevant fix is the addition of Another big miss was never providing
I haven't done a comparison yet, but one thing that I haven't tested the AppImage yet but I believe the Carla loading issue is at least resolved. I'm curious what the directory tree compare looks like. I used to use a commercial tool called "Beyond Compare" for comparing file trees, perhaps there's a FOSS equivalent. |
I've confirmed that Carla no longer crashes. However, it looks like there are copies of all the LMMS plugins and some other .so files in both As a side note, I just realized xpressive is a whopping 71 MB. It's also always last to finish compiling in our CI builds, so maybe something should be done about that. (Though not in this PR of course) |
Although undocumented, it appears |
Features:
make package
WANT_DEBUG_CPACK
flag to easily show detailed console messages about packagingDeveloper options ----------------------------------------- * Debug FP exceptions : Disabled * Debug using AddressSanitizer : Disabled * Debug using ThreadSanitizer : Disabled * Debug using MemorySanitizer : Disabled * Debug using UBSanitizer : Disabled + * Debug packaging commands : Enabled
TGZ
behavior through a hidden flag,WANT_CPACK_TARBALL
. Does anyone use this?CPACK_TOOL
parameter to switch between.AppImage
(default) and.run
(experimental)TODO:
install_apple.sh
script to CMake.It's called
MacDeployQt.cmake
now.package_linux.sh
script to CmakeIt's called
LinuxDeploy.cmake
now.BUGS:
WANT_DEBUG_CPACK
is incorrectly toggled in CI. https://github.com/LMMS/lmms/actions/runs/9131345271/job/25110338288. Closed via 82a3aa6.STRETCH GOALS:
linuxdeployqt
tolinuxdeploy
Examples:
linuxdeployqt
(in favor oflinuxdeploy
)[ ] Add ARM64 AppImageBackground:
Click to expand
make install
to be run prior to making packages. This PR is a proposal to remove that by switching toCPack
.make package
now, just like Windows doesCPACK_GENERATOR
ofDragNDrop
Bundle
which will create a DMG without the need for node'sappdmg
package (still requiresmacdeployqt
)CPACK_GENERATOR
of "External", which will use custom CMake commands to replacepackage_linux.sh
(still requireslinuxdeployqt
).