Ensure local linuxdeploy plugins are executable #829
Conversation
rmartin16
force-pushed
the
linuxdeploy-plugin-exe
branch
2 times, most recently
from
07e6bb7
to
a842396
Compare
|
Ahhh....I didn't realize plugins could be an AppImage themselves. Nonetheless, when I try to use a local version of the Qt plugin or just specify Using Using local Qt plugin (that's been patched): The error is only happening in Docker. I'd be curious if you're hitting the error as well with |
|
I can confirm I'm seeing the same error in Docker; I'm wondering if this might be because of the ELF header - or possibly because the AppImage needs to be unpacked to run in Docker... |
|
I also tried it unpatched and had the same problem. This linuxdeploy/linuxdeploy#154 (comment) does suggest unpacking may be a possible solution. Alternatively, there's even some interesting solutions around |
|
I do so enjoy using a project where the project-recommended solution for getting something to run at all is "run dd over a binary"... (/me grumbles) It would appear that the dd fix is at least partially overlapping with the patch-elf-binary fix... if the |
|
I was able to hack things together to get the unpacked Qt AppImage plugin to run in Docker....but that approach is going to encompass a lot more management for that style of plugin... Initial testing with |
rmartin16
force-pushed
the
linuxdeploy-plugin-exe
branch
from
4fd2746
to
1efcde5
Compare
|
Ok....here's the story. The This all required a little bit of refactoring.....and I made the ELF file detection a bit more targeted. |
| use them as plugins while building the AppImage. ELF files need | ||
| special "magic" bytes zeroed to run properly in Docker. | ||
| """ | ||
| with self.command.input.wait_bar(f"Installing {self.file_name}..."): |
There was a problem hiding this comment.
These installations are already prefaced with prefixed log using the full_name....using the file_name here seemed more useful to understanding exactly what was being talked about. As for user-provided plugins, this is especially more useful since it explicitly tells the user which file is being installed in to the project.
Previously:
[newapp] Checking for Linuxdeploy plugins...
Using default gtk plugin
[linuxdeploy] linuxdeploy GTK plugin was not found; downloading and installing...
Downloading linuxdeploy-plugin-gtk.sh...
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100.0% • 00:00
Installing linuxdeploy GTK plugin... done
Using local file plugin ./linuxdeploy-plugin-my-plugin.sh
[linuxdeploy] Copying user-provided plugin into project
[newapp] Configuring Linuxdeploy plugins...
>>> Running Command:
>>> docker run --volume /home/user/tmp/beeware/newapp/linux:/app:z --volume /home/user/.cache/briefcase:/home/brutus/.cache/briefcase:z --rm briefcase/com.example.newapp:py3.10 /bin/bash -c 'echo $PATH'
>>> Working Directory:
>>> /home/user/tmp/beeware/newapp
>>> Command Output:
>>> /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
>>> Return code: 0
[newapp] Building AppImage...
Now:
[newapp] Checking for Linuxdeploy plugins...
Using default gtk plugin
[linuxdeploy] linuxdeploy GTK plugin was not found; downloading and installing...
Downloading linuxdeploy-plugin-gtk.sh...
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100.0% • 00:00
Installing linuxdeploy-plugin-gtk.sh... done
Using local file plugin ./linuxdeploy-plugin-my-plugin.sh
[linuxdeploy] Copying user-provided plugin into project
Installing linuxdeploy-plugin-my-plugin.sh... done
[newapp] Configuring Linuxdeploy plugins...
>>> Running Command:
>>> docker run --volume /home/user/tmp/beeware/newapp/linux:/app:z --volume /home/user/.cache/briefcase:/home/brutus/.cache/briefcase:z --rm briefcase/com.example.newapp:py3.10 /bin/bash -c 'echo $PATH'
>>> Working Directory:
>>> /home/user/tmp/beeware/newapp
>>> Command Output:
>>> /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
>>> Return code: 0
[newapp] Building AppImage...
With that said, when plugins are being downloaded, the Using {plugin} messages are awkwardly interspersed with the fetching of the plugins. This could probably be done better but I didn't want to do here. Furthermore, the downloading happens much less frequently than just enumerating the plugins to be used....which looks fine:
[newapp] Checking for Linuxdeploy plugins...
Using default gtk plugin
Using local file plugin ./linuxdeploy-plugin-my-plugin.sh
[newapp] Configuring Linuxdeploy plugins...
>>> Running Command:
>>> docker run --volume /home/user/tmp/beeware/newapp/linux:/app:z --volume /home/user/.cache/briefcase:/home/brutus/.cache/briefcase:z --rm briefcase/com.example.newapp:py3.10 /bin/bash -c 'echo $PATH'
>>> Working Directory:
>>> /home/user/tmp/beeware/newapp
>>> Command Output:
>>> /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
>>> Return code: 0
[newapp] Building AppImage...
[newapp] Entering Docker context...
1efcde5
to
6c4063a
Compare
| f"--appdir={app_dir}", | ||
| "-d", | ||
| "--appdir", | ||
| str(app_dir), |
There was a problem hiding this comment.
A minor detail: when converting pathlib objects into strings, we should be using os.fsdecode(), not str(). This is part of PEP519. It's unlikely to cause a problem in this case, but it's a good discipline to get in the habit of using so we don't get caught in a case when it does matter.
There was a problem hiding this comment.
I think that tweet is talking about external interfaces where you accept a "path-like" object, which could be a Path, str or bytes. But for internal code where you know it's a Path, calling str is just as safe as using the / operator.
PEP 519 explains this pretty well in the paragraph that starts "Another way to view this is as a hierarchy of file system path representations".
| f"--appdir={app_dir}", | ||
| "-d", | ||
| "--appdir", | ||
| str(app_dir), |
There was a problem hiding this comment.
As above, this should be os.fsdecode()
| f"--appdir={app_dir}", | ||
| "-d", | ||
| "--appdir", | ||
| str(app_dir), |
There was a problem hiding this comment.
As above - this should be os.fsdecode()
| with self.command.input.wait_bar(f"Installing {self.full_name}..."): | ||
| self.command.os.chmod(download_path, 0o755) | ||
| if self.file_name.endswith("AppImage"): | ||
| def post_install_processing(self): |
There was a problem hiding this comment.
This is the only change that doesn't make sense to me - why break this out as a different method? Or, if it's broken out - why not call it as part of install()?
Is there any condition where we'd not do post_install is there any condition in which we would install, but not post install, or only post install without an install occurring? Do you envisage a subclass needing to override/augment post-installation procedures? Unless either of these things are true, I'm not sure I see the value in splitting this method out.
There was a problem hiding this comment.
So, I struggled a bit with how to handle this. The problem as I perceive it is all current and future install code paths should all run the logic in post_install_processing()....so, then, who should be responsible for ensuring it's called?
As with the other tools, the code flow for verify and upgrade is controlled by the base class; these are important because they are the only way the tool ever gets installed. Since install() is being overridden for local plugin files, should that subclass (and future ones like it) take on the responsibility of calling post_install_processing() themselves?
I think I see either implementation as mostly equal....have the base class always call post_install_processing() or require the subclasses to call it themselves when they override install().
That's my logic, though, if it makes sense. Definitely open to feedback about it.
There was a problem hiding this comment.
I think I can see where you're coming from; however, from my perspective, the place where that approach falls down is that uninstall() and install() are atomic operations; but there's no use case (that I can think of) where you'd want post_install_processing() to be similarly atomic. Post install processing is something that has to be done for an install to be complete; by splitting it out, you've created a case where someone could do an install, forget that post-install processing is required, and end up with a state where you effectively have an "incomplete" installation. Unless there's a situation where that "incomplete" state is useful, it just seems like a foot-gun waiting to go off.
One approach would be to use the refactoring you've done here and make post_install_processing() the last thing that install() does, which provides a clean extension point if a different plugin requires different post-install processing - but we don't actually have that requirement, and I can't see a case where it would be required in future, either.
Given that it's easy enough to add back in if it turns out we need it later, my inclination is to collapse this back in to install() and keep the implementation simple - and if it turns out we do need this feature in future, you have my permission to keep the receipts so you can say "I told you so" :-)
There was a problem hiding this comment.
That makes sense....except for the exact implementation....I feel like I might be missing something.
If LinuxDeployBase.post_install_processing() doesn't remain its own method and is incorporated back in to LinuxDeployBase.install(), wouldn't it need to be effectively duplicated in to LinuxDeployLocalFilePlugin.install()?
There was a problem hiding this comment.
Oh yeah - that little detail... the point of the whole PR ;-)
In that case, factoring into a method makes perfect sense - but I'd argue it should be invoked from inside install(), rather than verify(), for the same reason - there's no case (I can see) where you'd want to install, but not have it fully post-installed.
| # treats the AppImage like a self-extracting executable. Using | ||
| # this environment variable instead of --appimage-extract-and-run | ||
| # is necessary to ensure AppImage plugins are extracted as well. | ||
| env["APPIMAGE_EXTRACT_AND_RUN"] = "1" |
There was a problem hiding this comment.
I needed another environment variable here: env['ARCH'] = 'x86_64'. Without it, it got complaints about multiple architectures in the AppImage when building a QT test app. This is when running on an x86 machine, so it's not M1 leakage; but it was building on Docker under macOS, so maybe something is leaking there. Either way, it can't hurt to add an explicit env['ARCH'] = self.host_arch.
There was a problem hiding this comment.
I added env['ARCH'] = self.host_arch.....but I think I'm a bit dubious of this approach.
-
Do you know where this requirement is coming from?
Neitherlinuxdeploynor the Qt plugin sources directly reference this environment variable. That implies to me some dependency is pulling it in and leveragesARCH.....but without knowing what, I don't know what it's using it for. -
If the problem is happening inside Docker, do we really want to use the arch of the host?
I'm not too sure of all the permutations possible but if the host could possibly report something other thanx86_64while that's what the docker container is using, should we be sourcing the arch from the container instead? -
Given these nebulous circumstances, I'm particularly inclined to let users override whatever Briefcase sets this to.
So,env.setdefault("ARCH", arch)instead.
There was a problem hiding this comment.
It looks like it's the appimagetool plugin, which AFAIK is bundled internally with linuxdeploy. Here's the log excerpt from an AppImage being built without the qt plugin enabled, on macOS in Docker, on an x86_64 machine:
-- Deploying desktop files --
Deploying desktop file /app/appimage/PySide6 Test/PySide6 Test.AppDir/com.example.pyside6test.desktop
-- Copying files into AppDir --
Copying file /app/appimage/PySide6 Test/PySide6 Test.AppDir/com.example.pyside6test.desktop to /app/appimage/PySide6 Test/PySide6 Test.AppDir/usr/share/applications/com.example.pyside6test.desktop
-- Deploying files into AppDir root directory --
Deploying files to AppDir root using desktop file: /app/appimage/PySide6 Test/PySide6 Test.AppDir/usr/share/applications/com.example.pyside6test.desktop
Deploying desktop file to AppDir root: /app/appimage/PySide6 Test/PySide6 Test.AppDir/usr/share/applications/com.example.pyside6test.desktop
Creating symlink for file /app/appimage/PySide6 Test/PySide6 Test.AppDir/usr/share/applications/com.example.pyside6test.desktop in/as /app/appimage/PySide6 Test/PySide6 Test.AppDir
Deploying icon to AppDir root: /app/appimage/PySide6 Test/PySide6 Test.AppDir/usr/share/icons/hicolor/128x128/apps/pyside6test.png
Creating symlink for file /app/appimage/PySide6 Test/PySide6 Test.AppDir/usr/share/icons/hicolor/128x128/apps/pyside6test.png in/as /app/appimage/PySide6 Test/PySide6 Test.AppDir
Deploying AppRun symlink for executable in AppDir root: /app/appimage/PySide6 Test/PySide6 Test.AppDir/usr/bin/com.example.pyside6test
Creating symlink for file /app/appimage/PySide6 Test/PySide6 Test.AppDir/usr/bin/com.example.pyside6test in/as /app/appimage/PySide6 Test/PySide6 Test.AppDir/AppRun
-- Running output plugin: appimage --
[appimage/stdout] Found appimagetool: /tmp/appimage_extracted_178f157ec37b2710f37a5a6e5bb1323f/plugins/linuxdeploy-plugin-appimage/usr/bin/appimagetool
[appimage/stdout]
[appimage/stderr] Running command: /tmp/appimage_extracted_178f157ec37b2710f37a5a6e5bb1323f/plugins/linuxdeploy-plugin-appimage/usr/bin/appimagetool "/app/appimage/PySide6 Test/PySide6 Test.AppDir" "-g"
[appimage/stderr]
[appimage/stdout] WARNING: appstreamcli command is missing, please install it if you want to use AppStream metadata
[appimage/stderr] appimagetool, continuous build (commit 7f9be69), build <local dev build> built on 2022-07-16 02:23:34 UTC
[appimage/stderr] More than one architectures were found of the AppDir source directory "/app/appimage/PySide6 Test/PySide6 Test.AppDir"
[appimage/stderr] A valid architecture with the ARCH environmental variable should be provided
[appimage/stderr] e.g. ARCH=x86_64 appimagetool ...
ERROR: Failed to run plugin: appimage (exit code: 1)
The process of running x86_64 Docker images on M1 hardware is still sufficiently buggy that I'm willing to put that into the "we'll make it work when they do" bucket. At least for now, I think it's reasonable to assume/require that the architecture on which you're building is the architecture you're targeting, and not provide any special handling to override; we can revisit that decision if it turns out to be more complicated that this in practice.
rmartin16
force-pushed
the
linuxdeploy-plugin-exe
branch
from
6c4063a
to
24d6e7f
Compare
rmartin16
force-pushed
the
linuxdeploy-plugin-exe
branch
from
24d6e7f
to
ac7e04e
Compare
Codecov Report
|
Issue
If a user specifies a local file without execute permissions as a
linuxdeployplugin,linuxdeploywill ignore the plugin.linuxdeployLog ExcerptNote: these entries only show in
linuxdeploy's log with the-v0flag andDEBUG_PLUGIN_DETECTIONenv var non-null.Additionally, plugins at that are AppImages (whether user-provided or downloaded) causes errors in Docker either because their ELF header is not patched or they are not extracted before they are run.
Resolution
After the plugin is copied in to the app's bundle directory, set the
executepermissions for the file. Plugins specified via URL already have execute permissions added after the download.All AppImage files now have their ELF header patched. The
--appimage-extract-and-runflag forlinuxdeployis replaced withAPPIMAGE_EXTRACT_AND_RUN=1environment variable so all AppImages are extracted before use.PR Checklist: