build(agd): use timestamps again; hashes weren't necessary #9179

michaelfig · 2024-04-01T19:11:41Z

refs: #8715
(ostensibly) closes: #8996

Description

Recommendation: Best viewed with hide whitespace changes.

The build hashes introduced in #8715 didn't actually solve the # endo-branch: XXX problem that it was designed to, the more careful handling of the Endo-based packages and caches in #8715 did instead.

The complexity introduced by trying to use build hashes is not worth the cost, so this PR both simplifies and makes the original timestamp-based builds more robust.

Security Considerations

n/a

Scaling Considerations

Simpler to understand and more efficient to execute agoric-sdk/bin/agd.

As a drive-by, reuse longer yarn install timeouts, so that the QEMU arm64 Docker builds succeed consistently again. This is a temporary fix until Agoric-SDK uses a solution for faster multiplatform builds (like #9043).

Documentation Considerations

n/a

Testing Considerations

Build process and CI tests should be adequate for this PR.

Upgrade Considerations

n/a

turadg · 2024-04-01T20:54:57Z

bin/agd

-  fi
-  diff -u "$stamp" "$stamp.new" || return 1
-  return 0
+	echo ${1+"$@"} 1>&2


the indentation changed here. time for .editorconfig?

I thought shfmt -w could do no wrong!

I discovered recently it needs --indent 4 in our codebase. I just checked and it ignores .editorconfig.

I can add a .sh format check to CI if you think it's worthwhile.

A format check would be welcome, but of lower priority. I don't mind doing manual shfmt for a little while.

please use the formatting of .sh scripts. CI isn't checking it because the script doesn't end in .sh. Consider renaming the file go agd-with-build.sh or something like that and having bin/agd symlink to it.

For the reformatting, please have the base reformat in a separate commit before your changes. I did that in https://github.com/Agoric/agoric-sdk/tree/ta-agd-build-fix if you want to use those

a symlink might not be compatible with tooling in the cosmovisor ecosystem

A symlink is not compatible, but a tiny redirecting shell script is, so I did that.

packages/deployment/Dockerfile.sdk

kriskowal

I appreciate the simplification and trust CI exercises this. From this review, my knowledge of bash grew from “more than I would like” to “even more than I would like”.

golang/cosmos/Makefile

kriskowal · 2024-04-09T02:47:32Z

bin/agd

-  fi
-  diff -u "$stamp" "$stamp.new" || return 1
-  return 0
+    echo ${1+"$@"} 1>&2


What does this do?

It echoes the fatal arguments to stderr with proper escaping.

bin/agd

turadg · 2024-04-09T15:08:56Z

.github/actions/restore-node/action.yml

+        STAMPS=node_modules/.cache/agoric
+        mkdir -p "$STAMPS"


glad to see CI less coupled to the agd script implementation.

can't we remove this too? The script already defines STAMPS so it just needs mkdir. Did you avoid that due to cross-platform concerns? We only support Mac and Linux and I think mkdir -p works in both. Then you can just have the script mkdir idempotently.

Just some detritus... I've removed it.

turadg · 2024-04-09T15:10:01Z

bin/agd

+    set -x
+    case "${2-''}" in
+    --force | -f)
+        rm -rf "$thisdir/../$STAMPS"


this will remove all agoric cache. shouldn't it remove just the files in its purview?

Now I've isolated the stamps in a subdirectory so that the whole cache isn't deleted.

turadg · 2024-04-09T15:21:49Z

bin/agd

+            yarn "$@"
+        }
+
+        # Check if any package.json is newer than the installation stamp.


This is a really broad scope for agd. I don't expect a tool that I run to do yarn install or yarn scripts. This also unnecessarily couples agd to a particular package manager.

Not a blocker but a discussion I'd like to start. Can we leave package management to the package manager? I just started #9209 to track

Suggested change

# Check if any package.json is newer than the installation stamp.

# Check if any package.json is newer than the installation stamp.

# If some package.json is newer than the last yarn install, run yarn install

# UNTIL https://github.com/Agoric/agoric-sdk/issues/9209

turadg · 2024-04-09T15:24:13Z

package.json

+    "build": "yarn workspaces run build && mkdir -p node_modules/.cache/agoric && date >node_modules/.cache/agoric/yarn-built",
+    "postinstall": "patch-package && mkdir -p node_modules/.cache/agoric && date >node_modules/.cache/agoric/yarn-installed",


this is quite a bit of boilerplate for package.json. please have a helper script.

Suggested change

"build": "yarn workspaces run build && mkdir -p node_modules/.cache/agoric && date >node_modules/.cache/agoric/yarn-built",

"postinstall": "patch-package && mkdir -p node_modules/.cache/agoric && date >node_modules/.cache/agoric/yarn-installed",

