Re: [PATCH 3/4] Common make rules for building erofs images

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 9040 bytes --]

On 9/8/25 06:01, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
> 
>> Instead of duplicating the logic in three different places, write common
>> code for all three makefiles to use instead.  This will make future
>> changes much easier.
>>
>> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> 
> Makes sense, and we could do this now for the s6-rc directory without
> having to wait for the generated file list stuff.  Does need to be
> updated for my review comments from the previous patch though (stick to
> POSIX).

If we are going to stick with POSIX, I think it is best to test with both
GNU make and BSD make.  Otherwise, GNUisms are bound to creep in.

As an aside, BSD make has an amazing feature ("meta" mode) that would be
awesome to support in GNU make.

>> ---
>>  host/rootfs/Makefile    | 51 ++++---------------------------------------------
>>  host/rootfs/default.nix |  1 +
>>  img/app/Makefile        | 42 ++++------------------------------------
>>  img/app/default.nix     |  1 +
>>  lib/erofs.mk            | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  vm/sys/net/Makefile     | 35 +++------------------------------
>>  vm/sys/net/default.nix  |  1 +
>>  7 files changed, 65 insertions(+), 117 deletions(-)
>>
>> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile
>> index 3560707d21bcf53e4f3ad5e916d21e8be56cc3a1..dce8315fe5be19d2569d6c9a429276e4abd696ad 100644
>> --- a/host/rootfs/Makefile
>> +++ b/host/rootfs/Makefile
>> @@ -4,11 +4,10 @@
>>  .POSIX:
>>  
>>  include ../../lib/common.mk
>> +include ../../lib/erofs.mk
>>  include file-list.mk
>>  
>> -dest = build/rootfs.erofs
>> -
>> -DIRS = \
>> +override DIRS ::= \
>>  	dev \
>>  	etc/s6-linux-init/env \
>>  	etc/s6-linux-init/run-image/configs \
>> @@ -42,46 +41,9 @@ DIRS = \
>>  	proc \
>>  	sys
>>  
>> -FIFOS = etc/s6-linux-init/run-image/service/s6-svscan-log/fifo
>> -
>> -BUILD_FILES = build/etc/s6-rc
>> -
>> -$(dest): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(addprefix image/,$(FILES)) $(BUILD_FILES) build/empty build/fifo file-list.mk
>> -	( \
>> -	    cat $(PACKAGES_FILE) ;\
>> -	    for file in $(FILES) $(LINKS); do printf 'image/%s\n%s\n' $$file $$file; done ;\
>> -	    for file in $(BUILD_FILES); do printf '%s\n%s\n' $$file $${file#build/}; done ;\
>> -	    printf 'build/empty\n%s\n' $(DIRS) ;\
>> -	    printf 'build/fifo\n%s\n' $(FIFOS) ;\
>> -	) | ../../scripts/make-erofs.sh $@
>> -
>> -build/fifo:
>> -	mkdir -p build
>> -	mkfifo -m 0600 $@
>> -
>> -build/empty:
>> -	mkdir -p $@
>> +override FIFOS ::= etc/s6-linux-init/run-image/service/s6-svscan-log/fifo
>>  
>> -# s6-rc-compile's input is a directory, but that doesn't play nice
>> -# with Make, because it won't know to update if some file in the
>> -# directory is changed, or a file is created or removed in a
>> -# subdirectory.  Using the whole source directory could also end up
>> -# including files that aren't intended to be part of the input, like
>> -# temporary editor files or .license files.  So for all these reasons,
>> -# only explicitly listed files are made available to s6-rc-compile.
>> -build/etc/s6-rc: $(addprefix image/,$(S6_RC_FILES)) file-list.mk
>> -	mkdir -p $$(dirname $@)
>> -	rm -rf $@
>> -
>> -	dir=$$(mktemp -d) && \
>> -	    tar -C image -c $(S6_RC_FILES) | tar -C $$dir -x --strip-components 2 && \
>> -	    s6-rc-compile $@ $$dir; \
>> -	    exit=$$?; rm -r $$dir; exit $$exit
>> -
>> -clean:
>> -	-chmod -Rf +w build
>> -	rm -rf build
>> -.PHONY: clean
>> +all: $(dest)
>>  
>>  # veritysetup format produces two files, but Make only (portably)
>>  # supports one output per rule, so we combine the two outputs then
>> @@ -111,11 +73,6 @@ debug:
>>  	    $(VMLINUX)
>>  .PHONY: debug
>>  
>> -update-file-list:
>> -	../../scripts/genfiles.awk image > file-list.mk
>> -
>> -.PHONY: update-file-list
>> -
>>  run: build/live.img $(EXT_FS) build/rootfs.verity.roothash
>>  	@set -x && \
>>  	ext="$$(mktemp build/spectrum-rootfs-extfs.XXXXXXXXXX.img)" && \
>> diff --git a/host/rootfs/default.nix b/host/rootfs/default.nix
>> index 998220d7b6ed322f64ee52c704e71ec9b4643f59..561bf660829dcd5f5e2ee841662727b262d43ed3 100644
>> --- a/host/rootfs/default.nix
>> +++ b/host/rootfs/default.nix
>> @@ -178,6 +178,7 @@ stdenvNoCC.mkDerivation {
>>      fileset = fileset.intersection src (fileset.unions [
>>        ./.
>>        ../../lib/common.mk
>> +      ../../lib/erofs.mk
>>        ../../scripts/make-erofs.sh
>>      ]);
>>    };
>> diff --git a/img/app/Makefile b/img/app/Makefile
>> index 5bb1a6a2f9acd13aba95abb0e918a7f21943b230..0c3a8311973f5f82eb2af6ea3ba43f67d36dec2e 100644
>> --- a/img/app/Makefile
>> +++ b/img/app/Makefile
>> @@ -5,6 +5,7 @@
>>  .POSIX:
>>  
>>  include ../../lib/common.mk
>> +include ../../lib/erofs.mk
>>  include file-list.mk
>>  
>>  prefix = build/host
>> @@ -18,7 +19,6 @@ HOST_BUILD_FILES = \
>>  	$(imgdir)/appvm/vmlinux
>>  
>>  all: $(HOST_BUILD_FILES)
>> -.PHONY: all
>>  
>>  $(imgdir)/appvm/vmlinux: $(KERNEL)
>>  	mkdir -p $$(dirname $@)
>> @@ -30,38 +30,13 @@ $(imgdir)/appvm/blk/root.img: ../../scripts/make-gpt.sh ../../scripts/sfdisk-fie
>>  	    build/rootfs.erofs:root:5460386f-2203-4911-8694-91400125c604:root
>>  	mv $@.tmp $@
>>  
>> -VM_DIRS = dev run proc sys tmp \
>> +DIRS = dev run proc sys tmp \
>>  	etc/s6-linux-init/run-image/service \
>>  	etc/s6-linux-init/run-image/user \
>>  	etc/s6-linux-init/run-image/wait
>> -VM_FIFOS = etc/s6-linux-init/run-image/service/s6-linux-init-shutdownd/fifo
>> +FIFOS = etc/s6-linux-init/run-image/service/s6-linux-init-shutdownd/fifo
>>  
>> -VM_BUILD_FILES = build/etc/s6-rc
>> -
>> -build/fifo:
>> -	mkdir -p build
>> -	mkfifo -m 0600 $@
>> -
>> -build/empty:
>> -	mkdir -p $@
>> -
>> -build/rootfs.erofs: ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(addprefix image/,$(FILES)) $(VM_BUILD_FILES) build/empty build/fifo file-list.mk
>> -	( \
>> -	    cat $(PACKAGES_FILE) ;\
>> -	    for file in $(FILES) $(LINKS); do printf 'image/%s\n%s\n' $$file $$file; done ;\
>> -	    for file in $(VM_BUILD_FILES); do printf '%s\n%s\n' $$file $${file#build/}; done ;\
>> -	    printf 'build/empty\n%s\n' $(VM_DIRS) ;\
>> -	    printf 'build/fifo\n%s\n' $(VM_FIFOS) ;\
>> -	) | ../../scripts/make-erofs.sh $@
>> -
>> -build/etc/s6-rc: $(addprefix image/,$(S6_RC_FILES)) file-list.mk
>> -	mkdir -p $$(dirname $@)
>> -	rm -rf $@
>> -
>> -	dir=$$(mktemp -d) && \
>> -	    tar -C image -c $(S6_RC_FILES) | tar -C $$dir -x --strip-components 2 && \
>> -	    s6-rc-compile $@ $$dir; \
>> -	    exit=$$?; rm -r $$dir; exit $$exit
>> +BUILD_FILES =
>>  
>>  debug:
>>  	$(GDB) -q \
>> @@ -138,14 +113,5 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd
>>  	    $(KERNEL)
>>  .PHONY: run-crosvm
>>  
>> -update-file-list:
>> -	../../scripts/genfiles.awk image > file-list.mk
>> -
>> -.PHONY: update-file-list
>> -
>>  run: run-$(VMM)
>>  .PHONY: run
>> -
>> -clean:
>> -	rm -rf build
>> -.PHONY: clean
>> diff --git a/img/app/default.nix b/img/app/default.nix
>> index d3eed1f0accdc8968d1ba5bdec74ab597789082f..cf10e712ab84a9e85cb1373024be5f64deef0370 100644
>> --- a/img/app/default.nix
>> +++ b/img/app/default.nix
>> @@ -107,6 +107,7 @@ stdenvNoCC.mkDerivation {
>>      fileset = lib.fileset.intersection src (lib.fileset.unions [
>>        ./.
>>        ../../lib/common.mk
>> +      ../../lib/erofs.mk
>>        ../../scripts/make-erofs.sh
>>        ../../scripts/make-gpt.sh
>>        ../../scripts/sfdisk-field.awk
>> diff --git a/lib/erofs.mk b/lib/erofs.mk
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..b3fc112f5e793725977cd8c4b2e71d6ed8d888c4
>> --- /dev/null
>> +++ b/lib/erofs.mk
>> @@ -0,0 +1,51 @@
>> +override basedir ::= $(dir $(lastword $(MAKEFILE_LIST)))/..

This is one part that is not possible in POSIX make.  Without
this, there is no way for the included makefile to know where
its wrapper scripts are.

>> +override BUILD_FILES ::= $(BUILD_FILES) build/etc/s6-rc
>> +# No override here so that it can be overridden in host/rootfs/default.nix.
>> +dest ::= build/rootfs.erofs
>> +
>> +all:
>> +.PHONY: all
>> +$(dest): $(basedir)/scripts/make-erofs.sh $(PACKAGES_FILE) $(addprefix image/,$(FILES)) $(BUILD_FILES) build/empty build/fifo file-list.mk
>> +	set -euo pipefail; ( \
>> +	    cat $(PACKAGES_FILE) ;\
>> +	    for file in $(FILES) $(LINKS); do printf 'image/%s\n%s\n' "$$file" "$$file"; done ;\
>> +	    for file in $(BUILD_FILES); do printf '%s\n%s\n' "$$file" "$${file#build/}"; done ;\
>> +	    $(and $(DIRS),printf 'build/empty\n%s\n' $(DIRS);)\
>> +	    $(and $(FIFOS),printf 'build/fifo\n%s\n' $(FIFOS);)\

This also has no analog in POSIX make, though it might be possible
to emulate it with some shell hacks.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]