[bug#44039] [PATCH] gnu: Add slade.

[Tobias Geerinckx-Rice via Guix-patches via <guix-patches@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 2980 bytes --]

James,

Thanks for the patch!

James Smith 写道：
> +(define-public slade

Let's keep modules vaguely alphabetical.  Could you move this 
above ‘tiled’?

> +         (add-after 'install 'patch-gdk-backend

Nitpick: it's not patching anything.

> +           ;; Set GDK_BACKEND to x11 to prevent crash on 
> Wayland.
> +           ;; See 
> https://github.com/sirjuddington/SLADE/issues/1097 for details.
> +           (lambda* _

No need for lambda* over lambda when you're not using any of its 
extended features....

> +                 (string-append (assoc-ref %outputs "out") 
> "/bin/slade")

...however, you *could* use its keyword arguments to get rid of 
this ugly %outputs pseudo-global.  See below.

> +               `("GDK_BACKEND" "" = (,"x11")))

This works but the "" and , are redundant.

All in all, the phase can be rewritten as:

         (add-after 'install 'wrap-with-x11-gdk-backend
           ;; Set GDK_BACKEND to x11 to prevent crash on Wayland. 
           See
           ;; https://github.com/sirjuddington/SLADE/issues/1097 
           for details.
           (lambda* (#:key outputs #:allow-other-keys)
             (wrap-program
                 (string-append (assoc-ref outputs "out") 
                 "/bin/slade")
               '("GDK_BACKEND" = ("x11")))
             #t)))

> +       #:tests? #f))

Are there no tests at all?  If so, note in a comment:

       #:tests? #f))                    ; no test suite

Same if there are tests but they're broken or pointless (linting 
etc.).

There's one more problem:

  set(ZIP_COMMAND "${ZIPTOOL_ZIP_EXECUTABLE}" -X -UN=UTF8 -9 -r \
    "${CMAKE_BINARY_DIR}/slade.pk3" .)

Even zip -X won't create an identical archive on every run.  There 
doesn't seem to be an option to do so.  The result:

  --- /gnu/store/aaa-slade-3.1.12a/share/slade3/slade.pk3
  +++ /gnu/store/bbb-slade-3.1.12a/share/slade3/slade.pk3
   Zip file size: 3624588 bytes, number of entries: 768
  -drwxr-xr-x  3.0 unx  0 b- stor 20-Oct-16 19:17 html/
  -drwxr-xr-x  3.0 unx  0 b- stor 20-Oct-16 19:17 config/
  -drwxr-xr-x  3.0 unx  0 b- stor 20-Oct-16 19:17 config/colours/
  +drwxr-xr-x  3.0 unx  0 b- stor 20-Oct-16 19:31 html/
  +drwxr-xr-x  3.0 unx  0 b- stor 20-Oct-16 19:31 config/
  +drwxr-xr-x  3.0 unx  0 b- stor 20-Oct-16 19:31 config/colours/

This is not ideal: Guix aims for reproducible builds.

The following made multiple builds on one machine identical, but 
was not consistent between file systems, probably due to readdir 
order:

+ (add-before 'build 'reset-slade.pk3-timestamps
+   ;; This appears sufficient to make slade.pk3 reproducible.
+   (lambda _
+     (invoke "find" "../source/dist/res" "-exec" "touch"
+             "--no-dereference" "-t" "197001010000.00" "{}" 
"+")))

I suppose I could try using find to sort the files before invoking 
zip, or something.  Thoughts?

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]