Force xsnap rebuild #9618

mhofman · 2024-06-30T08:36:18Z

closes: #9614
closes: #7012
refs: agoric-labs/xsnap-pub#49

Description

Wire a mechanism to force rebuilds of xsnap when some build environments change, or if the xsnap binary is found to be out of date through the embedded xsnap package version.

Moves the agd build check to depend on a version check implemented by the xsnap package, which now only depends on the package version that is dynamically inserted instead of an explicit magic string that wasn't getting updated.

#7012 (comment) expressed concerns with relying on the xsnap package version, however:

any changes to xsnap-pub or moddable submodules would result in a change to the checked in build.env file, which would be detected by lerna during the publish process. While a version bump would not apply for contributors of the sdk (aka not every xsnap change results in a version change), it will apply for anyone using published versions, even if the change is in submodules only.
By having the version check look up the expected version from the package.json file directly, we avoid having to modify both the package.json file and the check itself. Only the automatically generated single publish commit will trigger a forced rebuild, and satisfy the check on its own,
At the same time we remove the dependency of agd build onto the the internal structure of xsnap.

Security Considerations

Forces all validators to use the expected version of xsnap

Scaling Considerations

Does a few more checks when building xsnap, and causes full rebuilds in some cases where they might not be strictly necessary.

Documentation Considerations

Should all be transparent

Testing Considerations

Manually tested by touching the xsnap package version, or reverting just the release binary to a previous copy (or deleting altogether). If the binary is meddled with like this, agd build checks will fail, but a manual yarn build will fix. I thought this was better than transparently forcing a rebuild in that abnormal situation.

Upgrade Considerations

Smoother upgrades by validators

michaelfig · 2024-06-30T18:00:54Z

Inserting myself as a reviewer, since I'm intimately familiar with the current xsnap build mechanism.

michaelfig

Please address shell quoting and sed portability fixes. Otherwise, this looks really good! I'm glad to see your evolution of the build process.

michaelfig · 2024-06-30T18:05:39Z

packages/xsnap/build.env

@@ -1,4 +1,4 @@
 MODDABLE_URL=https://github.com/agoric-labs/moddable.git
 MODDABLE_COMMIT_HASH=f6c5951fc055e4ca592b9166b9ae3cbb9cca6bf0
 XSNAP_NATIVE_URL=https://github.com/agoric-labs/xsnap-pub
-XSNAP_NATIVE_COMMIT_HASH=2d8ccb76b8508e490d9e03972bb4c64f402d5135
+XSNAP_NATIVE_COMMIT_HASH=6e7161a3022dc40d70cbf49f02b07dbc5a10c013


I audited the differences between these two commits, and verified that the changes looked beneficial and non-breaking.

michaelfig · 2024-06-30T18:22:55Z

packages/xsnap/scripts/get_xsnap_version.sh

+esac
+
+# extract the xsnap package version from the long version printed by xsnap-worker
+./xsnap-native/xsnap/build/bin/${platform}/release/xsnap-worker -v | sed -e 's/^xsnap\s\+\([^\s]\+\)\s\+(XS\s\+[^)]\+)$/\1/g'


This uses sed features that aren't available under Darwin (\s and \+), which makes me suspect they're GNUish or otherwise non-POSIX (possibly just a newer POSIX than macOS). Can we use something more straightforward?

Also, please habitually quote shell variable expansions in things you'd expect to be a single token. (They're also not strictly necessary for vname=$anything or case $vname. Yes, shell quoting rules are really weird and have lots of corner cases.)

Suggested change

./xsnap-native/xsnap/build/bin/${platform}/release/xsnap-worker -v | sed -e 's/^xsnap\s\+$[^\s]\+$\s\+(XS\s\+[^)]\+)$/\1/g'

"./xsnap-native/xsnap/build/bin/${platform}/release/xsnap-worker" -v | sed -e 's/^xsnap *$[^( ]*$.*/\1/'

yeah, I got an error under linux (a docker container, under a macOS host):

