Fully support `agoric install <dist-tag>` #4129

michaelfig · 2021-11-26T20:44:13Z

closes: #3857

Description

If a dist-tag is specified with agoric install <dist-tag>, ensure we refresh the dapp's yarn.lock dependencies.

This makes it possible to build and develop a published dapp without needing the agoric install command (just yarn install suffices). Indeed, npm install -g agoric or yarn global add agoric should be able to produce a working agoric command that can do everything that yarn link-cli ~/bin/agoric does, once this PR's version of agoric-cli has been published to NPM.

Security Considerations

Documentation Considerations

agoric install beta will update the workspace's yarn.lock with the beta dependencies. From that point on, installing the dapp only needs yarn install (though agoric install will work, too).

Testing Considerations

Tested manually on dapp-oracle.

dckc

I would need to level up my npm / yarn game to feel confident about this.

Maybe @kriskowal could take a look?

packages/agoric-cli/src/install.js

dckc · 2021-11-26T22:20:47Z

scripts/dist-tag.sh

@@ -0,0 +1,47 @@
+#! /bin/bash
+# dist-tag.sh - manage dist-tags for NPM registry packages 


elaborate a bit on "manage"?

I'm not really fluent in bash. I could use a comment for each clump of lines, actually.

dckc · 2021-11-26T22:23:46Z

