Re: [PATCH v3 1/2] Build verity images in rootfs Nix derivation

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

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

On 11/13/25 06:46, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
> 
>> Avoid redundant rebuilds of the rootfs verity superblock and roothash.
>> Remove duplicate code.  Clean up Makefile to avoid temporary files.
>>
>> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
>> ---
>>  host/initramfs/Makefile  | 26 +++++---------------------
>>  host/initramfs/shell.nix |  4 +++-
>>  host/rootfs/Makefile     | 45 +++++++++++++++++++++------------------------
>>  host/rootfs/default.nix  |  2 +-
>>  host/rootfs/shell.nix    |  2 +-
>>  release/live/Makefile    | 26 +++++---------------------
>>  release/live/default.nix |  4 +++-
>>  7 files changed, 39 insertions(+), 70 deletions(-)
> 
> Looking good!

Thank you!

>> diff --git a/host/initramfs/Makefile b/host/initramfs/Makefile
>> index cb13fbb35f065b67d291d4a35591d6f12720060c..102870ecba4456303414e2531ea592473ddfc1cf 100644
>> --- a/host/initramfs/Makefile
>> +++ b/host/initramfs/Makefile
>> @@ -35,26 +35,10 @@ build/mountpoints:
>>  	cd build/mountpoints && mkdir -p $(MOUNTPOINTS)
>>  	find build/mountpoints -mindepth 1 -exec touch -d @0 {} ';'
>>  
>> -# veritysetup format produces two files, but Make only (portably)
>> -# supports one output per rule, so we combine the two outputs then
>> -# define two more rules to separate them again.
>> -build/rootfs.verity: $(ROOT_FS)
>> -	mkdir -p build
>> -	$(VERITYSETUP) format $(ROOT_FS) build/rootfs.verity.superblock.tmp \
>> -	    | awk -F ':[[:blank:]]*' '$$1 == "Root hash" {print $$2; exit}' \
>> -	    > build/rootfs.verity.roothash.tmp
>> -	cat build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp \
>> -	    > $@
>> -	rm build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp
>> -build/rootfs.verity.roothash: build/rootfs.verity
>> -	head -n 1 build/rootfs.verity > $@
>> -build/rootfs.verity.superblock: build/rootfs.verity
>> -	tail -n +2 build/rootfs.verity > $@
>> -
>> -build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk build/rootfs.verity.superblock build/rootfs.verity.roothash $(ROOT_FS)
>> +build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk $(ROOT_FS_VERITY) $(ROOT_FS_VERITY_ROOTHASH) $(ROOT_FS)
>>  	../../scripts/make-gpt.sh $@.tmp \
>> -	    build/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=build/rootfs.verity.roothash bs=32 skip=1 count=1 status=none)") \
>> -	    $(ROOT_FS):root:$$(../../scripts/format-uuid.sh "$$(head -c 32 build/rootfs.verity.roothash)")
>> +	"$$ROOT_FS_VERITY:verity:$$(../../scripts/format-uuid.sh "$$(dd "if=$$ROOT_FS_VERITY_ROOTHASH" bs=32 skip=1 count=1 status=none)")" \
> 
> Indentation got messed up here.
> 
> Given rootfs has a well-defined output structure, maybe we could just
> write $(ROOT_FS)/rootfs.verity.roothash, so we don't need to define lots
> of different environment variables in each component that uses the
> verity data.
> 
> I think we should consistently use Make variable expansion rather than
> shell variable expansion when we're using the variable in a Make
> dependency line too, to avoid the possibility of them being different.

Make expansion followed by shell expansion and just Make expansion aren't
even consistent with each other.

>> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile
>> index f107ca44fcf1ad85af5788d87f6c772910d40072..d7764d9b796f1773b4bebd0d50eec52b9be29e42 100644
>> --- a/host/rootfs/Makefile
>> +++ b/host/rootfs/Makefile
>> @@ -6,7 +6,7 @@
>>  include ../../lib/common.mk
>>  include file-list.mk
>>  
>> -dest = build/rootfs.erofs
>> +dest = build
>>  
>>  DIRS = \
>>  	dev \
>> @@ -46,15 +46,27 @@ 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) $(FILES) $(BUILD_FILES) build/empty build/fifo file-list.mk
>> -	set -euo pipefail; \
>> +# This rule produces three files but Make only (portably)
>> +# supports one output per rule.  Instead of resorting to temporary
>> +# files, a timestamp file is created as the last step.  The actual
>> +# outputs are produced as side-effects.
>> +$(dest)/timestamp: ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_FILES) build/empty build/fifo file-list.mk $(dest)
> 
> Semes a bit odd to install the timestamp in $(dest).
> 
>>  	{ \
>>  	    cat $(PACKAGES_FILE) ;\
>>  	    for file in $(FILES) $(LINKS); do printf '%s\n%s\n' $$file "$${file#image/}"; 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 $@
>> +	} | ../../scripts/make-erofs.sh $(dest)/rootfs
>> +	$(VERITYSETUP) format \
>> +		--root-hash-file $(dest)/rootfs.verity.roothash \
>> +		-- $(dest)/rootfs $(dest)/rootfs.verity.superblock
>> +	# Add trailing newline
>> +	echo >> $(dest)/rootfs.verity.roothash
>> +	touch -- $(dest)/timestamp
> 
> I prefer smaller independent Make rules where possible.  Can't the
> verity data stay in a separate rule from make-erofs.sh?
> 
> Either way, change deserves a copyright header IMO. :)
> 
>> +
>> +$(dest):
>> +	mkdir -p $(dest)
> 
> Would rather this was inlined into every target in $(dest), because
> directories can be a bit confusing to me as Make targets.  "Does the
> target just create the directory, or does it create the directory and
> everything in it?" sort of thing.

Will fix.
  >>  build/fifo:
>>  	mkdir -p build
>> @@ -83,25 +95,10 @@ clean:
>>  	rm -rf build
>>  .PHONY: clean
>>  
>> -# veritysetup format produces two files, but Make only (portably)
>> -# supports one output per rule, so we combine the two outputs then
>> -# define two more rules to separate them again.
>> -build/rootfs.verity: $(dest)
>> -	$(VERITYSETUP) format $(dest) build/rootfs.verity.superblock.tmp \
>> -	    | awk -F ':[[:blank:]]*' '$$1 == "Root hash" {print $$2; exit}' \
>> -	    > build/rootfs.verity.roothash.tmp
>> -	cat build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp \
>> -	    > $@
>> -	rm build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp
>> -build/rootfs.verity.roothash: build/rootfs.verity
>> -	head -n 1 build/rootfs.verity > $@
>> -build/rootfs.verity.superblock: build/rootfs.verity
>> -	tail -n +2 build/rootfs.verity > $@
>> -
>> -build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk build/rootfs.verity.superblock build/rootfs.verity.roothash $(dest)
>> +build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk $(dest)/timestamp
>>  	../../scripts/make-gpt.sh $@.tmp \
>> -	    build/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=build/rootfs.verity.roothash bs=32 skip=1 count=1 status=none)") \
>> -	    $(dest):root:$$(../../scripts/format-uuid.sh "$$(head -c 32 build/rootfs.verity.roothash)")
>> +	    $(dest)/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=$(dest)/rootfs.verity.roothash bs=32 skip=1 count=1 status=none)") \
>> +	    $(dest)/rootfs:root:$$(../../scripts/format-uuid.sh "$$(head -c 32 $(dest)/rootfs.verity.roothash)")
>>  	mv $@.tmp $@
>>  
>>  debug:
>> @@ -111,7 +108,7 @@ debug:
>>  	    $(VMLINUX)
>>  .PHONY: debug
>>  
>> -run: build/live.img $(EXT_FS) build/rootfs.verity.roothash
>> +run: build/live.img
> 
> What happened to $(EXT_FS)?

Since commit 12b64009d9cde56b5629a832086d2c2311908ebe
("host/initramfs/extfs.nix: remove") it has been unused.
I deleted it while changing other stuff in this line.
-- 
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 --]