all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Leo Prikler <leo.prikler@student.tugraz.at>
To: Maxime Devos <maximedevos@telenet.be>, 49828@debbugs.gnu.org
Subject: [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal".
Date: Thu, 05 Aug 2021 17:15:29 +0200	[thread overview]
Message-ID: <96a038c941308c8ec427d4b37bb333b32a7d0204.camel@student.tugraz.at> (raw)
In-Reply-To: <c436498dbd7f7d83fe364156e73dfa7951fb95ed.camel@telenet.be>

Am Donnerstag, den 05.08.2021, 16:41 +0200 schrieb Maxime Devos:
> Leo Prikler schreef op do 05-08-2021 om 15:42 [+0200]:
> > Am Donnerstag, den 05.08.2021, 15:16 +0200 schrieb Maxime Devos:
> > > [...]
> > > > > +(define* (install #:key inputs #:allow-other-keys #:rest
> > > > > arguments)
> > > > > +  (apply (@@ (guix build copy-build-system) install)
> > > > > +         #:install-plan (mod-install-plan (apply guess-mod-
> > > > > name
> > > > > arguments))
> > > > > +         arguments))
> > > > @@ is a code smell, as far as Guix is concerned.  Rather import
> > > > copy-build-system with the copy: prefix.
> > > 
> > > 'copy-build-system' does not export 'install', so I have to use
> > > '@@'
> > > here.
> > > Modifying 'copy-build-system' to export 'install' would
> > > presumably
> > > entail
> > > a many rebuilds.
> > I think the thing that's usually done here is fetching through
> > %standard-phases.
> > I.e. (define copy:install (assoc-ref copy-build-system:%standard-
> > phases 
> > 'install))
> 
> Done.
LGTM.

> > > > > +(define png-file?
> > > > > +  ((@@ (guix build utils) file-header-match) %png-magic-
> > > > > bytes))
> > > > Likewise import (guix build utils) directly.
> > > 
> > > Likewise.
> > In that case fine, but make a note to move the variable and
> > procedure
> > over to (guix build utils).
> 
> I made a note.
I'm not seeing it, did you send the right file?

> > The new lower-mod mostly LGTM, but
> > > +           ;; Mods are architecture-independent.
> > > +           ((#:target target #f) #f)
> > should be `target' imho.  What if the mod e.g. actually builds a
> > shared
> > object and somehow uses Lua's very dynamic FFI to load it?  (Even
> > if
> > that's not currently possible, it might be in the future).  Setting
> > it
> > to #f by default OTOH sounds very reasonable to me.
> 
> 'target' is set by 'make-bag' in (guix build-system), so if the code
> above is made
> 
>     ((#:target target #f) target)
> 
> then #:target will always be set to some triplet.  Mostly harmless,
> but a bit a waste of disk space since this leads to (slightly)
> different derivations depending on the value of "target".
I think deduplication should take care of that, but yeah.

> I don't think any mods use Lua's FFI, but some mods use 'os.execute',
> which takes a file name referring to an executable, which might need
> to be absolutised in Guix, and therefore some mods are architecture-
> dependent.
> 
> It dropped the ((#:target target #f) #f).  I noticed "#:implicit-
> cross-inputs?" wasn't set to #f.  That has been corrected as well.
Good catch.  

Since my only remaining complaint is somewhat minor, you don't need to
resend this patch; I'll have a look at the importer later (or maybe
someone else gets to do that in between), but I won't cover the package
definitions for the mods.  If they work for you and you checked the
licenses, they're probably going to be fine.

Greetings





  reply	other threads:[~2021-08-05 15:16 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-02 15:46 [bug#49828] [PATCH 00/20] Add minetest mods Maxime Devos
2021-08-02 15:50 ` [bug#49828] [PATCH 01/20] gnu: minetest: Respect --without-tests Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 02/20] gnu: minetest: Search for mods in MINETEST_MOD_PATH Maxime Devos
2021-08-02 17:28     ` Leo Prikler
2021-08-02 17:53       ` Maxime Devos
2021-08-02 18:47         ` Leo Prikler
2021-08-03 11:09           ` Maxime Devos
2021-08-03 11:10             ` Maxime Devos
2021-08-03 11:54               ` Leo Prikler
2021-08-02 15:50   ` [bug#49828] [PATCH 03/20] gnu: minetest: New package module Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 04/20] build-system: Add 'minetest-mod-build-system' Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal" Maxime Devos
2021-08-03  9:17     ` Leo Prikler
2021-08-03 11:59       ` Maxime Devos
2021-08-03 12:28         ` Leo Prikler
2021-08-05 11:01           ` Maxime Devos
2021-08-05 12:04             ` Leo Prikler
2021-08-05 13:16               ` Maxime Devos
2021-08-05 13:42                 ` Leo Prikler
2021-08-05 14:41                   ` Maxime Devos
2021-08-05 15:15                     ` Leo Prikler [this message]
2021-08-02 15:50   ` [bug#49828] [PATCH 06/20] guix: Add ContentDB importer Maxime Devos
2021-08-05 16:41     ` Leo Prikler
2021-08-07 18:31       ` Maxime Devos
2021-08-07 19:47         ` Leo Prikler
2021-08-09 20:00           ` [bug#49828] [PATCH 06/20] guix: Add ContentDB importer. (XXX Don't send yet) Maxime Devos
2021-08-09 20:04             ` Maxime Devos
2021-08-09 21:45             ` [bug#49828] [PATCH 06/20] guix: Add ContentDB importer. (XXX Yes send now) Leo Prikler
2021-08-10 11:02               ` [bug#49828] [PATCH 06/20] guix: Add ContentDB importer Maxime Devos
2021-08-10 12:16                 ` Leo Prikler
2021-08-10 15:07                 ` [bug#49828] [PATCH v3 00/20] Add minetest mods Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 01/20] gnu: minetest: Respect --without-tests Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 02/20] gnu: minetest: Search for mods in MINETEST_MOD_PATH Maxime Devos
2021-08-20 11:45                     ` bug#49828: " Leo Prikler
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 03/20] gnu: minetest: New package module Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 04/20] build-system: Add 'minetest-mod-build-system' Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 05/20] import/utils: Recognise GPL-3.0-or-later and friends Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 06/20] guix: Add ContentDB importer Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 07/20] gnu: Add minetest-mesecons Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 08/20] gnu: Add minetest-basic-materials Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 09/20] gnu: Add minetest-unifieddyes Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 10/20] gnu: Add minetest-pipeworks Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 11/20] gnu: Add minetest-coloredwood Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 12/20] gnu: Add minetest-ethereal Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 13/20] gnu: Add minetest-technic Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 14/20] gnu: Add minetest-throwing Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 15/20] gnu: Add minetest-throwing-arrows Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 16/20] gnu: Add minetest-unified-inventory Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 17/20] gnu: Add minetest-worldedit Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 18/20] gnu: Add minetest-mobs Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 19/20] gnu: Add minetest-mobs-animal Maxime Devos
2021-08-10 15:07                   ` [bug#49828] [PATCH v3 20/20] gnu: Add minetest-homedecor-modpack Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 07/20] gnu: Add minetest-mesecons Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 08/20] gnu: Add minetest-basic-materials Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 09/20] gnu: Add minetest-unifieddyes Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 10/20] gnu: Add minetest-pipeworks Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 11/20] gnu: Add minetest-coloredwood Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 12/20] gnu: Add minetest-ethereal Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 13/20] gnu: Add minetest-technic Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 14/20] gnu: Add minetest-throwing Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 15/20] gnu: Add minetest-throwing-arrows Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 16/20] gnu: Add minetest-unified-inventory Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 17/20] gnu: Add minetest-worldedit Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 18/20] gnu: Add minetest-mobs Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 19/20] gnu: Add minetest-mobs-animal Maxime Devos
2021-08-02 15:50   ` [bug#49828] [PATCH 20/20] gnu: Add minetest-homedecor-modpack Maxime Devos
2021-08-02 17:14   ` [bug#49828] [PATCH 01/20] gnu: minetest: Respect --without-tests Leo Prikler
2021-08-02 17:18     ` Maxime Devos
2021-08-02 17:22       ` Leo Prikler
2021-08-05 12:46 ` [bug#49828] [PATCH 00/20] Add minetest mods Andrew Ward
2021-08-05 21:10   ` Maxime Devos

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=96a038c941308c8ec427d4b37bb333b32a7d0204.camel@student.tugraz.at \
    --to=leo.prikler@student.tugraz.at \
    --cc=49828@debbugs.gnu.org \
    --cc=maximedevos@telenet.be \
    /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.