Replace libappimage submodule with FetchContent #1190
Conversation
| @@ -53,7 +53,7 @@ | |||
| #include <limits.h> | |||
| #include <stdbool.h> | |||
|
|
|||
| #include "appimage/appimage.h" | |||
| #include <appimage/appimage_shared.h> | |||
There was a problem hiding this comment.
Depending on appimage.h would introduce a dependency on the generated config.h. However it seems that including appimage_shared.h is enough to fix the build and make the tests pass. Same for validate.c.
There was a problem hiding this comment.
I agree. validate.c may be removed soon anyway, I recently published a replacement (which actually works even) built from AppImageUpdate's code base. https://github.com/AppImage/AppImageUpdate/releases
|
Will have a look later, thanks. |
|
I guess this will also fix #1185 |
TheAssassin
force-pushed
the
remove-git-submodule
branch
from
8749879
to
9e377dc
Compare
There was a problem hiding this comment.
Sorry for the late review. Looks good so far. Please see @Tachi107's comment, we need to bump the minimum required CMake version.
TheAssassin
changed the title
The submodule is replaced with CMake FetchContent commands, which fetch the source at configure time (We need to include scripts.cmake from libappimage. ExternalProject_Add only downloads at build time). Also update libappimage version to latest (otherwise the build fails on g++11) and patch in AppImageCommunity/libappimage#160 (to allow using latest libappimage in AppImageKit. Also update squashfs-tools version to latest release (otherwise the build fails on g++11) Also related to AppImage#1165
lalten
force-pushed
the
remove-git-submodule
branch
from
9e377dc
to
2fc2a91
Compare
|
Updated to rebase your earlier force-push 8749879 to latest master and added a separate commit for the new changes |
| FetchContent_MakeAvailable(libappimage_patch) | ||
|
|
||
| FetchContent_Declare(libappimage | ||
| # We can not use a URL source with a github-generated source archive: libappimage's gtest submodule would be missing |
There was a problem hiding this comment.
Eventually we may just want to use gtest from the system (generally provided externally), these submodules are more than annoying anyway.
cmake/dependencies.cmake
Outdated
| FetchContent_Declare(libappimage | ||
| # We can not use a URL source with a github-generated source archive: libappimage's gtest submodule would be missing | ||
| GIT_REPOSITORY https://github.com/AppImage/libappimage | ||
| GIT_TAG aa7d9fb03d3d64415c37120f20faa05412458e94 # latest as of 2022-08-18 |
There was a problem hiding this comment.
Eventually we may want to use master, once that works reliably.
cmake/dependencies.cmake
Outdated
| GIT_REPOSITORY https://github.com/AppImage/libappimage | ||
| GIT_TAG aa7d9fb03d3d64415c37120f20faa05412458e94 # latest as of 2022-08-18 | ||
| # The patch command has || true to prevent the build from failing if the patch has already been applied | ||
| PATCH_COMMAND patch -p 1 < ${libappimage_patch_SOURCE_DIR}/b3398bb496e47947864b4b8bc2999c8427f86a9a.patch || true |
There was a problem hiding this comment.
This will let unrelated errors slip. How about the -N flag? It should work with this patch.
There was a problem hiding this comment.
Good point! I added --forward and it immediately failed ;P I guess that patch file doesn't actually apply cleanly to latest. It also doesn't work with the currently pinned version on master. It does work with https://github.com/AppImage/libappimage/commits/1d4d57622de2c7d39f7cc6c4980144c713cc59ca, which was the latest at the time of writing. I'm guessing that PR the patch is coming from needs to be rebased.
| include(FetchContent) | ||
|
|
||
| # Need this patch until https://github.com/AppImage/libappimage/pull/160 is resolved | ||
| FetchContent_Declare(libappimage_patch |
There was a problem hiding this comment.
Maybe we can use an older version of libappimage for now?
| # If you update the GIT_TAG and the patch does not apply anymore you need to rebase libappimage_patch (see above) | ||
| GIT_REPOSITORY https://github.com/AppImage/libappimage | ||
| GIT_TAG aa7d9fb03d3d64415c37120f20faa05412458e94 # Eventually we may want to use master, once that works reliably. | ||
| PATCH_COMMAND patch -p 1 --forward < ${libappimage_patch_SOURCE_DIR}/ce0a186a5a3cd8f31f4afd216d5322410a0a8e26.patch |
There was a problem hiding this comment.
--forward is not POSIX compliant, you may want to use -N
There was a problem hiding this comment.
POSIX compliance is not really relevant for this repository, we use glibc by default. I also think this is no longer necessary, we can just use patch -p1.
The submodule is replaced with CMake FetchContent commands, which fetch the source at configure time (We need to include scripts.cmake from libappimage. ExternalProject_Add only downloads at build time).
Also update libappimage version to latest (otherwise the build fails on g++11) and patch in AppImageCommunity/libappimage#160 (to allow using latest libappimage in AppImageKit.
Also update squashfs-tools version to latest release (otherwise the build fails on g++11)
Also related to #1165