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.

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

Also use $QT_ROOT_DIR #302

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

Also use $QT_ROOT_DIR #302

wants to merge 2 commits into from

Conversation

probonopd
Copy link
Owner

Also use the environment variable $QT_ROOT_DIR

#300 (comment)

Also use the environment variable `$QT_ROOT_DIR`

#300 (comment)
Copy link

github-actions bot commented Sep 3, 2024

Build for testing:
artifacts
Use at your own risk.

@probonopd
Copy link
Owner Author

@AlbrechtL does this improve the #300 situation for you when you, in other words, does it work without explicitly setting $QTDIR with this build? Thanks.

AlbrechtL added a commit to AlbrechtL/welle.io that referenced this pull request Sep 3, 2024
@AlbrechtL
Copy link

AlbrechtL commented Sep 3, 2024

Yes, it is working! See https://github.com/AlbrechtL/welle.io/actions/runs/10688777042

Corresponding commit: AlbrechtL/welle.io@bb40872

Copy link

github-actions bot commented Sep 4, 2024

Build for testing:
artifacts
Use at your own risk.

AlbrechtL added a commit to AlbrechtL/welle.io that referenced this pull request Sep 4, 2024
@AlbrechtL
Copy link

I my case the new version 843 works like the previous 841 one.
10_Create AppImage.txt

@probonopd
Copy link
Owner Author

probonopd commented Sep 4, 2024

Let's see whether @kevle can confirm that this improves the issue he experienced, too - then I think it's ready for being merged.
Thanks a lot!

@kevle
Copy link

kevle commented Sep 10, 2024

I just tested the build artifacts and it seems to abort correctly now:

2024/09/10 10:02:34 libPath: /toolchain/exports/x64-linux/installed/x64-linux-dynamic/lib/libQt6Core.so.6
ERROR Could not find offset for qt_prfxpath=: no qt_prfxpath= token in binary

In my own build I tried just skipping patching libQt6Core, and Qt manages to load the platform plugins just fine:

diff --git a/src/appimagetool/appdirtool.go b/src/appimagetool/appdirtool.go
index 842704d..2d35306 100644
--- a/src/appimagetool/appdirtool.go
+++ b/src/appimagetool/appdirtool.go
@@ -511,7 +511,12 @@ func patchQtPrfxpath(appdir helpers.AppDir, lib string, libraryLocationsInAppDir
        f.Seek(0, 0)
        // Search from the beginning of the file
        search := []byte("qt_prfxpath=")
-       offset := ScanFile(f, search) + int64(len(search))
+       prfxpathPos := ScanFile(f, search)
+       if prfxpathPos < 0 {
+               helpers.PrintError("Could not find offset for " + string(search), errors.New("no " + string(search) + " token in binary"))
+               return
+       }
+       offset := prfxpathPos + int64(len(search))
        log.Println("Offset of qt_prfxpath:", offset)
        /*
                What does qt_prfxpath=. actually mean on a Linux system? Where is "."?

In my environment, Qt is being built by vcpkg. Judging from what I understand from the design of go-appimage, it probably is out of scope to support libraries being built this way, since it expects to use system libraries.

If this is true, then aborting is probably the right thing to do to avoid scope creep.
If not, then checking for qt_prfxpath and only patching it when it was found is probably the way to go.

EDIT:

I just dug a little deeper into this, and it appears that since Qt 5.14, Qt can be build as "relocatable". Those builds do not need to be patched.

The windeployqt tool conveniently shares the build configuration of the Qt library files, so it has hardcoded knowledge about whether to patch the shared library files.

@probonopd
Copy link
Owner Author

@kevle could you send a pull request that only does the patching if qt_prfxpath exists and is set? Would highly appreciate it. Thank you very much for your investigation!

@kevle
Copy link

kevle commented Sep 12, 2024

@probonopd I'll give it a go!

@Samueru-sama

This comment was marked as outdated.

@probonopd
Copy link
Owner Author

Thanks @kevle. There is now a build for testing in #306, and it would be great if someone could give it a through test. Thank you!

@probonopd
Copy link
Owner Author

I lost track of this a bit, do you think it should still be merged or should we wait on #306 to be ready for primetime?

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants