Re: [PATCH v3] Generate file lists from a script

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

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

On 9/21/25 04:47, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
> 
>> Right now, the makefiles in host/rootfs, vm/sys/net, and img/app have
>> manually-maintained lists of files and symlinks.  These duplicate the
>> information in the git repository and can easily get out of sync or
>> cause unnecessary merge conflicts.  Fix all of these issues by having
>> the git repository be the source of truth, and using a script to
>> generate the file lists.  Developers can regenerate the lists before
>> every commit, or even add a git hook to do that.
>>
>> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
>> ---
> 
> I like where this is going. :)

Yay!

>> Changes in v3:
>> - Only include the file list generator.  Move the rest to separate patch
>>   series.
>> - Remove the update-file-list make targets from img/app/Makefile and
>>   vm/sys/net/Makefile.
>> - Link to v2: https://lore.kernel.org/r/20250910-genfiles-v2-0-37ebe07a3cdc@gmail.com
>>
>> Changes in v2:
>> - Drop the last patch (switching to /etc/s6-rc/compiled) as it is
>>   controversial and should be reviewed separately.
>> - Add missing copyright notices.
>> - Use a wrapper shell script to make the awk code easier to read.
>> - Improve documentation.
>> - Add helper scripts for use in git hooks and rebasing.
>> - Link to v1: https://spectrum-os.org/lists/archives/spectrum-devel/20250903-genfiles-v1-0-cc993fcb1e4c@gmail.com/
>> ---
>>  Documentation/development/built-in-vms.adoc |  17 ++++
>>  host/rootfs/Makefile                        | 102 +----------------------
>>  host/rootfs/file-list.mk                    |  99 +++++++++++++++++++++++
>>  img/app/Makefile                            |  80 +++----------------
>>  img/app/file-list.mk                        |  65 +++++++++++++++
>>  lib/common.mk                               |   1 +
>>  scripts/genfiles.awk                        | 120 ++++++++++++++++++++++++++++
>>  scripts/genfiles.sh                         |  29 +++++++
>>  scripts/git-rebase                          |  17 ++++
>>  scripts/pre-commit.sh                       |  11 +++
> 
> Let's take git-rebase and pre-commit.sh out of this patch, and focus on
> the generated file lists first.

Will change.

>>  vm/sys/net/Makefile                         |  50 ++----------
>>  vm/sys/net/file-list.mk                     |  42 ++++++++++
>>  12 files changed, 422 insertions(+), 211 deletions(-)
>>
>> diff --git a/Documentation/development/built-in-vms.adoc b/Documentation/development/built-in-vms.adoc
>> index e90009ee5a3c2c254a7ae11e36121576b819eee7..0addc7d1a2fd322fa12918656baa3d169478504d 100644
>> --- a/Documentation/development/built-in-vms.adoc
>> +++ b/Documentation/development/built-in-vms.adoc
> 
> Copyright header please!

Will fix.  Also, in the future you have permission to fix missing copyright
headers when you commit.  It's fine if you aren't comfortable doing that.

>> @@ -44,6 +44,23 @@ NOTE: As a special convenience, it's not necessary to run `make clean`
>>  if the only change to the Nix files is modifying the packages
>>  installed in the VM.
>>  
>> +The list of files used for the VM image is stored in a separate file,
>> +`file-lists.mk`.  To update it, run `scripts/genfiles.sh`
> 
> Typo: file-list*s*.mk.  Also, so far we haven't used code syntax for
> file names.
> 
> Maybe "used for images" would be better, since this also applies to
> host/rootfs.  (Obviously the ideal would be if this documentation wasn't
> only written for VM images but that's out of scope.  We'll get to it.)

Will fix.

>> +which will regenerate it from the output of `git ls-files`.  Any
>> +changes you made will be lost.  This script uses uses Git's index to
> 
> I think "Any changes you made will be lost." is a bit scary, because
> it's not clear it only means changes to those files.  The sentence could
> probably just be dropped altogether — I think it's implied by "regenerate".

Will fix.

>> +generate the list, so you need to use `git add`, `git rm`, and `git mv`
>> +to ensure that Git knows about your changes.  It is not necessary to
>> +commit the changes.
> 
> "so only staged changes will be reflected"?  All the extra stuff has
> potential for confusion I think — for example "It is not necessary to
> commit the changes." could be read as "when you make a commit, do not
> include changes to file-list.mk".

Will fix.

>> diff --git a/lib/common.mk b/lib/common.mk
>> index 277c3544036d9a9057f8ba4ad37fe2207548cc59..0a03ff440cc671264d2b859a2ae048db9252d047 100644
>> --- a/lib/common.mk
>> +++ b/lib/common.mk
>> @@ -1,5 +1,6 @@
>>  # SPDX-License-Identifier: EUPL-1.2+
>>  # SPDX-FileCopyrightText: 2021, 2023, 2025 Alyssa Ross <hi@alyssa.is>
>> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>>  
>>  BACKGROUND = background
>>  CPIO = cpio
> 
> Accident?

