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

Refactor installation code #27

Merged
merged 2 commits into from
Apr 26, 2018
Merged

Conversation

TheAssassin
Copy link
Owner

Extracted code from the maintainer scripts to a central script to
avoid duplicate code.

Added support for systemd-binfmt with a binfmt.d config for distros
without binfmt-support packages.

Fixes #19

Extracted code from the maintainer scripts to a central script to
avoid duplicate code.

Added support for systemd-binfmt with a binfmt.d config for distros
without binfmt-support packages.

Fixes #19
@TheAssassin
Copy link
Owner Author

CC @NuLogicSystems

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

The debian binfmt-support package actually uses it's own systemd service:
/lib/systemd/system/binfmt-support.service
/etc/init/binfmt-support.conf
https://packages.debian.org/en/sid/amd64/binfmt-support/filelist

So, if this package is installed it will use that, not the upstream (systemd) bundled service.

@TheAssassin
Copy link
Owner Author

@NuLogicSystems that file just enables or disables the whole binfmts support entirely, apparently. I'm furthermore not 100% sure whether it loads the binfmt.d config files, and doesn't just read the /var/lib/binfmts files.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

Either way, if binfmt-support was installed when you installed your package, then systemctl restart systemd-binfmt wouldn't work, but systemctl restart binfmt-support probably would.

@TheAssassin
Copy link
Owner Author

@NuLogicSystems that's getting a little too complicated, in my opinion. I just tried restarting by the way, it didn't activate the binfmt.d files, but only applied the changes in /var/lib/binfmts.

@TheAssassin TheAssassin merged commit 64b53b3 into master Apr 26, 2018
@NuLogicSystems
Copy link

Yea, that makes sense if it follows the normal systemd config file hierarchy actually.

@TheAssassin TheAssassin deleted the refactor-installation-code branch April 26, 2018 16:56
@TheAssassin
Copy link
Owner Author

@NuLogicSystems I merged the PR for now, as it contains a few fixes, and doesn't break with the existing integration methods. I'll test the whole thing with the AUR PKGBUILD on Manjaro now, to see whether the integration works now.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

Did you already fix the pre >> post uninstall issue?
That does make quite a difference.

@TheAssassin
Copy link
Owner Author

Ah, crap! I forgot to push that commit. I'll fix that immediately. Thanks.

@NuLogicSystems
Copy link

Building it now on my Enlightenment system.

@NuLogicSystems
Copy link

I pushed a commit to AUR removing the old install file.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

The default pacman hook for binfmts "Registering binary formats" works on installation.
However, "Registering binary formats" does not run after uninstall.
We may need to add a post_remove hook for our package.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

OK, /usr/share/libalpm/hooks/systemd-binfmt.hook only has Install and Upgrade as operations.
Maybe I can get the maintainer to add Remove, otherwise my first thought applies.
If not then at the very least on uninstall, a message that tells the end user to run systemctl restart systemd-binfmt, or to reboot.

@NuLogicSystems
Copy link

I added a task here:
https://bugs.archlinux.org/task/58362

@TheAssassin
Copy link
Owner Author

Did you try to manually call this hook after uninstalling? Did it clean up the integration?

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

No the hook needs an Operation=Remove line for that to work.
Here is the hook:

