Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(agoric-cli): sh->js port of agops-perf-smoketest with zx #6966

Closed
wants to merge 5 commits into from

Conversation

dckc
Copy link
Member

@dckc dckc commented Feb 10, 2023

refs: #2160, #6955

Description

Very literal port of agops-perf-smoketest.sh to JS using zx

Security Considerations

I hope this is a useful step toward OCap Discipline.

As a first step, ambient authority is localized to a folding region:

// #region ambient authority
import process from 'process';
import { $ } from 'zx';

const { env, argv: nodeArgv, exit } = process;
const echo = (...args) => console.log(...args);
// #endregion

Also, I find shell quoting rules frightening. Shellcheck and such can help, but we don't have that in CI. zx does much of that automatically:

Everything passed through ${...} will be automatically escaped and quoted.

We also rely on shell quoting mixed with make syntax more than I'm comfortable with in places like packages/cosmic-swingset/Makefile.

Scaling Considerations

This should allow us to gracefully push code down into modules and migrate it into things like the agoric CLI, or at least agops.

Documentation Considerations

This allows us to lean into our JS coding style, lint conventions, static typing, and such, though that's not without cost to folks more versed in shell scripting than JS.

Testing Considerations

Here's hoping this is also a step toward making such integration scripts run "lights out" in CI or the like.

cc @turadg @arirubinstein @michaelfig @erights @ivanlei @mhofman @gibson042 @warner

@dckc
Copy link
Member Author

dckc commented Feb 10, 2023

single-executable deployment?

further to #6455 / #6574 ... nexe seems to be a leading tool for packaging node apps as a single executable.

I tried it and it worked:

~/projects/agoric-sdk/packages/agoric-cli$ yarn nexe test/agops-perf-smoketest.js --remote "$NEXE_REMOTE"
yarn run v1.22.19
$ /home/connolly/projects/agoric-sdk/node_modules/.bin/nexe test/agops-perf-smoketest.js --remote https://github.com/urbdyn/nexe_builds/releases/download/0.2.0/
ℹ nexe 4.0.0-rc.2
✔ Downloading...100%
✔ Compiling result
✔ Entry: 'test/agops-perf-smoketest.js' written to: agops-perf-smoketest
✔ Finished in 3.075s
Done in 3.40s.

~/projects/agoric-sdk/packages/agoric-cli$ ./agops-perf-smoketest 
AGORIC_NET env not set

e.g. AGORIC_NET=ollinet (or export to save typing it each time)
...

Packaging seems pretty reasonable:

~/projects/agoric-sdk/packages/agoric-cli$ ls -lsh ./agops-perf-smoketest 
79M -rwxrwxr-x 1 connolly connolly 79M Feb 10 03:07 ./agops-perf-smoketest

~/projects/agoric-sdk/packages/agoric-cli$ ldd ./agops-perf-smoketest 
        linux-vdso.so.1 (0x00007ffea77ec000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f10eca23000)
        libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f10ec7f9000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f10ec712000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f10ec6f2000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f10ec6ed000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f10ec4c3000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f10eca3c000)

Native Modules: no free lunch

In order to use native modules, the native binaries must be shipped alongside the binary generated by nexe.

so life looks complicated for sqlite3 and such. sad trombone

And of course, in our golang/js integration, nodejs is linked as a library. There isn't really a .js entrypoint. #5287 to the rescue?

prep, bumps in the road

rough notes...

nexe/nexe#1031 (comment)

nvm install 16  # got me 16.9
nvm use 16
npm install -g yarn
yarn add --dev nexe

NEXE_REMOTE="https://github.com/urbdyn/nexe_builds/releases/download/0.2.0/"
yarn nexe test/agops-perf-smoketest.js --remote "$NEXE_REMOTE"

linux-x64-16.19.0
https://github.com/urbdyn/nexe_builds/releases

supply chain risks... build it ourselves...

@dckc
Copy link
Member Author

dckc commented Feb 10, 2023

zx doesn't support node 14

at least not the current version:

error zx@7.1.1: The engine "node" is incompatible with this module. Expected version ">= 16.0.0". Got "14.21.2"

-- https://github.com/Agoric/agoric-sdk/actions/runs/4142272715/jobs/7162749774#step:3:206

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node 14

Active support ended Oct 2021 and Security support ends 30 Apr 2023, a couple months. I think it's fine to couple adopting of zx with dropping Node 14.

However, I don't think we necessarily need to include zx as a devDependency if we use it as a shell env. The smoketest scripts don't run in CI and even if they were to we might want to just include zx from the Ubuntu install.

Here's hoping this is also a step toward making such integration scripts run "lights out" in CI or the like.

IMO this script isn't a good candidate for that. It's meant to be didactic and used for local debugging. If we want a CI test integrating agops I think that's worth writing fresh.

@@ -0,0 +1,53 @@
#!/usr/bin/env node
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not #!/usr/bin/env zx? That would save many imports. I suppose you'd prefer those imports for being explicit about ambient authority. What is the risk that you're trying to mitigate by that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OCap discipline isn't just about mitigating risk. It's about testability, auditability, modularity, reusability. In sum: good engineering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Engineering is all about trade-offs. One not in your list is velocity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it's good for velocity too, especially if done always from the beginning so you don't have to retrofit later.

// NOTE: USDC_axl = ibc/usdt1234
// perf test wantMinted
let OFFER = await $`mktemp -t agops.XXX`;
await $`bin/agops psm swap --wantMinted 0.01 --feePct 0.01 --pair IST.USDC_axl >|${OFFER}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gung ho about zx but there's a downside to JS syntax for for these smoketest scripts. I tend to run them interactively by copying lines over to my shell. This makes that harder to do.

For this script, I'm more inclined to keep the shell commands but place them into a Markdown file that zx would run. I want to make the prose primary and the commands incidental to executing the prose.

Even with that we'd get easier refactoring like extracting all the arg setup and diagnostics to a support function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this script isn't a good candidate for that [=lights out].

got it.

I tend to run them interactively by copying lines over to my shell.

I hope to avoid that. Very much not my style.

If we want a CI test integrating agops I think that's worth writing fresh.

This script does represent a product scenario that's worth lots of testing. The extent to which any automated test shares code with the copy-to-my-shell script, I don't have strong feelings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope to avoid that. Very much not my style.

Sounds worth a conversion to find a mutually agreeable design compatible with our workflows.

@dckc
Copy link
Member Author

dckc commented Mar 31, 2023

This experiment has run its course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants