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

Jump to bottom

GTK 4 support #20

Merged
merged 14 commits into from
Dec 1, 2021
Merged

GTK 4 support #20

merged 14 commits into from
Dec 1, 2021

Conversation

TheTumultuousUnicornOfDarkness
Copy link
Collaborator

Hello,

This PR add GTK 4 support in linuxdeploy-plugin-gtk. According to this message, GTK+ 4.0 do not support IM modules.
I removed explicit mentions to GTK 2 and GTK 3 to cover GTK 4.

I added Dockerfiles to test the plugin with a GTK3 and a GTK4 application (the widget factory) on Debian, Fedora, openSUSE and Ubuntu.
There is an minor issue on the GitHub Runner with the shared volume (/AppImage inside containers), but I do not have this problem locally. It does not matter, because the volume store built AppImages and we do not really need them in fact.

Please note this PR is based on #19.

@TheTumultuousUnicornOfDarkness TheTumultuousUnicornOfDarkness linked an issue Apr 25, 2021 that may be closed by this pull request
GTK 4 support #17
Closed
@TheAssassin
Copy link
Member

Are you saying that Gtk+2 is no longer supported?

@TheTumultuousUnicornOfDarkness
Copy link
Collaborator Author

Well, before my change, I found Gtk+2 was not really supported, because the plugin does not copy Gtk+2 modules. Maybe it was working partially, but I guess if it works, it is because it uses system libraries.
Gtk+2 is EOL, I am not sure if it is worth it to add full Gtk+2 support in 2021.

@TheAssassin
Copy link
Member

Doesn't have to be full support, let's call it "best effort". There's still quite a bunch of Gtk+2 apps out there, e.g., anything written for DEs like XFCE or MATE. I just don't want to break existing deployment setups.

@TheAssassin
Copy link
Member

I see, you haven't actually removed any functionality. Well, that's fine then, I guess.

@TheTumultuousUnicornOfDarkness TheTumultuousUnicornOfDarkness marked this pull request as draft April 25, 2021 17:17
@TheTumultuousUnicornOfDarkness
Copy link
Collaborator Author

Let me add a new commit to allow people to be able to still use this plugin with Gtk+2 applications.

@TheTumultuousUnicornOfDarkness
Copy link
Collaborator Author

With c5af397, I restored the old behavior for Gtk+2 apps.
I think we are good now.

@TheTumultuousUnicornOfDarkness TheTumultuousUnicornOfDarkness marked this pull request as ready for review April 25, 2021 18:05
@TheTumultuousUnicornOfDarkness
Copy link
Collaborator Author

@TheAssassin do I need to change something else?

TheAssassin
TheAssassin reviewed Jun 27, 2021
View reviewed changes
Copy link
Member

@TheAssassin TheAssassin left a comment

Choose a reason for hiding this comment

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

Somehow this review got lost >6 weeks ago. I just found it on GitHub. Sorry for the delay.

linuxdeploy-plugin-gtk.sh Outdated Show resolved Hide resolved
@TheTumultuousUnicornOfDarkness
Store GTK version in DEPLOY_GTK_VERSION
5c8f98e
@TheTumultuousUnicornOfDarkness
CI: fix errors
04a5d67 
find: ‘/tmp/AppDir/usr/bin’: No such file or directory
TheAssassin
TheAssassin reviewed Jun 27, 2021
View reviewed changes
.github/workflows/containers.yml Outdated Show resolved Hide resolved
.github/workflows/containers.yml
- name: Build container
run: |
mkdir -pv "${ARTIFACT_DIR}"
buildah bud \
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference to the more standard Docker command line? I haven't heard of this tool until 5 minutes ago.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Buildah build rootless containers easily.
Docker on GitHub Actions requires QEMU to work, that is overkill.
EDIT: Ah, you mean like this? I am not familiar with that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I remember the "why" now.
Buildah allows to mount volume during the build, while docker build does not.
The idea is to mount a volume under /AppImage during the container build, so all built AppImages are available in this directory.
Then, we can create artifacts from AppImages easily (because docker cp only works with running containers, not with images).

Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to build the AppImages during the container build, though? The typical workflow is to build a Docker image with the dependencies, then run an instance that executes a build script.

Anyway, I don't really care, let's merge this!

.github/workflows/containers.yml Outdated

- uses: actions/upload-artifact@v2
with:
name: "${LINUX_DISTRO} with ${GTK_VERSION}"
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned that this is going to cause quota issues.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I don't understand why GitHub's own image registry is not used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea was to store built AppImages as artifact, not the container image itself.
It is more for troubleshooting so I can delete this step if needed.

.github/workflows/main.yml Outdated Show resolved Hide resolved
containers/gtk3/Dockerfile.fedora
@@ -0,0 +1,22 @@
FROM docker.io/fedora:33 AS build-stage
Copy link
Member

Choose a reason for hiding this comment

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

Why do you specify the docker.io (default) registry manually? I'm curious whether there is any kind of advantage in doing so.
Of course, it's fine, functionality wise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is required with buildah, because the docker.io registry is not implicitly set by default.

linuxdeploy-plugin-gtk.sh Outdated Show resolved Hide resolved
linuxdeploy-plugin-gtk.sh
@@ -108,6 +127,11 @@ else
exit 1
fi

if ! command -v find &>/dev/null && ! type find &>/dev/null; then
echo -e "$0: find not found.\nInstall findutils then re-run the plugin."
Copy link
Member

Choose a reason for hiding this comment

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

👍

@qarmin
Copy link

qarmin commented Dec 1, 2021 •
edited
Loading

Still some features are missing?
I tested this with binary - czkawka_gui.zip and it works fine

@TheAssassin TheAssassin merged commit b8e3a5e into master Dec 1, 2021
@TheAssassin TheAssassin deleted the gtk4 branch December 1, 2021 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheAssassin TheAssassin TheAssassin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GTK 4 support
3 participants
@TheTumultuousUnicornOfDarkness @TheAssassin @qarmin