GNU bug report logs - #49828
[PATCH 00/20] Add minetest mods

Previous Next

Package: guix-patches;

Reported by: Maxime Devos <maximedevos <at> telenet.be>

Date: Mon, 2 Aug 2021 15:48:02 UTC

Severity: normal

Tags: patch

Done: Leo Prikler <leo.prikler <at> student.tugraz.at>

Bug is archived. No further changes may be made.

Full log


Message #122 received at 49828 <at> debbugs.gnu.org (full text, mbox):

From: Leo Prikler <leo.prikler <at> student.tugraz.at>
To: Maxime Devos <maximedevos <at> telenet.be>, 49828 <at> debbugs.gnu.org
Subject: Re: [PATCH 05/20] build-system: minetest: Don't retain references
 to "bash-minimal".
Date: Thu, 05 Aug 2021 17:15:29 +0200
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





This bug report was last modified 3 years and 330 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.