"build": "yarn workspaces run build && scripts/timestamp.sh yarn-built",

"postinstall": "patch-package && scripts/timestamp.sh yarn-installed",

I wonder if there's a little npm package for reading and writing cached values or comparing timestamps with a cached value on disk.

I adopted this in scripts/agd-builder.sh.

turadg

👍

turadg · 2024-04-10T18:43:36Z

scripts/agd-builder.sh

+# Run the built Cosmos daemon.
+# shellcheck disable=SC2031
+export PATH="$thisdir/../packages/cosmic-swingset/bin:$PATH"
+exec "$thisdir/../$GOLANG_DAEMON" ${1+"$@"}


consider moving this to bin/agd so this builder script's only job is to build

Oh, good idea!

michaelfig added tooling repo-wide infrastructure agd Agoric (Golang) Daemon labels Apr 1, 2024

michaelfig self-assigned this Apr 1, 2024

michaelfig force-pushed the mfig-agd-build-fix branch from 2c39d9c to 27ca21e Compare April 1, 2024 19:12

michaelfig added automerge:no-update (expert!) Automatically merge without updates force:integration Force integration tests to run on PR and removed automerge:no-update (expert!) Automatically merge without updates labels Apr 1, 2024

michaelfig requested review from mhofman, turadg and kriskowal and removed request for turadg April 1, 2024 20:05

michaelfig marked this pull request as ready for review April 1, 2024 20:06

turadg reviewed Apr 1, 2024

View reviewed changes

packages/deployment/Dockerfile.sdk Show resolved Hide resolved

michaelfig force-pushed the mfig-agd-build-fix branch from 55d271f to 721452d Compare April 1, 2024 22:01

turadg mentioned this pull request Apr 1, 2024

format .sh #9183

Merged

michaelfig force-pushed the mfig-agd-build-fix branch from 721452d to 2ab96b0 Compare April 9, 2024 02:24

kriskowal approved these changes Apr 9, 2024

View reviewed changes

turadg reviewed Apr 9, 2024

View reviewed changes

michaelfig force-pushed the mfig-agd-build-fix branch from 2ab96b0 to 393cd47 Compare April 10, 2024 18:37

turadg approved these changes Apr 10, 2024

View reviewed changes

michaelfig force-pushed the mfig-agd-build-fix branch from 393cd47 to 2a091ad Compare April 10, 2024 18:44

michaelfig and others added 6 commits April 10, 2024 13:13

ci(build): reinstate longer yarn install timeouts

2fc82e5

build(cosmos): avoid needless go

9647671

chore(agd): rename to scripts/agd-builder.sh

ee6c21d

chore(agd): tiny jumper to scripts/agd-builder.sh

76bcc12

style: format agd-builder.sh with prettier

0ada7fe

build(agd): use timestamps again; hashes weren't necessary

233180d

michaelfig force-pushed the mfig-agd-build-fix branch from 2a091ad to c0ce234 Compare April 10, 2024 19:13

fix(agd): make agd-builder.sh only build

ff3b455

michaelfig force-pushed the mfig-agd-build-fix branch from c0ce234 to ff3b455 Compare April 10, 2024 19:21

michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Apr 10, 2024

mergify bot merged commit 00cccfc into master Apr 10, 2024
74 checks passed

mergify bot deleted the mfig-agd-build-fix branch April 10, 2024 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(agd): use timestamps again; hashes weren't necessary #9179

build(agd): use timestamps again; hashes weren't necessary #9179

michaelfig commented Apr 1, 2024 •

edited

Loading

turadg Apr 1, 2024

michaelfig Apr 1, 2024

turadg Apr 1, 2024

michaelfig Apr 1, 2024

turadg Apr 9, 2024

mhofman Apr 9, 2024

michaelfig Apr 10, 2024

kriskowal left a comment

kriskowal Apr 9, 2024

michaelfig Apr 10, 2024

turadg Apr 9, 2024

michaelfig Apr 10, 2024

turadg Apr 9, 2024

michaelfig Apr 10, 2024

turadg Apr 9, 2024

turadg Apr 9, 2024

michaelfig Apr 10, 2024

turadg left a comment

turadg Apr 10, 2024

michaelfig Apr 10, 2024

		"build": "yarn workspaces run build && mkdir -p node_modules/.cache/agoric && date >node_modules/.cache/agoric/yarn-built",
		"postinstall": "patch-package && mkdir -p node_modules/.cache/agoric && date >node_modules/.cache/agoric/yarn-installed",

build(agd): use timestamps again; hashes weren't necessary #9179

build(agd): use timestamps again; hashes weren't necessary #9179

Conversation

michaelfig commented Apr 1, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

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

kriskowal 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig commented Apr 1, 2024 •

edited

Loading