node $ yarn build yarn run v1.22.22 $ yarn build:bin && yarn build:env $ if test -d ./test; then node src/build.js; else yarn build:from-env; fi ./scripts/get_xsnap_version.sh: 3: set: Illegal option -o pipefail sh: 1: [[: not found

If I change the script to use /bin/bash instead of /bin/sh, it stops complaining about -o pipefail, but I still get the [[: not found error.

In both cases, the exit code is zero, so somehow this [[: not found is not actually causing a failure, so in my environment yarn check-version isn't really enforcing the version. I double-checked this by modifying my package.json to tweak the version: property, then re-ran yarn check-version without first doing a yarn build, and the exit code was still zero. So I think the [[: not found error is inhibiting the intended check and giving a false positive result.

If I change the package.json script line from [[ to [ (and ]] to ]), it seems to work as intended. Is the double-[[ something to deal with quoting?

I'd suggest moving the comparison logic into a separate scripts/check_xsnap_version.js, which knows how to run xsnap as a child process, use a real RegExp to parse the result (avoiding sed and the tricky shell quoting), can sense whether $npm_package_version is missing (indicating someone is mistakenly running the script outside of a yarn context), and which can exit(1) internally. Basically, avoid using a shell for anything except launching a JS program :).

sed is indeed a pain, it doesn't support common regular expression features unless you get into non posix land. Anyway, I simplified the expression. It should still work without issues as long as we don't tweak the -v output.

scripts/agd-builder.sh

michaelfig · 2024-06-30T18:27:36Z

scripts/agd-builder.sh

-# the xsnap binary lives in a platform-specific directory
-unameOut="$(uname -s)"
-case "${unameOut}" in
-  Linux*) platform=lin ;;
-  Darwin*) platform=mac ;;
-  *) platform=win ;;
-esac
-
-# check the xsnap version against our baked-in notion of what version we should be using
-xsnap_version=$("${thisdir}/../packages/xsnap/xsnap-native/xsnap/build/bin/${platform}/release/xsnap-worker" -n)
-[[ "${xsnap_version}" == "${XSNAP_VERSION}" ]] || fatal "xsnap version mismatch; expected ${XSNAP_VERSION}, got ${xsnap_version}"
-


Glad to see this go! 💯

Well arguably it moved

cloudflare-workers-and-pages · 2024-06-30T23:59:43Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`2b53896`
Status:	✅ Deploy successful!
Preview URL:	https://0bb9313b.agoric-sdk.pages.dev
Branch Preview URL:	https://mhofman-9614-xsnap-rebuild.agoric-sdk.pages.dev

View logs

warner · 2024-07-01T00:21:26Z

packages/xsnap/src/build.js

+
+  const expectedConfigEnvs = configEnvs.concat('').join('\n');
+  if (forceBuild || existingConfigEnvs.trim() !== expectedConfigEnvs.trim()) {
+    await fs.writeFile(configEnvFile, expectedConfigEnvs);


nice, so we update build.config.env if it differs from the const configEnvs here in build.js, and build.config.env is added as a *.o dependency via EXTRA_DEPS, so modifying most of src/build.js won't cause a rebuild, but modifying the -D__has_builtin or the XSNAP_VERSION value will.

Or if forceBuild is passed, which is if we detect a version mismatch, in case someone mucked around with the binary directly

warner · 2024-07-01T00:26:59Z

packages/xsnap/scripts/get_xsnap_version.sh

@@ -0,0 +1,14 @@
+#!/usr/bin/env bash


good enough, it assumes that bash is available, but I'll admit that's likely, and easier than figuring out how to write this in the purely sh subset

Well arguably this is already posix 2022 compliant, it's just that most shells still don't support -o pipefail in their posix mode.

I suppose I could check for the existence of the binary first before trying to invoke it

warner

confirmed that touch build.config.env && yarn build does a full rebuild
confirmed that subsequent yarn build does not rebuild
confirmed that yarn check-version fails (with rc=1) when I modify the package.json xsnap version:
confirmed: changing the -D__has_builtin in const configEnvs inside src/build.js, makes yarn build do a full build
confirmed: overwriting ./xsnap-native/xsnap/build/bin/lin/release/xsnap-worker with a stale version, causes yarn build to do a rebuild

Looks good to me.

closes: #9614 closes: #7012 refs: agoric-labs/xsnap-pub#49 ## Description Wire a mechanism to force rebuilds of xsnap when some build environments change, or if the xsnap binary is found to be out of date through the embedded xsnap package version. Moves the `agd build` check to depend on a version check implemented by the xsnap package, which now only depends on the package version that is dynamically inserted instead of an explicit magic string that wasn't getting updated. #7012 (comment) expressed concerns with relying on the xsnap package version, however: - any changes to `xsnap-pub` or `moddable` submodules would result in a change to the checked in `build.env` file, which would be detected by `lerna` during the publish process. While a version bump would not apply for contributors of the sdk (aka not every xsnap change results in a version change), it will apply for anyone using published versions, even if the change is in submodules only. - By having the version check look up the expected version from the `package.json` file directly, we avoid having to modify both the `package.json` file and the check itself. Only the automatically generated single publish commit will trigger a forced rebuild, and satisfy the check on its own, - At the same time we remove the dependency of `agd build` onto the the internal structure of xsnap. ### Security Considerations Forces all validators to use the expected version of xsnap ### Scaling Considerations Does a few more checks when building xsnap, and causes full rebuilds in some cases where they might not be strictly necessary. ### Documentation Considerations Should all be transparent ### Testing Considerations Manually tested by touching the xsnap package version, or reverting just the release binary to a previous copy (or deleting altogether). If the binary is meddled with like this, `agd build` checks will fail, but a manual `yarn build` will fix. I thought this was better than transparently forcing a rebuild in that abnormal situation. ### Upgrade Considerations Smoother upgrades by validators

Rebase todo ``` # Branch fix-vow-include-vat-js-in-package-files-9607- label base-fix-vow-include-vat-js-in-package-files-9607- pick b6ffa6f fix(vow): include vat.js in package files label fix-vow-include-vat-js-in-package-files-9607- reset base-fix-vow-include-vat-js-in-package-files-9607- merge -C a3826e9 fix-vow-include-vat-js-in-package-files-9607- # fix(vow): include vat.js in package files (#9607) # Pull Request #9601 pick 6bc363b fix(cosmos): only allow snapshot export at latest height (#9601) # Branch Force-xsnap-rebuild-9618- label base-Force-xsnap-rebuild-9618- pick 467435a fix(xsnap): force rebuild if build config changes pick a22772e fix(agd): check xsnap was rebuilt pick 2b53896 feat(xsnap): force rebuild if binary version mismatch label Force-xsnap-rebuild-9618- reset base-Force-xsnap-rebuild-9618- merge -C 78b6fec Force-xsnap-rebuild-9618- # Force xsnap rebuild (#9618) # Branch agd-enforce-Node-js-version-9623- label base-agd-enforce-Node-js-version-9623- #pick 4b35caf revert fix: typescript-estree does not support Node.js LTS (#9619) pick 5f01bef fix(agd): force own node.js version check label agd-enforce-Node-js-version-9623- reset base-agd-enforce-Node-js-version-9623- merge -C 4f70e66 agd-enforce-Node-js-version-9623- # agd enforce Node.js version (#9623) # Branch gibson-9623-followup label base-gibson-9623-followup pick 6102eb9 fix: Disallow Node.js major version >20 label gibson-9623-followup reset base-gibson-9623-followup merge -C caaec05 gibson-9623-followup # (upstream/master) fix: Disallow Node.js major version >20 (#9630) ```

mhofman requested review from warner and kriskowal June 30, 2024 08:36

mhofman added the force:integration Force integration tests to run on PR label Jun 30, 2024

michaelfig self-requested a review June 30, 2024 18:01

michaelfig approved these changes Jun 30, 2024

View reviewed changes

warner reviewed Jul 1, 2024

View reviewed changes

warner approved these changes Jul 1, 2024

View reviewed changes

mhofman added the automerge:rebase Automatically rebase updates, then merge label Jul 1, 2024

mhofman added 3 commits July 1, 2024 00:52

fix(xsnap): force rebuild if build config changes

467435a

fix(agd): check xsnap was rebuilt

a22772e

feat(xsnap): force rebuild if binary version mismatch

2b53896

mhofman force-pushed the mhofman/9614-xsnap-rebuild branch from ace552a to 2b53896 Compare July 1, 2024 00:52

mergify bot merged commit 78b6fec into master Jul 1, 2024
77 checks passed

mergify bot deleted the mhofman/9614-xsnap-rebuild branch July 1, 2024 01:25

mhofman mentioned this pull request Jul 2, 2024

Prevent validator from using mismatched agoric-sdk version #8471

Closed

gibson042 mentioned this pull request Jul 9, 2024

refactor(xsnap): Cleanup build.js #9674

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force xsnap rebuild #9618

Force xsnap rebuild #9618

mhofman commented Jun 30, 2024

michaelfig commented Jun 30, 2024

michaelfig left a comment

michaelfig Jun 30, 2024

michaelfig Jun 30, 2024

warner Jun 30, 2024

mhofman Jul 1, 2024

michaelfig Jun 30, 2024

mhofman Jun 30, 2024

cloudflare-workers-and-pages bot commented Jun 30, 2024 •

edited

Loading

warner Jul 1, 2024

mhofman Jul 1, 2024

warner Jul 1, 2024

mhofman Jul 1, 2024

warner left a comment

	./xsnap-native/xsnap/build/bin/${platform}/release/xsnap-worker -v \| sed -e 's/^xsnap\s\+\([^\s]\+\)\s\+(XS\s\+[^)]\+)$/\1/g'
	"./xsnap-native/xsnap/build/bin/${platform}/release/xsnap-worker" -v \| sed -e 's/^xsnap \([^( ]\).*/\1/'

Force xsnap rebuild #9618

Force xsnap rebuild #9618

Conversation

mhofman commented Jun 30, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

michaelfig commented Jun 30, 2024

michaelfig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Jun 30, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warner left a comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Jun 30, 2024 •

edited

Loading