[Trigger]
Type = File
Operation = Install
Operation = Upgrade
Target = usr/lib/binfmt.d/*.conf

[Action]
Description = Registering binary formats...
When = PostTransaction
Exec = /usr/share/libalpm/scripts/systemd-hook binfmt
NeedsTargets

@TheAssassin
Copy link
Owner Author

I meant like "do whatever the hook does" (like me calling systemd restart systemd-binfmt on Ubuntu). Then, check whether AppImages can be executed "normally", or whether any call to an AppImage will fail because the interpreter is no longer available on the system.

@NuLogicSystems
Copy link

Yes, that is basically what the hook does: systemd restart systemd-binfmt

@TheAssassin
Copy link
Owner Author

You didn't reply to my actual question: does it re-enable "normal" AppImage execution? In my tests on Debian/Ubuntu, it didn't work.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

I've never had an issue with appimages running if the exec bit is enabled on arch.
Or do you mean without changing the appimage files permisions manually?
I'm new to all this binfmt stuff myself.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

Stupid me, I was trying to do this in terminology.
Yes, after uninstall and triggering systemd restart systemd-binfmt double clicking no longer launches the appimage. After install double clicking launches the appimage again without playing with permissions, exec bit not set.

@TheAssassin
Copy link
Owner Author

That's a critical issue.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

So yes, it is working.
Terminology is just a PIA when it comes to this kind of stuff.
It doesn't even try if the exec but isn't set, just spits out "Permission denied"

@TheAssassin
Copy link
Owner Author

@NuLogicSystems You need to be a bit more elaborate, I think I'm missing your point.

Let's recapitulate:

If we set up binfmt support with such a config file, on installation, the format is registered in the kernel, and AppImageLauncher is called when (executable!) AppImages are executed.

Now, we want to remove the package. We restart this service, like the hook would do (doesn't matter if it's in the post_remove hook yet).

Expected behavior: kernel format is unregistered, AppImages can be executed, they launch normally.

Actual behavior (on Debian): Launching does no longer work. Expected problem: format is still registered, the "restart" didn't unregister it, but the interpreter is missing, therefore the call fails.

Actual behavior (on Arch): ?

@NuLogicSystems
Copy link

I'm switching to another machine, I've had issues launching appimages with appimagelauncher under Enlightenment.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

Does this work as intended under rpm based distrobutions?

@TheAssassin
Copy link
Owner Author

I'll re-check, let me quickly boot some openSUSE.

@NuLogicSystems
Copy link

The reason I ask is our conf file looks much different than the others I've looked at.

@TheAssassin
Copy link
Owner Author

Check https://www.freedesktop.org/software/systemd/man/binfmt.d.html for more information. The configuration file adheres to the format referred on this page, https://www.kernel.org/doc/Documentation/admin-guide/binfmt-misc.rst.

@NuLogicSystems
Copy link

Ours:
:appimage-type2:M:8:414902::/usr/bin/AppImageLauncher:
Others:
:Java:M::\xca\xfe\xba\xbe::/usr/bin/javawrapper:
:Applet:E::html::/opt/java/bin/appletviewer:
:Java:M::\xca\xfe\xba\xbe::/usr/bin/javawrapper:

:CLR:M::MZ::/usr/bin/mono:

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

Please let me know how openSUSE goes.
Any Idea what's causing this?
Failed to clean up old desktop files

@TheAssassin
Copy link
Owner Author

@NuLogicSystems they look different because the formats are different. I can explain to you what these lines mean. The line is correct, it works fine here, really!

@TheAssassin
Copy link
Owner Author

I just noticed a minor bug (another directory conflict) in the RPM, I'll fix it and will try then.

@NuLogicSystems
Copy link

Well, I'm getting the same error on both my arch enlightenment and Netrunner Rolling machines with the AUR package.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

If I use the old package from the repository I don't get the error runnning AppImageLauncher *.appimage.
That's even with manually adding the conf file and running the command as well.
However, launching just the appimige doesn't use appimagelauncher.

@TheAssassin
Copy link
Owner Author

What error?

I am referring to an installation bug "directory conflict", CMake's auto generated file list often includes empty directories, which is not a problem with Debian packages, but with RPM packages. Hence I added it to the exclude list I maintain.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

The "Failed to clean up old desktop files" error i'm getting with AppImageLauncher *.appimage.

$ AppImageLauncher lmms-1.2.0-rc5.43-linux-x86_64.AppImage
Failed to clean up old desktop files

@TheAssassin
Copy link
Owner Author

I can reproduce the bug now with the official packages. I'll have a look.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

Still, I don't know why it's not trying to launch with the interpereter when executing the appimage files by themselves. But one thing at a time. ;)

@TheAssassin
Copy link
Owner Author

The function is bool but didn't return, probably undefined behavior. I added an explicit return true statement, return false isn't used in the function (yet), so I hope this fixes the issue.

@TheAssassin
Copy link
Owner Author

@NuLogicSystems works now.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

That does, no more error.
But running just the appimage by itself is still not using the launcher.

$ ./lmms-1.2.0-rc5.43-linux-x86_64.AppImage
Carla appears to be installed on this system at /usr/lib[64]/carla so we'll use it.
Jack appears to be installed on this system, so we'll use it.
static QPlatformTheme* QKdeTheme::createKdeTheme(): Unable to determine KDEHOME
VST sync support disabled in your configuration
ALSA lib seq_hw.c:466:(snd_seq_hw_open) open /dev/snd/seq failed: No such device
cannot open sequencer: No such device
Couldn't create MIDI-client, neither with ALSA nor with OSS. Will use dummy-MIDI-client.

@TheAssassin
Copy link
Owner Author

TheAssassin commented Apr 26, 2018

What system? How can I reproduce the issue? Maybe create a separate issue with more information?

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

Netrunner Rolling
I built appimagelauncher.git from the AUR using pacaur.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

This works:
:appimage-type2:E::AppImage::/usr/bin/AppImageLauncher:

So it has to be something in your magic that arch doesn't like.
That or I just have the wrong type of AppImages, is there a type-0 or type-3?

@TheAssassin
Copy link
Owner Author

@NuLogicSystems this only uses the extension, and isn't reliable at all. We need the magic bytes check.

So far I can tell that the registration itself works, calling cat /proc/sys/fs/binfmt_misc/appimage-type2 yields a description of the format I defined.

Looking at the contents, though, I found the issue:
screenshot_2018-04-26_23-27-03

The magic description is wrong. I think I know how to fix it. Give me a minute or two, please.

@TheAssassin
Copy link
Owner Author

Fixed, @NuLogicSystems.

@NuLogicSystems
Copy link

BTW, in the interum since using my /etc/binmft.d/appimage.conf file was working.
I ran systemd restart systemd-binfmt after I uninstalled the package and deleted my file, normal function for appimages was restored.

@NuLogicSystems
Copy link

Rebuilding from AUR now.

@TheAssassin
Copy link
Owner Author

Good to know. Nevertheless, your file was only relying on the extension, which is unreliable for various reasons. Some AppImages ship without extension. And, technically, an extension on unix-ish platforms is merely a form of decorating the filename, but nothing technically necessary.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

Thanks for that. Been using Linux since 1995, before that i used Minux, AIX, 4.4BSD + (actually still use freeBSD), SunOS (Solaris), and System V.

I also owned a SPARC V8 and then a DEC Alpha back in the day..

Man I'm getting old..

Anyway, I only used the extension as a test to see if the magic numbers might have been the problem. ;)

@NuLogicSystems
Copy link

Yes, success. Launched the appimage with appimagelauncher this time.

@NuLogicSystems
Copy link

NuLogicSystems commented Apr 26, 2018

And uninstalling the pakage, then running systemctl restart systemd-binfmt, restores normal function.

We have a winner.. Yeahhhh.

@TheAssassin
Copy link
Owner Author

@NuLogicSystems thanks for your help. I did by the way not try to lecture you, but I write my comments in a way that future readers who don't know about a fact will understand it completely, therefore I tend to be more elaborate in my replies than the average GitHub user. I'm just glad we have a working Arch setup now... was way more work than for any other distro...

@NuLogicSystems
Copy link

I didn't think you were lecturing me. It just inadvertantly made me feel bit old, not your fault. ;)

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.

2 participants