packages/agoric-cli/src/install.js

        const pj = JSON.parse(packageJSON);
        for (const section of ['dependencies', 'devDependencies']) {
          const deps = pj[section];
          if (deps) {
            for (const pkg of Object.keys(deps)) {
              if (versions.has(pkg)) {
+                if (deps[pkg] === forceSdkVersion) {


I would have thought we need to "invalidate the cache" on a version mismatch.

I can't see where deps is initialized in this diff, and when I looked at the whole file, I got a little dizzy. (color me lazy)

I would have thought we need to "invalidate the cache" on a version mismatch.

Actually, yarn already updates packages that have changed. It's the packages whose version specifier has not changed that we need to first prune the old one, install, add the new one, and install to get it to update correctly.

dckc · 2021-11-26T22:28:52Z

Does this depend on #4120?

michaelfig · 2021-11-27T18:24:09Z

Does this depend on #4120?

Actually, no. It's still valuable even if someone has to manually publish packages and dist-tags all the time.

kriskowal

Everything in here looks like it does what it says it’s supposed to, but after a couple passes, I’m still fuzzy on why it works. Is the notion that install does a number of shallow installs and revises the package.jsons for the installed packages in-place? Is this recursive?

Would the effect be comparable to amending the dapp package.json to include resolutions for all the agoric-sdk packages using a particular dist-tag?

kriskowal · 2021-11-29T20:22:48Z

packages/agoric-cli/src/install.js

+        const pjson = `${subdir}/package.json`;
+        const packageJSON = await fs
+          .readFile(pjson, 'utf-8')
+          .catch(_ => undefined);
+        if (!packageJSON) {
+          return;
+        }
+        const pj = JSON.parse(packageJSON);
+        const prunedPj = { ...pj };


I struggle with naming variables that all refer to the same thing, but in different forms. My principle is that all the names should have some portion that is the same to reflect that they refer to something common, and a term that varies to indicate what’s different about them. So, for something like this, I’ve been using names like packageDescriptorBytes, packageDescriptorText, packageDescriptor, and packageDescriptorPruned. I think Json instead of Descriptor might be valid but not precise. I personally loathe the case convention conundrum raised by acronyms and initialisms, particularly when they run-on. And, I don’t mind some verbosity, even for local variables where the verbosity is unnecessary.

This is a recommendation.

kriskowal · 2021-11-29T22:32:51Z

packages/agoric-cli/src/install.js

+        if (needsPruning) {
+          pruningTodo.push(async () => {
+            // Update on exit, in case we are interrupted.
+            process.on('beforeExit', updatePackageJson);


I presume you considered and decided against framing this in terms of a finally. I don’t think it would occur to me to use the beforeExit event under any circumstances.

updatePackageJson returns a promise that the EventEmitter will ignore. That would suggest that any error will be realized as an unhandled rejection warning. I’d consider reframing this to sink the promise.

I presume you considered and decided against framing this in terms of a finally. I don’t think it would occur to me to use the beforeExit event under any circumstances.

It is framed in terms of a pruningP.finally(...). It's quite important that these updates are done even if the process is interrupted. Do you have a better mechanism at hand?

No, there’s nothing better in the face of uncaught exceptions.

kriskowal · 2021-11-29T22:35:58Z

packages/agoric-cli/src/install.js

+          // After all have settled, try throwing the first rejection.
+          const firstFailure = results.find(
+            ({ status }) => status !== 'fulfilled',
+          );
+          if (firstFailure) {
+            throw firstFailure.reason;
+          }


Perhaps consider using AggregateError. That will of course be impossible inside a Compartment for now, but I do not think this is inside a Compartment.

AggregateError is not available in Node v14. Do you have a better suggestion?

In the absence of AggregateError, I like to aggregate error messages, e.g.,

new Error(`Cannot x for multiple causes (${causes.map(x => x.message).join('; '))`)

But I won’t press the issue. The first cause is certainly sufficient and indeed all you’d see in a serial causal graph.

kriskowal · 2021-11-29T22:40:57Z

packages/agoric-cli/src/install.js

-    );
-  }
+      // This open-coding of the above yarn command is quiet and fast.
+      const linkName = `${linkFolder}/${pjName}`;


How do you feel about path.join? My current personal rule is not hard and fast either way.

Unless portability to older versions or Windows failures demand otherwise, I just use slashes and trust that Node.js's fs module figures out the rest.

kriskowal · 2021-11-29T22:41:44Z

scripts/npm-dist-tag.sh

+# Try: `npm-dist-tag.sh` for usage.
+
+# Exit on any errors.
+set -e


Please consider cargoing set -ueo pipefail as a matter of style.

kriskowal · 2021-11-29T22:47:13Z

scripts/npm-dist-tag.sh

@@ -0,0 +1,67 @@
+#! /bin/bash
+# npm-dist-tag.sh - manage dist-tags for NPM registry packages 


Perhaps add/remove/list is more evocative than manage.

kriskowal · 2021-11-29T22:48:33Z

scripts/npm-dist-tag.sh

+priv=$(jq -r .private package.json)
+case "$priv" in
+true)
+  echo 1>&2 "Private package, skipping npm-dist-tag.sh"


Wherein the reviewer relearns that redirects can be interpolated anywhere in a command.

kriskowal · 2021-11-29T22:50:56Z

scripts/npm-dist-tag.sh

+  ;;
+*)
+  # Usage instructions.
+  echo 1>&2 "Usage: $0 [lerna] <add|remove|list> [<tag>]"


Is the distinction that lerna means to apply in bulk to the workspaces? Would we ever not do that?

Is the distinction that lerna means to apply in bulk to the workspaces? Would we ever not do that?

Yes and yes, if we want to add/remove/list the dist-tags for just the package in the cwd.

kriskowal · 2021-11-29T23:03:13Z

packages/agoric-cli/src/install.js

-        }
+  // Create symlinks to the SDK packages.
+  await Promise.all(
+    dirPackages.map(async ([dir, pjName]) => {


Is this not equivalent to packages.entries()? Can these be consolidated?

kriskowal · 2021-11-29T23:05:05Z

packages/agoric-cli/src/install.js

+      Promise.allSettled(updatesTodo.map(async update => update())),
+    );
+  };
+


There’s a hazard on line 157 where a parse error will cause a reference error on 158.

michaelfig

Comments addressed to the best of my ability.

michaelfig · 2021-12-02T05:00:09Z

scripts/npm-dist-tag.sh

+  ;;
+*)
+  # Usage instructions.
+  echo 1>&2 "Usage: $0 [lerna] <add|remove|list> [<tag>]"


Is the distinction that lerna means to apply in bulk to the workspaces? Would we ever not do that?

Yes and yes, if we want to add/remove/list the dist-tags for just the package in the cwd.

michaelfig · 2021-12-02T05:02:50Z

scripts/npm-dist-tag.sh

+pkg=$(jq -r .name package.json)
+version=$(jq -r .version package.json)
+# echo "$OP $pkg@$version"
+
+# Get the second argument, if any.
+TAG=$2


I use TRAIN_CASE for environment variables and command-line arguments.

michaelfig · 2021-12-02T05:05:54Z

packages/agoric-cli/src/install.js

-    );
-  }
+      // This open-coding of the above yarn command is quiet and fast.
+      const linkName = `${linkFolder}/${pjName}`;


Unless portability to older versions or Windows failures demand otherwise, I just use slashes and trust that Node.js's fs module figures out the rest.

michaelfig · 2021-12-02T05:17:36Z

packages/agoric-cli/src/install.js

+          // After all have settled, try throwing the first rejection.
+          const firstFailure = results.find(
+            ({ status }) => status !== 'fulfilled',
+          );
+          if (firstFailure) {
+            throw firstFailure.reason;
+          }


AggregateError is not available in Node v14. Do you have a better suggestion?

michaelfig · 2021-12-02T05:19:57Z

packages/agoric-cli/src/install.js

+        if (needsPruning) {
+          pruningTodo.push(async () => {
+            // Update on exit, in case we are interrupted.
+            process.on('beforeExit', updatePackageJson);


I presume you considered and decided against framing this in terms of a finally. I don’t think it would occur to me to use the beforeExit event under any circumstances.

It is framed in terms of a pruningP.finally(...). It's quite important that these updates are done even if the process is interrupted. Do you have a better mechanism at hand?

michaelfig · 2021-12-02T05:57:43Z

Is the notion that install does a number of shallow installs

No, it handles the old-style dapp environment which is a top-level worktree and another worktree with its own yarn.lock in the ui directory. Typically the package.json are updated and then there is only one yarn install per worktree.

This works fine if the package.json versions don't already match, but yarn really doesn't want to reinstall a package that already appears in yarn.lock if it's semver-compatible.

When a forced version (like agoric install dev) is supplied, and a given package.json' already refers to it, the package.jsonupdates andyarn installabove are preceded by first removing the matching Agoric SDK packages from thepackage.jsons, then run a pruning yarn install` per worktree to clear out the old dependencies.

This is the fastest way to trim out the obsolete dependencies from both the workspaces' node_modules directories and the worktrees' yarn.locks, at a cost of at most 1-2 yarn installs per worktree. Other methods I investigated but threw out were:

yarn remove the packages one-by-one, then update the package.json and yarn install
prune the yarn.locks with code written especially for agoric install
Delete the yarn.locks entirely, and run a yarn install

Would the effect be comparable to amending the dapp package.json to include resolutions for all the agoric-sdk packages using a particular dist-tag?

I don't think so. The idea is that package.jsons and yarn.locks should point to real packages all the time, and be reinstallable from scratch just with yarn install. The "symlink-to-SDK" mode is an add-on, not a replacement.

kriskowal

I cannot profess to be able to maintain agoric install based on this review, but the code looks like it should do as advertised. Given that hopefully agoric install gives way to yarn install soonish, I’m okay with this.

mhofman · 2021-12-11T02:44:55Z

packages/agoric-cli/src/install.js

-  const versions = new Map();
-  const dirPackages = [];
+
+  const sdkRoot = path.resolve(dirname, `../../..`);


Where is this coming from ?

mhofman · 2021-12-11T03:05:27Z

agoric install beta will update the workspace's yarn.lock with the beta dependencies. From that point on, installing the dapp only needs yarn install (though agoric install will work, too).

This actually doesn't work for me. The yarn.lock file of my existing dapp doesn't seem to be updated. The preinstall script is also left in place, which means yarn install is not possible.

michaelfig added the agoric-cli package: agoric-cli label Nov 26, 2021

michaelfig requested a review from dckc November 26, 2021 20:44

michaelfig self-assigned this Nov 26, 2021

michaelfig force-pushed the mfig-npm-dist-tag branch 3 times, most recently from 165916e to 3d8ff0f Compare November 26, 2021 21:56

dckc reviewed Nov 26, 2021

View reviewed changes

michaelfig requested a review from kriskowal November 27, 2021 18:24

michaelfig force-pushed the mfig-npm-dist-tag branch from 3d8ff0f to 0fc3e9b Compare November 28, 2021 00:27

kriskowal reviewed Nov 29, 2021

View reviewed changes

michaelfig force-pushed the mfig-npm-dist-tag branch from 0fc3e9b to 1634180 Compare December 2, 2021 05:39

michaelfig commented Dec 2, 2021

View reviewed changes

michaelfig requested a review from kriskowal December 2, 2021 05:58

kriskowal approved these changes Dec 2, 2021

View reviewed changes

michaelfig added 3 commits December 2, 2021 18:59

feat(dist-tag): add script and instructions to manipulate NPM tags

241c10a

feat(agoric-cli): install <TAG> forces redownload of <TAG>

c41be9d

fix(agoric-cli): make agoric --no-sdk install work as well

e852ee5

michaelfig force-pushed the mfig-npm-dist-tag branch from 1634180 to e852ee5 Compare December 3, 2021 00:59

michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Dec 3, 2021

Merge branch 'master' into mfig-npm-dist-tag

e3b8a21

mergify bot merged commit fbc7c64 into master Dec 3, 2021

mergify bot deleted the mfig-npm-dist-tag branch December 3, 2021 01:28

mhofman reviewed Dec 11, 2021

View reviewed changes

mhofman mentioned this pull request Dec 11, 2021

> agoric install beta does not update dapp workspace #4170

Closed

dckc mentioned this pull request Dec 11, 2021

no supported workflow for installing from npm #3857

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully support `agoric install <dist-tag>` #4129

Fully support `agoric install <dist-tag>` #4129

michaelfig commented Nov 26, 2021 •

edited

Loading

dckc left a comment

dckc Nov 26, 2021

dckc Nov 26, 2021

michaelfig Nov 27, 2021

dckc commented Nov 26, 2021

michaelfig commented Nov 27, 2021

kriskowal left a comment

kriskowal Nov 29, 2021

kriskowal Nov 29, 2021

michaelfig Dec 2, 2021

kriskowal Dec 2, 2021

kriskowal Nov 29, 2021

michaelfig Dec 2, 2021

kriskowal Dec 2, 2021

kriskowal Nov 29, 2021

michaelfig Dec 2, 2021

kriskowal Nov 29, 2021

kriskowal Nov 29, 2021

kriskowal Nov 29, 2021

kriskowal Nov 29, 2021

michaelfig Dec 2, 2021

kriskowal Nov 29, 2021

kriskowal Nov 29, 2021

michaelfig left a comment

michaelfig Dec 2, 2021

michaelfig Dec 2, 2021

michaelfig Dec 2, 2021

michaelfig Dec 2, 2021

michaelfig Dec 2, 2021

michaelfig commented Dec 2, 2021

kriskowal left a comment

mhofman Dec 11, 2021

mhofman commented Dec 11, 2021

		@@ -0,0 +1,47 @@
		#! /bin/bash
		# dist-tag.sh - manage dist-tags for NPM registry packages

		@@ -0,0 +1,67 @@
		#! /bin/bash
		# npm-dist-tag.sh - manage dist-tags for NPM registry packages

Fully support agoric install <dist-tag> #4129

Fully support agoric install <dist-tag> #4129

Conversation

michaelfig commented Nov 26, 2021 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

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

dckc commented Nov 26, 2021

michaelfig commented Nov 27, 2021

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

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

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 commented Dec 2, 2021

kriskowal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Dec 11, 2021

Fully support `agoric install <dist-tag>` #4129

Fully support `agoric install <dist-tag>` #4129

michaelfig commented Nov 26, 2021 •

edited

Loading