Package: guix-patches;
Reported by: Noé Lopez <noelopez <at> free.fr>
Date: Wed, 16 Oct 2024 21:52:01 UTC
Severity: normal
Tags: patch
Done: Ludovic Courtès <ludo <at> gnu.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Ludovic Courtès <ludo <at> gnu.org> To: Noé Lopez <noelopez <at> free.fr> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, 73842 <at> debbugs.gnu.org, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Sebastian Dümcke <code <at> sam-d.com>, Christopher Baines <guix <at> cbaines.net> Subject: [bug#73842] [PATCH] pack: Add support for AppImage pack format. Date: Fri, 18 Oct 2024 14:20:33 +0200
Hi Noé & Sebastian,
Noé Lopez <noelopez <at> free.fr> skribis:
> From: Sebastian Dümcke <code <at> sam-d.com>
>
> * guix/scripts/pack.scm: Add Appimage format.
> * gnu/packages/appimage.scm
> (gnu packages appimage): New module.
> (fuse-for-appimage, squashfuse-for-appimage)
> (appimage-type2-runtime): New variables.
> * doc/guix.texi: Document AppImage pack.
>
> Co-authored-by: Noé Lopez <noelopez <at> free.fr>
> Change-Id: I09f1241dfb9b267f94dce59914dea527d35ac60e
[...]
> This patch adds a new AppImage export format to « guix pack ». This
> enables guix users to easily share packets or environments with their
> peers that don’t have guix via a statically linked, runs-everywhere
> executable.
>
> Sebastian and I co-authored this patch, I did the runtime packaging
> and Sebastian did the actual command. I’ve personally tested the
> generated AppImages with my friend’s various distros, they work great!
Nice work! Overall looks great to me. What follows are pretty minor
suggestions.
> +@cindex AppImage, create an AppImage file with guix pack
s/guix pack/@command{guix pack}/
> +Another format internally based on SquashFS is
> +@uref{https://appimage.org/, AppImage}. An AppImage file can be made
^
Nitpick: please leave two spaces after end-of-sentence periods
throughout this patch.
> +The resulting AppImage does not conform to the complete standard as it
> +currently does not contain a @code{.DirIcon} file. This does not impact
s/@code/@file/
> +@quotation Warning
> +Unless @option{--relocatable} is used, the software will contain
> +references to items inside the guix store, which are not present on
> +systems without Guix. It is recommended to use @option{--relocatable}
> +when distributing software to 3rd parties. A warning is printed when
> +this option is not used.
I would it perhaps more upfront, like:
When building an AppImage, always @emph{pass} the
@option{--relocatable} option (or @option{-R}, or @option{-RR}) to
make sure the image can be used on systems where Guix is not
installed.
In practice, if Guix is installed but the image’s dependencies are not
in the store, it won’t work either. So I think the “always” bit is not
exaggerated.
WDYT?
> +++ b/gnu/packages/appimage.scm
Could you add this file as a separate commit, before the one adding
AppImage support to ‘guix pack’?
> + (arguments
> + `(#:configure-flags ,#~(list (string-append "-Dudevrulesdir="
> + #$output "/udev/rules.d")
> + "-Duseroot=false" "--default-library=static")
> + #:tests? #f
Please add a short comment explaining why tests are skipped.
> + #:phases ,#~(modify-phases %standard-phases
The recommended style is to avoid quasiquote/unquote and instead write:
(arguments
(list #:configure-flags …
#:phases #~(…)))
> + (add-before 'configure 'set-paths
> + (lambda* (#:key inputs outputs #:allow-other-keys)
> + (let ((dummy-init.d (string-append (getcwd)
> + "/etc/init.d")))
> + (setenv "MOUNT_FUSE_PATH"
> + (string-append #$output "/sbin"))
> + (setenv "UDEV_RULES_PATH"
> + (string-append #$output "/lib/udev/rules.d"))))))))))
Maybe call this phase ‘set-fuse+udev+paths’, to avoid confusion with the
standard ‘set-paths’ phase.
> +(define squashfuse-for-appimage
> + (let ((revision "0")
> + (commit "e51978cd6bb5c4d16fae9eee43d0b258f570bb0f"))
> + (package
> + (inherit squashfuse)
> + (name "squashfuse")
> + (version (git-version "0.1.104" revision commit))
Can you add a comment explaining why this specific commit is needed?
That way, our future selves will know when ‘squashfuse-for-appimage’ can
be removed.
> +(define-public appimage-type2-runtime
> + (let ((revision "0")
> + (commit "47b665594856b4e8928f8932adcf6d13061d8c30"))
> + (package
> + (name "appimage-type2-runtime")
Likewise.
> + #:phases #~(modify-phases %standard-phases
> + (delete 'configure)
> + (delete 'check)
> + (replace 'install
> + (lambda _
> + (install-file "src/runtime/runtime-fuse3"
> + (string-append #$output "/bin"))))
> + ;; must be after all elf reliant phases
> + (add-after 'make-dynamic-linker-cache 'set-magic-bytes
> + (lambda _
> + (use-modules (ice-9 binary-ports))
Please do not use ‘use-modules’ in a non-top-level context; it’s not
guaranteed to work in that context.
Instead use #:modules (check the repo for examples).
> + (home-page "https://github.com/AppImage/type2-runtime")
> + (synopsis "Runtime for AppImages")
> + (description "The runtime is the executable part of every AppImage. It
Please make it a full sentence, as per
<https://guix.gnu.org/manual/devel/en/html_node/Synopses-and-Descriptions.html>.
> + (define (concatenate result file1 file2)
Please add a short comment below ‘define’ explaining what it does,
similar to docstrings.
> + (let ((p (open-file-output-port result))
Rather:
(call-with-output-file result
(lambda (output)
…))
> + (f1 (open-file-input-port file1))
> + (f2 (open-file-input-port file2)))
> + (put-bytevector p (get-bytevector-all f1))
> + (close-port f1)
> + (put-bytevector p (get-bytevector-all f2))
To avoid loading it all in memory, rather use:
(call-with-input-file file1
(lambda (input)
(dump-port input output)))
Same with FILE2.
> + (let* ((appdir "AppDir")
> + (squashfs "squashfs")
> + (profile-items (map store-info-item
> + (call-with-input-file "profile" read-reference-graph)))
> + (profile-path (find (cut (file-name-predicate "profile$") <> #f) profile-items)))
s/-path//
Also, rather: (find (lambda (item)
(string-suffix? "-profile" item))
items)
> + (mkdir-p appdir)
> + ;; copy all store items from the profile to the AppDir
> + (for-each (lambda (f)
> + (copy-store-item f appdir)) profile-items)
I believe you could write:
(populate-store '("profile") appdir)
> + ;; symlink the provided entry-point to AppDir/AppRun
> + (symlink (string-append "." profile-path "/" #$entry-point)
> + (string-append appdir "/AppRun"))
> + ;; create .desktop file as required by the spec
> + (make-desktop-entry-file
> + (string-append appdir "/" #$name ".desktop")
> + #:name #$name
> + #:exec #$entry-point)
> + ;; compress the AppDir
> + (invoke #+(file-append squashfs-tools "/bin/mksquashfs") appdir
> + squashfs "-root-owned" "-noappend"
> + "-comp" #+(compressor-name compressor))
> + ;; append runtime and squashFS into file AppImage
> + (concatenate #$output
> + #$(file-append runtime-package "/" runtime-path)
> + squashfs))))))
And you can finish with: (chmod #$output #o555).
(Then you can remove the doc that says to “chmod +x” the result.)
It looks like this procedure ignores its #:symlinks argument. Could you
add support for it?
Could you send an updated patch?
At any rate, kudos for this new backend! This had been suggested many
times, so I’m sure it’ll find some good use.
Thank you!
Ludo’.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.