[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#49828] [PATCH 05/20] build-system: minetest: Don't retain reference
From: |
Leo Prikler |
Subject: |
[bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal". |
Date: |
Thu, 05 Aug 2021 15:42:49 +0200 |
User-agent: |
Evolution 3.34.2 |
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))
> > > +(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).
> > > +(define (lower-mod name . arguments)
> > > + (define lower (build-system-lower gnu-build-system))
> > > + (apply lower
> > > + name
> > > + #:imported-modules %minetest-build-system-modules
> > > + #:modules %default-modules
> > > + #:phases '%standard-phases
> > > + #:implicit-inputs? #f
> > > + ;; Mods are architecture-independent.
> > > + #:target #f
> > > + ;; Ensure nothing sneaks into the closure.
> > > + #:allowed-references '()
> > > + (substitute-keyword-arguments arguments
> > > + ((#:native-inputs native-inputs '())
> > > + (append native-inputs (standard-minetest-
> > > packages))))))
> > This appears a little confusing. On first glance, it does not seem
> > to
> > allow overriding e.g. #:phases, but on a second look using `apply'
> > together with shallowly substituted arguments would enable that.
>
> I modified the patch to move more things into 'substitute-keyword-
> arguments' to reduce confusion.
LGTM.
> > The
> > only thing that's missing imo is that #:implicit-inputs? is not
> > honoured for (standard-minetest-packages) -- I think you might want
> > to
> > rectify that.
>
> I modified 'lower-mod' to _not_ add 'standard-minetest-packages' to
> 'native-inputs' when #:implicit-inputs? is false, though I don't see
> why a package definition for a Minetest mod would do that.
>
> I also found a spellig bug ("to minimisation" -> "to minimise") which
> is now fixed.
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.
> btw, I submitted the "MINETEST_MOD_PATH" patch upstream, with your
> suggestion for "{std::string(...)}" construction:
> <https://github.com/minetest/minetest/pull/11515>;.
Nice, perhaps in 5.5 we will be able to enjoy this out-of-the-box.
Greetings
- [bug#49828] [PATCH 04/20] build-system: Add 'minetest-mod-build-system'., (continued)
- [bug#49828] [PATCH 04/20] build-system: Add 'minetest-mod-build-system'., Maxime Devos, 2021/08/02
- [bug#49828] [PATCH 03/20] gnu: minetest: New package module., Maxime Devos, 2021/08/02
- [bug#49828] [PATCH 09/20] gnu: Add minetest-unifieddyes., Maxime Devos, 2021/08/02
- [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal"., Maxime Devos, 2021/08/02
- [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal"., Leo Prikler, 2021/08/03
- [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal"., Maxime Devos, 2021/08/03
- [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal"., Leo Prikler, 2021/08/03
- [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal"., Maxime Devos, 2021/08/05
- [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal"., Leo Prikler, 2021/08/05
- [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal"., Maxime Devos, 2021/08/05
- [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal".,
Leo Prikler <=
- [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal"., Maxime Devos, 2021/08/05
- [bug#49828] [PATCH 05/20] build-system: minetest: Don't retain references to "bash-minimal"., Leo Prikler, 2021/08/05
[bug#49828] [PATCH 07/20] gnu: Add minetest-mesecons., Maxime Devos, 2021/08/02
[bug#49828] [PATCH 13/20] gnu: Add minetest-technic., Maxime Devos, 2021/08/02
[bug#49828] [PATCH 11/20] gnu: Add minetest-coloredwood., Maxime Devos, 2021/08/02
[bug#49828] [PATCH 14/20] gnu: Add minetest-throwing., Maxime Devos, 2021/08/02
[bug#49828] [PATCH 16/20] gnu: Add minetest-unified-inventory., Maxime Devos, 2021/08/02
[bug#49828] [PATCH 20/20] gnu: Add minetest-homedecor-modpack., Maxime Devos, 2021/08/02
[bug#49828] [PATCH 06/20] guix: Add ContentDB importer., Maxime Devos, 2021/08/02