all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Tobias Geerinckx-Rice via Guix-patches via <guix-patches@gnu.org>
To: James Smith <jsubuntuxp@disroot.org>
Cc: 44039@debbugs.gnu.org
Subject: [bug#44039] [PATCH] gnu: Add slade.
Date: Fri, 16 Oct 2020 23:07:46 +0200	[thread overview]
Message-ID: <87zh4lanh9.fsf@nckx> (raw)
In-Reply-To: <20201016184333.1445-1-jsubuntuxp@disroot.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 --]

  reply	other threads:[~2020-10-16 21:08 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-16 18:43 [bug#44039] [PATCH] gnu: Add slade James Smith
2020-10-16 21:07 ` Tobias Geerinckx-Rice via Guix-patches via [this message]
2020-10-16 21:14   ` Tobias Geerinckx-Rice via Guix-patches via
2020-10-17  0:41   ` James Smith
2020-10-28 14:54   ` Ludovic Courtès
2020-10-29  2:49     ` James Smith
2020-10-29 23:39       ` Ludovic Courtès
2020-10-30  9:38         ` Tobias Geerinckx-Rice via Guix-patches via
2020-10-31 10:01           ` Ludovic Courtès
2021-05-15 20:52 ` James Smith
2021-09-18 17:47 ` [bug#44039] [PATCH v3] " James Smith via Guix-patches via
2021-09-22 14:21   ` bug#44039: [PATCH] " Ludovic Courtès

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87zh4lanh9.fsf@nckx \
    --to=guix-patches@gnu.org \
    --cc=44039@debbugs.gnu.org \
    --cc=jsubuntuxp@disroot.org \
    --cc=me@tobias.gr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.