Skip to content

Conversation

@lina128
Copy link
Collaborator

@lina128 lina128 commented Mar 5, 2020

This change is Reviewable

@lina128 lina128 requested review from caisq, dsmilkov and nsthorat March 5, 2020 21:16
@lina128 lina128 requested a review from pyu10055 March 5, 2020 21:27
Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

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

@lina128 lina128 merged commit e71c512 into tensorflow:master Mar 5, 2020
"build": "tsc",
"build-core": "cd ../tfjs-core && yarn && yarn build",
"build-core-ci": "cd ../tfjs-core && yarn && yarn build-ci",
"build-npm": "./scripts/build-npm.sh",
Copy link
Contributor

Choose a reason for hiding this comment

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

A suggestion: to defend against publishing layers with a linked core package, we should have build-npm.sh check that "@tensorflow/tfjs-core": points to a version and not "link:../tfjs-core" . Likewise for the other build-npm.sh scripts in the other packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a reasonable suggestion for extra safety but in practice this won’t happen because of the release script :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The release script makes sure it uses a pinned version. But yes, it can't prevent link leak to npm if package is released manually. I will add the safe guard in another PR.

@nsthorat
Copy link
Contributor

nsthorat commented Mar 6, 2020

One thing I forgot about, you need to do something like this: https://github.com/tensorflow/tfjs/blob/master/tfjs-backend-wasm/package.json#L23

This script runs every time we build (or do anything for development) to make sure that when developing we're reverting the changes of the version pinning of the release script back to link:../. You should put the link script in top-level of the union package because it can be shared by all packages.

@lina128
Copy link
Collaborator Author

lina128 commented Mar 6, 2020

One thing I forgot about, you need to do something like this: https://github.com/tensorflow/tfjs/blob/master/tfjs-backend-wasm/package.json#L23

This script runs every time we build (or do anything for development) to make sure that when developing we're reverting the changes of the version pinning of the release script back to link:../. You should put the link script in top-level of the union package because it can be shared by all packages.

Exactly, I noticed that too. Created another PR for that: #2834

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.

4 participants