build(agd): rely on checksums, not timestamps

[michaelfig]

refs: #8136

Description

Don't use timestamps to track the build dependencies in the bin/agd script and restore-node/action.yml... they get confused because Git doesn't preserve the timestamps. Instead, use sha1sum to generate simple manifests of the dependencies matched to their hashes, and compare those manifests to decide if something needs rebuilding.

This solves a longstanding problem with the #endo-branch: PR feature... it would fail during cosmic-swingset tests since those would be fooled into rerunning the yarn install but without the Endo-overriding packages. With the new manifest-based builds, bin/agd is much more robust.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

[erights]

@michaelfig , this seems like it would be a big help immediately, seems done, small, and thus possibly quickly reviewable by those with the relevant knowledge. Could you move this forward? Thanks.

[michaelfig]

I pinged y'all as reviewers because it's both CI and shell scripting. Feel free to review or ignore as you feel most comfortable.

[kriskowal]

Thank you!

[kriskowal]

What is this notation? Is this mustache interpolation?

[michaelfig]

Yup, or something like moustache.

[kriskowal]

TIL cmp

[kriskowal]

Could also avoid the temporary file: <(git cat-file blob HEAD:package.json)

[michaelfig]

Nice!

[kriskowal]

This might be read more clearly with information piping strictly left to right

Suggested change

	while IFS= read -r line; do
	files+=( "$line" )
	done < <(yarn -s workspaces info |
	sed -ne '/"location":/{ s/.*": "//; s!",.*!/package.json!; p; }')
	yarn -s workspaces info |
	sed -ne '/"location":/{ s/.*": "//; s!",.*!/package.json!; p; }' |
	while IFS= read -r line; do
	files+=( "$line" )
	done

Is there no other idiom to self-concat the bash array? while subshells for every iteration. I don’t expect that’s a problem, but I have a blind spot for bash arrays.

[kriskowal]

Did some fiddling locally and it seems that this should work:

file+=( $(
  yarn -s workspaces info |
  sed -ne '/"location":/{ s/.*": "//; s!",.*!/package.json!; p; }'
) )

That said, I’m weary of bash arrays because it’s hard to predict when they’re going to compose poorly with the command line length limit. File variable concatenation would look like:

FILES=$(git hash-object -w /dev/null)
FILES=$( git hash-object -w <(
  git cat-file blob "$FILES"
  yarn -s workspaces info |
  sed -ne '/"location":/{ s/.*": "//; s!",.*!/package.json!; p; }'
))

[michaelfig]

while subshells for every iteration

That is surprising to me. Even when read is a builtin that has to set a variable in its caller?

[michaelfig]

I use a bash array because I don't trust the escaping (or lack thereof) of other subcommands. I'd like this script to match bin/agd, which doesn't currently require Git.

[kriskowal]

I’ve had trouble with while accumulators in the past. Yeah, verified:

numbers=( 0 )
seq 10 | while read -r number; do
  numbers+=( $numbers "$number" )
done
echo "${numbers[*]}"
# 0

[michaelfig]

It's the pipe that starts the subprocess, not the while:

numbers=( 0 )
while read -r number; do
  numbers+="$numbers $number";
done < <(seq 10)
echo "${numbers[*]}"

results in:

00 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 600 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 6 700 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 600 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 6 7 800 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 600 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 6 700 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 600 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 6 7 8 900 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 600 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 6 700 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 600 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 6 7 800 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 600 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 6 700 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 600 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 500 100 1 200 100 1 2 300 100 1 200 100 1 2 3 400 100 1 200 100 1 2 300 100 1 200 100 1 2 3 4 5 6 7 8 9 10

[kriskowal]

Ahah! Thank you. That makes sense and rationalizes the right to left flow!

[kriskowal]

In the sync, I fixed this in one place but not here. In the future, grepping for build:types and clean:types should be sufficient to be sure we update everywhere it’s needed, but I wonder if there’s anywhere we can leave a note for our future selves where we’d be likely to find it when we needed it. I don’t have a suggestion.

[JimLarson]

I think you want

Suggested change

	if cmp -s package.json.orig package.json; then
	if ! cmp -s package.json.orig package.json; then

Otherwise add the comment "if package.json is unchanged...".

[michaelfig]

Thanks. Good catch!

[JimLarson]

Would it make sense to normalize this to true in the "else" case? I see below that the value of the variable is being executed as a command to terminate a pipeline with the desired status.

[michaelfig]

While I don't want to force it to true, it does make sense to normalize it to an executable boolean (one of true or false). Thanks for the suggestion!

JimLarson approved the PR; he probably just forgot to dismiss this request for changes.