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

Leo Prikler schreef op di 03-08-2021 om 11:17 [+0200]:
> Hi,
> 
> I'd merge this and 04/20 into a single patch.  04/20 does of its own
> give a good incentive as to why a new build system is to be used (this
> could instead be handled by the importer), with this phase added it
> makes slightly more sense.

As an argument for having a 'minetest-mod-build-system', consider
some ways 'minetest-mod-build-system' could be improved in the future:

  * a phase could be added to minimise PNG images (e.g. using 'optipng')
  * likewise, for lua code
  * some basic tests could be added (e.g. creating a new world and
    loading the mod, testing that Minetest doesn't raise an error during
    mod loading)

Also, having "#:install-plan '(("." "share/minetest/mods/the-mod-name"))"
appear in every package definition seems rather repetitive to me.

The idea behind "04/20" and "05/20" being separate patches, is to start
with a basic "minetest-mod-build-system" and gradually improve it.
The small improvements (currently only one, i.e., 05/20) could be reviewed
separately from each other and whether there should be a
"minetest-mod-build-system" at all.

E.g., see attached a patch that sets #:allowed-references '(), ensuring
nothing sneaks into the closure.  Another change I'm thinking of, is including
only "tar", "gzip" and the like as implicit inputs, and not "bash" or "coreutils",
though that's probably useless if shebang patching has been disabled.

> OTOH, perhaps we shouldn't install those shell scripts in the first
> place?  Perhaps we can instead make the importer generate packages
> based directly on copy-build-system, in which those static strings are
> already evaluated.  WDYT?

Directly using 'copy-build-system' makes it more difficult to make the
improvements listed above. I don't know what you mean with ‘in which those
static strings are already evaluated’ -- what are ‘those static strings’ here?

I suppose it is possible to exclude shell scripts from installation, but
just installing everything (and disabling shebang patching) seems simpler.

Greetings,
Maxime.

Hi,

Am Dienstag, den 03.08.2021, 13:59 +0200 schrieb Maxime Devos:
> Leo Prikler schreef op di 03-08-2021 om 11:17 [+0200]:
> > Hi,
> > 
> > I'd merge this and 04/20 into a single patch.  04/20 does of its
> > own
> > give a good incentive as to why a new build system is to be used
> > (this
> > could instead be handled by the importer), with this phase added it
> > makes slightly more sense.
> 
> As an argument for having a 'minetest-mod-build-system', consider
> some ways 'minetest-mod-build-system' could be improved in the
> future:
> 
>   * a phase could be added to minimise PNG images (e.g. using
> 'optipng')
>   * likewise, for lua code
>   * some basic tests could be added (e.g. creating a new world and
>     loading the mod, testing that Minetest doesn't raise an error
> during
>     mod loading)
Of course there's more that can be done here, but this just reaffirms
my earlier point, that the build system on its own as it is in 04/20
looks rather unfinished.  I'd personally prefer introducing it as one
whole as it gives a bigger picture of the whole thing rather than
digging into every detail.

> Also, having "#:install-plan '(("." "share/minetest/mods/the-mod-
> name"))"
> appear in every package definition seems rather repetitive to me.
Perhaps, but it's not like there aren't other groups of things that can
be implemented trivially in terms of copy-build-system.

> The idea behind "04/20" and "05/20" being separate patches, is to
> start
> with a basic "minetest-mod-build-system" and gradually improve it.
> The small improvements (currently only one, i.e., 05/20) could be
> reviewed
> separately from each other and whether there should be a
> "minetest-mod-build-system" at all.
See above.

> E.g., see attached a patch that sets #:allowed-references '(),
> ensuring
> nothing sneaks into the closure.  Another change I'm thinking of, is
> including
> only "tar", "gzip" and the like as implicit inputs, and not "bash" or
> "coreutils",
> though that's probably useless if shebang patching has been disabled.
IMO that patch should also be merged "as one" with the others.

> > OTOH, perhaps we shouldn't install those shell scripts in the first
> > place?  Perhaps we can instead make the importer generate packages
> > based directly on copy-build-system, in which those static strings
> > are
> > already evaluated.  WDYT?
> 
> Directly using 'copy-build-system' makes it more difficult to make
> the improvements listed above. I don't know what you mean with ‘in
> which those static strings are already evaluated’ -- what are ‘those
> static strings’ here?
‘Those static strings’ are exactly "the-mod-name" in 
> #:install-plan '(("." "share/minetest/mods/the-mod-name"))
I'm still not quite convinced, that whatever improvements you seem
there to be can't be made by using a good enough include regexp.

> I suppose it is possible to exclude shell scripts from installation,
> but just installing everything (and disabling shebang patching) seems
> simpler.
Likewise leaving shell references in there and using plain copy-build-
system would be simpler than making a new build system, wouldn't it?  I
don't think that's a good reason not to make a proper install plan.

Greetings