[bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal".

[Leo Prikler]

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