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

Show file tree
Hide file tree
Changes from 2 commits
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
3 changes: 0 additions & 3 deletions .gitmodules
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
[submodule "cmake/sanitizers-cmake"]
path = cmake/sanitizers-cmake
url = https://github.com/arsenm/sanitizers-cmake
[submodule "lib/libappimage"]
path = lib/libappimage
url = https://github.com/AppImage/libappimage.git
5 changes: 0 additions & 5 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,6 @@ set(CMAKE_POSITION_INDEPENDENT_CODE ON)
##########################

set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${PROJECT_SOURCE_DIR}/cmake)

# configure dependencies
add_subdirectory(lib)

include(lib/libappimage/cmake/tools.cmake)
include(cmake/dependencies.cmake)


Expand Down
31 changes: 25 additions & 6 deletions cmake/dependencies.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,26 @@
# >= 3.2 required for ExternalProject_Add_StepDependencies
cmake_minimum_required(VERSION 3.2)


include(${PROJECT_SOURCE_DIR}/lib/libappimage/cmake/scripts.cmake)
# FetchContent_MakeAvailable() is only available in CMake 3.14 or newer
cmake_minimum_required(VERSION 3.14)

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
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
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 want to use master, once that works reliably.

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

Choose a reason for hiding this comment

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

This will let unrelated errors slip. How about the -N flag? It should work with this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

)
FetchContent_MakeAvailable(libappimage)
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${libappimage_SOURCE_DIR}/cmake)
include(${libappimage_SOURCE_DIR}/cmake/scripts.cmake)
include(${libappimage_SOURCE_DIR}/cmake/tools.cmake)


# the names of the targets need to differ from the library filenames
Expand Down Expand Up @@ -53,9 +71,10 @@ if(NOT USE_SYSTEM_MKSQUASHFS)

ExternalProject_Add(mksquashfs
GIT_REPOSITORY https://github.com/plougher/squashfs-tools/
GIT_TAG 4.4
GIT_TAG 4.5.1
TheAssassin marked this conversation as resolved.
Show resolved Hide resolved
UPDATE_COMMAND "" # Make sure CMake won't try to fetch updates unnecessarily and hence rebuild the dependency every time
CONFIGURE_COMMAND ${SED} -i "s|CFLAGS += -DXZ_SUPPORT|CFLAGS += ${mksquashfs_cflags}|g" <SOURCE_DIR>/squashfs-tools/Makefile
COMMAND ${SED} -i "/INSTALL_MANPAGES_DIR/d" <SOURCE_DIR>/squashfs-tools/Makefile
TheAssassin marked this conversation as resolved.
Show resolved Hide resolved
COMMAND ${SED} -i "s|LIBS += -llzma|LIBS += -Bstatic ${mksquashfs_ldflags}|g" <SOURCE_DIR>/squashfs-tools/Makefile
COMMAND ${SED} -i "s|install: mksquashfs unsquashfs|install: mksquashfs|g" squashfs-tools/Makefile
COMMAND ${SED} -i "/cp unsquashfs/d" squashfs-tools/Makefile
Expand Down
1 change: 0 additions & 1 deletion lib/CMakeLists.txt
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion lib/libappimage
Open in desktop
Submodule libappimage deleted from 9c1fae
4 changes: 0 additions & 4 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@ target_compile_definitions(appimagetool
PRIVATE -DENABLE_BINRELOC
)

target_include_directories(appimagetool
TheAssassin marked this conversation as resolved.
Show resolved Hide resolved
PUBLIC $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include/>
INTERFACE $<INSTALL_INTERFACE:include/>
)

if(AUXILIARY_FILES_DESTINATION)
message(STATUS "Installing auxiliary files in path: ${AUXILIARY_FILES_DESTINATION}")
Expand Down
6 changes: 3 additions & 3 deletions src/appimagetool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@
#include <sys/stat.h>
#include <sys/wait.h>

#include "binreloc.h"

#include <libgen.h>

#include <unistd.h>
Expand All @@ -56,7 +54,9 @@
#include <gpgme.h>
#include <assert.h>

#include "appimage/appimage.h"
#include <appimage/appimage_shared.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


#include "binreloc.h"
#include "appimagetool_sign.h"

#ifdef __linux__
Expand Down
4 changes: 2 additions & 2 deletions src/build-runtime.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ set(runtime_cflags
-DGIT_COMMIT=\\"${GIT_COMMIT}\\"
-I${squashfuse_INCLUDE_DIRS}
-I${PROJECT_SOURCE_DIR}/include
-I${PROJECT_SOURCE_DIR}/lib/libappimage/include
-I${PROJECT_SOURCE_DIR}/lib/libappimage/src/libappimage_hashlib/include
-I${libappimage_SOURCE_DIR}/include
-I${libappimage_SOURCE_DIR}/src/libappimage_hashlib/include
${DEPENDENCIES_CFLAGS}
)
# must not include -Wl,--gc-sections in the following flags, otherwise the data sections will be stripped out
Expand Down
4 changes: 2 additions & 2 deletions src/validate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
#include <fcntl.h>
#include <sys/mman.h>

#include "appimage/appimage.h"
#include "appimage/appimage_shared.h"
#include <appimage/appimage_shared.h>

#include "light_elf.h"

typedef unsigned char byte;
Expand Down