Yes.

>> diff --git a/scripts/genfiles.awk b/scripts/genfiles.awk
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..6fe327fd0a314d226dbce23854aa8f119e9c8f34
>> --- /dev/null
>> +++ b/scripts/genfiles.awk
>> @@ -0,0 +1,120 @@
>> +#!/usr/bin/env -S LC_ALL=C LANGUAGE=C awk -E
>> +# SPDX-License-Identifier: EUPL-1.2+
>> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>> +BEGIN {
>> +	RS = "\n";
>> +	FS = "\t";
>> +	file_count = 0;
>> +	symlink_count = 0;
>> +	rc_count = 0;
>> +	is_rc = 0;
>> +	exit_code = 0;
>> +	done = 0;
> 
> awk variables are implicitly initialized to 0 when you try to do
> arithmetic on an undefined variable, so no need for these.

GNU Awk can lint against that.  I used its lint mode because it also
warns against non-portable constructs.  Also, an undefined awk
variable used as an array subscript is treated as the empty string,
not 0, which could lead to confusion.

>> +	modes["120000"] = "symlink";
>> +	modes["040755"] = "directory";
>> +	modes["100644"] = "regular";
>> +	modes["100755"] = "regular";
>> +}
>> +
>> +function fail(msg, status) {
>> +	if (status ~ /^([1-9][0-9]?|1[0-9]{2}|2[0-4][1-9]|25[1-5])$/) {
>> +		exit_code = status;
>> +	} else {
>> +		exit_code = 1;
>> +		status = 1;
>> +	}
>> +	print ("FATAL: " msg) > "/dev/stderr";
>> +	exit status;
> 
> Do we ever want to exit something other than 1 from this function?

Nope.

>> +}
>> +done { fail("Junk after DONE", 1); }
>> +/^DONE$/ {
>> +	done = 1
>> +	next
>> +}
>> +
>> +# Make sure git produced valid output.
>> +!/^[0-7]{6}\t[ -~]+$/ {
>> +	fail("git ls-files produced invalid output", 1);
>> +}
>> +
> 
> This is very unlikely to happen, and if it does, it will be obvious from
> the diff.

Will drop.

>> +# Extract data from built-in variables.
>> +{
>> +	filename = $2;
>> +	raw_mode = $1;
>> +	# awk autocreates empty string entries if the key is invalid,
>> +	# but the code exits in this case so that is okay.
>> +	mode = modes[raw_mode];
>> +}
>> +
>> +# Another check for a git bug.
>> +filename ~ /^\/|((^|\/)\.{0,2}($|\/))/ {
>> +	fail("git ls-files output non-canonical or absolute path '" filename "'", 1);
>> +}
>> +
> 
> If there are git bugs, we will notice and report them.  We do not need
> to be the test suite for git here.

Okay, fair!

>> +filename ~ /[^[:alnum:]_.+@/-]/ {
>> +	fail("filename '" filename "' has forbidden characters", 1);
>> +}
>> +
>> +/\.license$/ {
>> +	if (raw_mode != "100644") {
>> +		fail("License file '" filename "' is executable or not regular file", 1);
>> +	}
>> +	next;
>> +}
> 
> This is also not really in scope for a script that does not care about
> license files.

Fair.  I will leave that to the reuse check.

>> +
>> +mode == "directory" { next }
> 
> Getting a directory from git ls-files would be sufficiently unexpected
> that I don't think we should treat it any differently from an
> unrecognized mode.

Will fix.

>> +
>> +filename ~ /^image\/etc\/s6-rc\// {
>> +	if (mode != "regular") {
>> +		fail("s6-rc-compile input '" filename "' isn't a regular file");
>> +	}
>> +	rc_count += 1;
>> +	rc_files[rc_count] = filename;
> 
> rc_files[rc_count++]
> 
> (will make it 0-indexed though so update the loops too)

I think this might break without explicit variable initialization.

