Skip to content

Navigation Menu

Sign in
Sign up

Replace libappimage submodule with FetchContent - #1190

Open
lalten wants to merge 3 commits into
AppImage:master from
lalten:remove-git-submodule
Open

Replace libappimage submodule with FetchContent #1190
lalten wants to merge 3 commits into
AppImage:master from
lalten:remove-git-submodule

Conversation

@lalten

@lalten lalten commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

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

Comment thread src/CMakeLists.txt
Comment thread src/appimagetool.c
#include <stdbool.h>

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

Copy link
Copy Markdown
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.

@TheAssassin TheAssassin Aug 18, 2022

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

Comment thread cmake/dependencies.cmake
Comment thread cmake/dependencies.cmake

Copy link
Copy Markdown
Member

Will have a look later, thanks.

lalten commented Apr 4, 2022

Copy link
Copy Markdown
Contributor Author

I guess this will also fix #1185

Comment thread cmake/dependencies.cmake

@TheAssassin TheAssassin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for the late review. Looks good so far. Please see @Tachi107's comment, we need to bump the minimum required CMake version.

lalten reacted with thumbs up emoji
@TheAssassin TheAssassin changed the title (削除) Remove libappimage git submodule (削除ここまで) (追記) Replace libappimage submodule with FetchContent (追記ここまで) Aug 18, 2022
lalten added 2 commits August 18, 2022 16:35 
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 August 18, 2022 14:36

lalten commented Aug 18, 2022

Copy link
Copy Markdown
Contributor Author

Updated to rebase your earlier force-push 8749879 to latest master and added a separate commit for the new changes

Comment thread cmake/dependencies.cmake
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

@TheAssassin TheAssassin Aug 18, 2022

Copy link
Copy Markdown
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.

Comment thread 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日

@TheAssassin TheAssassin Aug 18, 2022

Copy link
Copy Markdown
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.

Comment thread 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

@TheAssassin TheAssassin Aug 18, 2022

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread cmake/dependencies.cmake
include(FetchContent)

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

@TheAssassin TheAssassin Aug 18, 2022

Copy link
Copy Markdown
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?

Comment thread cmake/dependencies.cmake
# 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
Copy Markdown

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

@TheAssassin TheAssassin Aug 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheAssassin TheAssassin TheAssassin requested changes
+1 more reviewer
@Tachi107 Tachi107 Tachi107 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /