Improve npm publishing infra #7287

mattsoulanille · 2023-01-19T00:59:36Z

The current npm publishing infra is more manual than necessary. This PR makes the following improvements:

Publish all packages in a release unit with one command. Publishing no longer involves running yarn publish-npm multiple times.
Stage all packages to a local verdaccio repository before publishing. This catches any compatibility issues that may appear when publishing to npm and allows us to build all packages before publishing any to npm.
Publish with a single OTP. The previous infra required an OTP per package to publish to npm. This new infra publishes all packages sequentially after they're built, so it can use a single OTP for all of them. If another OTP is required due to the first one timing out, it asks for another.

A future PR will move our nightly verdaccio e2e tests to use this infra, which will allow us to avoid setting the npm registry globally. This will let us run verdaccio tests in parallel with the rest of nightly tests, which should reduce the time taken.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

This change is

mattsoulanille · 2023-01-19T01:01:26Z

e2e/scripts/verdaccio.yaml

@@ -19,7 +19,7 @@ packages:
  '@tensorflow/**':
    access: $all
    publish: $all
-    # Don't set proxy to avoid accidentally publishing to npm.


This should not be possible without an OTP, so it's okay to set proxy to npm.

I think this statement is incorrect, the proxy property does not allow publishing on npmjs, in fact only works in one way, from npmjs to your local cache. If you set proxy: npmjs what is gonna happens is when you publish a package that match @tensorflow/** will ask first npmjs if already exist there or not, if you are publishing a snapshot with a random version probably will goes through allow yo to publish otherwise you will have 409 publish conflict. That's the unique thing the proxy does, but never never verdaccio publish on npmjs, this never happens.

Thanks for the clarification. I was wondering if this comment was correct, and I couldn't find any description of this publishing behavior in the docs.

Thanks I'll try to highlight it better 👍🏼

mattsoulanille · 2023-01-19T01:02:39Z

scripts/publish-npm.ts

@@ -25,108 +25,150 @@
 import * as argparse from 'argparse';


Note to reviewer: Just review this as a new file. It shares very little logic with the previous implementation.

mattsoulanille · 2023-01-19T01:42:45Z

It may be worth writing tests for some parts of this PR, but we don't have any tests for our scripts directory yet. What do reviewers think about testing this code?

mattsoulanille · 2023-01-19T01:47:00Z

Nightly tests run with yarn nightly-cloudbuild, since this affects the verdaccio config file.

mattsoulanille · 2023-01-19T01:50:15Z

Nightly tests 2

mattsoulanille · 2023-01-19T01:57:30Z

Nightly tests 3

pyu10055

Reviewable status: 0 of 1 approvals obtained (waiting on @Linchenn and @mattsoulanille)

scripts/publish-npm.ts line 86 at r2 (raw file):

});

parser.addArgument(['--auto', '--guess-version'], {

the argument name does not seem to match the description, maybe auto-publish-local-newer?

scripts/publish-npm.ts line 180 at r2 (raw file):

          run(`${login}yarn --registry '${registry}' publish-npm ${dashes} ${otpFlag} --tag=${tag} --force`));
    } else {
      if (registry === NPM_REGISTRY && pkg.startsWith('tfjs-node')) {

should this be done after the publish is succeeded?

scripts/release-util.ts line 229 at r1 (raw file):

/**
 * A wrapper around shell.exec for readability.
 * @param cmd The bash commaond to execute.

command

scripts/release-util.ts line 243 at r1 (raw file):

export function $async(cmd: string,
		       env: Record<string, string> = {}): Promise<string> {

are there tabs on this line?

scripts/release-util.ts line 488 at r1 (raw file):

 * Get the next minor update version for the given version.
 *
 * i.e. given a.b.c, return a.b+1.0

a.b + 0.1.0 ?

e2e/scripts/local-registry.sh line 34 at r1 (raw file):

  # Set registry to local registry
  #npm set registry "$custom_registry_url"

remove these commented out lines?

mattsoulanille

Reviewable status: 0 of 1 approvals obtained (waiting on @Linchenn and @pyu10055)

scripts/publish-npm.ts line 86 at r2 (raw file):