>> +	next;
>> +}
>> +
>> +mode == "symlink" {
>> +	symlink_count += 1;
>> +	symlinks[symlink_count] = filename;
>> +	next;
>> +}
>> +
>> +mode == "regular" {
>> +	file_count += 1;
>> +	files[file_count] = filename;
>> +	next;
>> +}
>> +
>> +{ fail("File '" filename "' is not regular file, directory, or symlink (mode " raw_mode ")"); }
>> +
>> +END {
>> +	if (exit_code) {
>> +		exit exit_code;
>> +	}
>> +	if (!done) {
>> +		fail("Did not receive DONE line", 1);
>> +	}
>> +	printf ("# SPDX-License-Identifier: CC0-1.0\n" \
>> +	       "# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>\n" \
> 
> Okay, so, it's silly that this needs to have a copyright header on it at
> all, but since we have to have one to make reuse happy, I think it
> should be mine from 2021, because the comment about links is the closest
> thing to creative expression in here.

Will fix.

>> +	       "# Generated by scripts/genfile.sh.  Any changes will be overwritten.\n" \
>> +	       "FILES ::=") > out_file;
> 
> I note the change to ::=.  Do you think we should do that across the
> board in our Makefiles?

POSIX specifies ::= and it has better semantics in most cases, but I don't
know if the BSD makes implement it.  ::= causes the RHS to be expanded immediately,
so subsequent changes in variables referenced by it do not affect the LHS.

>> +	for (array_index = 1; array_index <= file_count; array_index += 1) {
>> +		printf " \\\n\t%s", files[array_index] > out_file;
>> +	}
>> +	printf ("\n\n" \
>> +"# These are separate because they need to be included, but putting\n" \
>> +"# them as make dependencies would confuse make.\n" \
>> +"LINKS ::=") > out_file;
>> +	for (array_index = 1; array_index <= symlink_count; array_index += 1) {
>> +		printf " \\\n\t%s", symlinks[array_index] > out_file;
>> +	}
>> +	printf "\n\nS6_RC_FILES ::=" > out_file;
>> +	for (array_index = 1; array_index <= rc_count; array_index += 1) {
>> +		printf " \\\n\t%s", rc_files[array_index] > out_file;
>> +	}
>> +	printf "\n" > out_file;
>> +	if (close(out_file)) {
>> +		print ("Cannot close output file: " ERRNO "\n") > "/dev/stderr";
>> +		exit 1;
>> +	}
>> +}
>> diff --git a/scripts/genfiles.sh b/scripts/genfiles.sh
>> new file mode 100755
>> index 0000000000000000000000000000000000000000..77a8d95e88b6851be9447698556efe4f1eab174b
>> --- /dev/null
>> +++ b/scripts/genfiles.sh
>> @@ -0,0 +1,29 @@
>> +#!/usr/bin/env -S LC_ALL=C LANGUAGE=C bash --
> 
> env -S is not portable, and I don't think anything here needs bash
> specifically.

$'\t' doesn't work with all shells, though I believe it is either
part of the current POSIX standard or will be added.  I'll use
/usr/bin/env bash, which breaks if the script is renamed to something
starting with '-'.

> We can set the locale variables after the script starts,
> because I don't think this wrapper script is going to do anything
> locale-specific. (And shouldn't they be C.UTF-8?)

The C locale is actually what I intended.  The script does not rely
on support for non-ASCII characters, and it does use the fact that
negated character classes match all bytes.  Admittedly, this will
only be needed if there is a git bug.

>> +set -euo pipefail
>> +unset output_file astatus
> 
> This is a bit overly defensive IMO.  Both of these variables are
> assigned before use, and if they weren't, the person making those
> changes would be very unlikely to not notice because they had those
> variables defined in their environment.

Fair!

>> +case $0 in
>> +(/*) cd "${0%/*}/..";;
>> +(*/*) cd "./${0%/*}/..";;
>> +(*) cd ..;;
>> +esac
> 
> Perhaps we could use git rev-parse --show-toplevel?

git ls-files doesn't have that option.

>> +for i in host/rootfs img/app vm/sys/net; do
>> +    output_file=$i/file-list.mk
>> +    {
>> +	git -C "$i" -c core.quotePath=true ls-files $'--format=%(objectmode)\t%(path)' -- image |
>> +	sort -t $'\t' -k 2
> 
> TIL sort -t and -k! 🤯
> 
>> +	echo DONE
> 
> Why do we need this?

To avoid producing any output file if the input is truncated.

>> +    } |
>> +    gawk -v "out_file=$output_file.tmp" -E scripts/genfiles.awk
> 
> Why not stdout?

The output file is created by awk so that it is only created if
nothing went wrong.

> And why gawk?  I didn't immediately notice anything
> non-POSIX, and as usual would prefer to stick to it.

POSIX does not specify -E.  I can use -f instead, though.

>> +    if [ -f "$output_file" ]; then
>> +	    # Avoid changing output file if it is up to date, as that
>> +	    # would cause unnecessary rebuilds.
>> +	    if cmp -s -- "$output_file.tmp" "$output_file"; then
>> +		    rm -- "$output_file.tmp"
>> +		    continue
>> +	    else
>> +		    astatus=$?
>> +		    if [ "$astatus" != 1 ]; then exit "$astatus"; fi
> 
> Could avoid the need for the variable and multiple ifs.  Up to you
> whether you prefer it:
> 
> set +e
> cmp -s -- "$output_file.tmp" "$output_file"
> set -e
> case $? in
> 0)
> 	rm -- "$output_file.tmp"
>         continue
>         ;;
> 1)
> 	;;
> *)
> 	exit $?
>         ;;
> esac

This might set $? to the return value of 'set -e' (0).  Whether or
not it actually does is at least not obvious from reading the code.

>> +	    fi
>> +    fi
>> +    mv -- "$output_file.tmp" "$output_file"
>> +done


-- 
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 --]