introduce vm-config package to fix local-npm test #8141

turadg · 2023-08-03T22:44:41Z

Description

#8102 broke the some agoric-cli integration tests.
(https://github.com/Agoric/agoric-sdk/actions/runs/5753924697/job/15598129426 )

#8140 helped but there was one problem left with resolving the configs.

This fixes that by separating configs to their own package.

It also adds a timeout and fixes a benchmark test missed in the 'boot' move.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

I don't think these are documented outside the repo.

Testing Considerations

Improves factoring

Upgrade Considerations

The configs are only used in bootstrap, not upgrade.

mhofman

Just a review of the GH action change

mhofman · 2023-08-03T22:46:56Z

.github/workflows/integration.yml

@@ -127,6 +127,7 @@ jobs:
      - name: run agoric-cli integration-test
        working-directory: ./packages/agoric-cli
        run: yarn integration-test
+        timeout-minutes: 40


Let's move this at the job level instead of this individual step

dckc

I think it's worth waiting on review by @michaelfig for approval.

Meanwhile, some observations / questions.

dckc · 2023-08-03T22:49:23Z

packages/vm-config/README.md

+
+The configs themselves have no dependencies. Keeping them separate allows packages to depend on them without depending on the world.
+
+# Future plans


Plans seem like they belong in issues. Or at least: when we say "we plan to X" I prefer that X is linked to an issue.

And historical info ("Factored out...") belongs in commit messages.

not firm enough plans to merit an issue, imo. if it can't be here I would just drop it.

I could qualify this with "possible future directions"

if it can't be here

It's not a critical thing. I just wonder about it getting out of date. I get the impression that it's likely to be addressed soon anyway.

dckc · 2023-08-03T22:54:02Z

packages/vm-config/package.json

+    "lint": "run-s --continue-on-error lint:*",
+    "lint:types": "tsc -p jsconfig.json",
+    "lint:eslint": "eslint ."


Our docs on making a new package say:

populate a new packages/zoe/package.json, using other packages as a template

Is there any more to it than that? I'm sometimes overwhelmed wondering which other packages to use as a template.

Those docs are a little stale. e.g. we don't have to manually add to workspaces now because it has a globl.
I'll try to update in this PR.

as a template I used boot/packages.json (i actually copied the whole dir then cut it down)

separated #8142

dckc · 2023-08-03T22:57:01Z

packages/vm-config/README.md

+
+This is similar to `@agoric/boot` but because that has the integration testing of bootstrap, it depends on almost everything.
+
+The configs themselves have no dependencies. Keeping them separate allows packages to depend on them without depending on the world.


This seems like useful justification.

But I wonder to what extent it's true. The configs do reference other packages:

"coreProposals": [ "@agoric/vats/scripts/init-core.js", "@agoric/vats/scripts/init-network.js", { "module": "@agoric/inter-protocol/scripts/init-core.js",

yeah… in a way it's not true. It's only true in the way that you can get the package without any other dependencies.

thinking more, this is fully true. the configs have no dependencies. the runtime that wants to build or execute those configs does.

often takes 20min in production. allow 40 for future cases (e.g. Endo)

it was just for moving the github action value, which is done

michaelfig

Just one missing dependency from @agoric/inter-protocol to @agoric/vm-config, but that's easy to fix even if you land this PR first.

michaelfig · 2023-08-06T22:38:42Z

packages/inter-protocol/scripts/start-local-chain.sh

@@ -55,7 +55,7 @@ make scenario2-setup-nobuild >>"$CHAIN_LOG" 2>&1
 echo "Starting the chain..."
 # use -economy target to get the kitchen sink
 # disable pruning to keep all history https://docs.desmos.network/fullnode/overview/
-make AGC_START_ARGS="--pruning=nothing" CHAIN_BOOTSTRAP_VAT_CONFIG=@agoric/boot/decentral-itest-vaults-config.json scenario2-run-chain >>"$CHAIN_LOG" 2>&1 &
+make AGC_START_ARGS="--pruning=nothing" CHAIN_BOOTSTRAP_VAT_CONFIG=@agoric/vm-config/decentral-itest-vaults-config.json scenario2-run-chain >>"$CHAIN_LOG" 2>&1 &


Does this (and the reference in test-proposal-stuff.js) mean that inter-protocol/package.json should take a dependency on @agoric/vm-config?

I don't think so, because this is instructing cosmic-swingset what to do. That's the package that really has the dependency. Same as how the JSONs in vm-config that specify a slate of packages doesn't itself depend on them (the thing that reads their source does).

Also this script is not long for this world #7914

introduce vm-config package to fix local-npm test

turadg requested review from dckc and michaelfig August 3, 2023 22:44

mhofman previously requested changes Aug 3, 2023

View reviewed changes

dckc reviewed Aug 3, 2023

View reviewed changes

turadg force-pushed the fix-local-npm-test branch from 8c99191 to b50f7a9 Compare August 4, 2023 13:22

turadg mentioned this pull request Aug 4, 2023

update docs for adding a package #8142

Merged

turadg added 2 commits August 4, 2023 11:36

ci: timeout agoric-cli integration-test

c3b3b5f

often takes 20min in production. allow 40 for future cases (e.g. Endo)

test: use importMetaResolve

d337987

turadg force-pushed the fix-local-npm-test branch from 23ba708 to cfad735 Compare August 4, 2023 19:38

turadg added 5 commits August 4, 2023 15:47

feat: vm-config package

8b1ecad

test: move vats benchmark to 'boot'

440e62e

build: don't publish agoric/boot

97d4ec5

ci: fix lint

64a1b66

style: for loop

b22eef1

turadg force-pushed the fix-local-npm-test branch from cfad735 to b22eef1 Compare August 4, 2023 20:11

turadg requested review from mhofman and dckc August 4, 2023 21:54

michaelfig approved these changes Aug 6, 2023

View reviewed changes

turadg added this pull request to the merge queue Aug 7, 2023

Merged via the queue into master with commit a2db142 Aug 7, 2023
62 checks passed

turadg deleted the fix-local-npm-test branch August 7, 2023 13:57

mhofman pushed a commit that referenced this pull request Aug 7, 2023

Merge pull request #8141 from Agoric/fix-local-npm-test

3636cd1

introduce vm-config package to fix local-npm test

mhofman mentioned this pull request Sep 21, 2023

Publishable NPM packages for mainnet #8360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce vm-config package to fix local-npm test #8141

introduce vm-config package to fix local-npm test #8141

turadg commented Aug 3, 2023

mhofman left a comment

mhofman Aug 3, 2023

dckc left a comment

dckc Aug 3, 2023

turadg Aug 3, 2023

dckc Aug 4, 2023

dckc Aug 3, 2023

turadg Aug 3, 2023

turadg Aug 4, 2023

dckc Aug 3, 2023

turadg Aug 3, 2023

turadg Aug 4, 2023

michaelfig left a comment

michaelfig Aug 6, 2023 •

edited

Loading

turadg Aug 7, 2023


		The configs themselves have no dependencies. Keeping them separate allows packages to depend on them without depending on the world.

		# Future plans


		This is similar to `@agoric/boot` but because that has the integration testing of bootstrap, it depends on almost everything.

		The configs themselves have no dependencies. Keeping them separate allows packages to depend on them without depending on the world.

introduce vm-config package to fix local-npm test #8141

introduce vm-config package to fix local-npm test #8141

Conversation

turadg commented Aug 3, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc 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

michaelfig left a comment

Choose a reason for hiding this comment

michaelfig Aug 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig Aug 6, 2023 •

edited

Loading