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.

Sign up for GitHub

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

Replace libappimage submodule with FetchContent #1190

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Replace libappimage submodule with FetchContent #1190

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1c9ab2
Remove libappimage git submodule
lalten Apr 3, 2022
2fc2a91
Update minimum CMake and libappimage pinning
lalten Aug 18, 2022
5d7f314
Update lib and patch to latest libappimage
lalten Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions cmake/dependencies.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@ include(FetchContent)

# Need this patch until https://github.com/AppImage/libappimage/pull/160 is resolved
FetchContent_Declare(libappimage_patch
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can use an older version of libappimage for now?

URL https://github.com/AppImage/libappimage/commit/b3398bb496e47947864b4b8bc2999c8427f86a9a.patch
URL https://github.com/AppImage/libappimage/commit/ce0a186a5a3cd8f31f4afd216d5322410a0a8e26.patch
DOWNLOAD_NO_EXTRACT TRUE
)
FetchContent_MakeAvailable(libappimage_patch)
TheAssassin marked this conversation as resolved.
Show resolved Hide resolved

FetchContent_Declare(libappimage
# We can not use a URL source with a github-generated source archive: libappimage's gtest submodule would be missing
Copy link
Member

Choose a reason for hiding this comment

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

Eventually we may just want to use gtest from the system (generally provided externally), these submodules are more than annoying anyway.

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

Choose a reason for hiding this comment

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

--forward is not POSIX compliant, you may want to use -N

Copy link
Member

@TheAssassin TheAssassin Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

)
FetchContent_MakeAvailable(libappimage)
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${libappimage_SOURCE_DIR}/cmake)
Expand Down