Skip to content

Conversation

@lina128
Copy link
Collaborator

@lina128 lina128 commented Mar 23, 2020

Tried the new release scripts, this PR fixes below issues:

  1. Change from yarn publish to yarn publish-npm. Context: yarn publish does not work, it throws error: "Can't answer a question unless a user TTY", same issue is discussed in [lerna publish] Can't answer a question unless a user TTY lerna/lerna#1200. The reason is because yarn publish has an interactive cli, which makes it impossible to use for automation process. Tried the yarn --non-interactive publish method discussed in the thread, doesn't work either. So change to yarn publish-npm, tested locally using npm whoami as a substitute. Yarn 2.x has yarn npm publish natively. So once we upgrade to yarn 2.x, we don't need to have publish-npm: "npm publish" in each package.json.

  2. Combined tag version steps. Context: This script is called from the publish-npm.ts. It didn't throw any errors. But actually, only the first step that creates the tag succeed, the second step that pushes the tag didn't get executed. Combining the two steps works. It is something related to child processes.


This change is Reviewable

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, and @pyu10055)


scripts/publish-npm.ts, line 128 at r1 (raw file):

    console.log(chalk.magenta.bold(`~~~ Publishing ${pkg} to npm ~~~`));
    shell.cd(pkg);
    $('YARN_REGISTRY="https://registry.npmjs.org/" yarn npm-publish');

do it work if you replace s/yarn npm-publish/npm publish/ (keeping the YARN_REGISTRY= bit)

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, and @pyu10055)


scripts/publish-npm.ts, line 128 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

do it work if you replace s/yarn npm-publish/npm publish/ (keeping the YARN_REGISTRY= bit)

does* it work

Copy link
Collaborator Author

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @pyu10055)


scripts/publish-npm.ts, line 128 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

does* it work

I tried YARN_REGISTRY=... npm whoami, it doesn't work. Throw the same authentication error.

@dsmilkov
Copy link
Contributor

Sounds good. Thanks for checking. LGTM

@lina128 lina128 merged commit 51d5568 into tensorflow:master Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants