Re: [PATCH v1 14/14] Build bucts and patched flashrom for I945 ThinkPads with Guix.

[Denis 'GNUtoo' Carikli]

On Mon, 29 Apr 2024 16:58:38 +0200
Adrien 'neox' Bourmault <neox@gnu.org> wrote:
>       - the duplicated .PHONY is removed in 
>         resources/packages/i945-thinkpads-install-utilities/Makefile.am
For the history behind that, some of the patches in this patch set have
already been extensively tested but at the last minute I sent the patch
set because I was away the week after, so there are things I forgot
about. So there were also some leftover from previous directions (like
trying to install and configure Guix automatically).

As for this specific patch, I forgot that I still needed to check the
added Makefile.am and (1) how it interacted with the topdir Makefile.am
and also (2) potential new Makefile.am in packages as well.

So I've checked now and automake does no warnings with (1) but (2)
has issues (more on that later).

For the .PHONY, apparently it just adds the .PHONY as-is to the
generated Makefile and multiple .PHONY are allowed in Makefiles, so
we're good here (I wasn't sure how automake was supposed to deal with
.PHONYs).

The underlying issue is that my approach has many problems (and that's
not necessarily exhaustive):
- Standard targets like 'check' 'clean' 'distclean' etc are redefined,
  and as I understand this is strongly discouraged against.
- Anything defined in the new Makefile.am is also included somehow so
  when adding a new Makefile.am, the contributors will need to take
  care of not using any conflicting variables. Hopefully at least on
  Guix, automake output warnings when that happens.
- The tests are not made with the test support of automake, and so this
  has a lot of disadvantages like not being able to run them in
  parallel, the lack of automatic logging support which complicates the
  code, and so on.
- The new targets are also not made in accordance to Makefile.am good
  practices, so for instance we can't change prefix and move the
  installed files.

All the issues above come from the fact that I tried not to complicate
too much the patch, and make it easy to review.

I've also tried another approach which fixes all the above directly but
the downside is that it's not clean either: part of the build system is
in bash and that's not well integrated in autotools. 

Changing the Guix part is relatively trivial to do and it works
well: the resulting utilities are installed in the right place with a
very small diff in the new Makefile.am:
> -DESTDIR ?= $(TOPDIR)/release
> [...]
> -RELEASE_DIR = $(DESTDIR)/i945-thinkpads-install
> +RELEASE_DIR = $(DESTDIR)/$(libexecdir)/gnuboot/i945-thinkpads-install
and we can even do it as it should and have something like:
> dist_pkgdata_DATA = $(PACK_NAME)-tarball-pack.tar.gz [...]
and adjust the rest accordingly (I've tested that for other unrelated
Guix images and it works well).

But then in both cases we either need to change all the bash code as
well to somewhat respect the settings gathered by autotools or to have
some Frankenstein's-monster-like system where there are two inconsistent
parts and part of the build is configurable (guix) and part is not. 

I'd prefer to avoid the later if possible.

Having the bash scripts respect all the autotools settings is doable and
gsrc (https://www.gnu.org/software/gsrc/) does exactly that: this
project generates some shell script (with autoconf) with the
settings, and that script can then be sourced by other scripts, so that
approach is doable and relatively easily to integrate in the build
system part.

The downside is that we'd need to modify every bash script and not miss
anything. Since that's painful to review and test, I'd prefer a route
where the autotools system look more like configurable Makefile(s) and
have this fixed later on when more and more parts are moved to Guix
packages.

Denis.

pgpttufgw6f_N.pgp
Description: OpenPGP digital signature