-
Notifications
You must be signed in to change notification settings - Fork 2k
[converter]Temprory solution to symlink dependencies. #2834
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
Conversation
nsthorat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @lina128, @nsthorat, and @pyu10055)
tfjs-converter/scripts/link-core-master.js, line 1 at r1 (raw file):
#!/usr/bin/env node
can we put this script in the union package and share it?
Done. |
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @lina128, and @pyu10055)
tfjs-converter/package.json, line 57 at r2 (raw file):
}, "scripts": { "build": "yarn gen-json --test && tsc",
can you call link-core-master here (build) as well
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @lina128, and @pyu10055)
tfjs-converter/package.json, line 57 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
can you call link-core-master here (build) as well
or is this used from our CI? maybe split into "build" and "build-ci" in that case?
Hi Daniel, build is used in CI and build-npm.sh. For build-npm, we shouldn't link to core. For CI, the link-to-core is done in an earlier step. |
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @lina128 and @pyu10055)
tfjs-converter/package.json, line 57 at r2 (raw file):
Previously, lina128 (Na Li) wrote…
Hi Daniel, build is used in CI and build-npm.sh. For build-npm, we shouldn't link to core. For CI, the link-to-core is done in an earlier step.
so can we split into two: "build" and "build-ci". "build" calls "link-core-master" but "build-ci" doesn't. Just like we do for wasm:https://github.com/tensorflow/tfjs/blob/master/tfjs-backend-wasm/package.json#L16
this way both "yarn build" and "yarn test" will assure that core is linked at master, while "yarn build-ci" and "yarn test-ci" will not. Makes it symmetric with "test" and "test-ci"
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r1, 3 of 3 files at r3.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @pyu10055)
The symlink is changed back to pinned version after release. I thought the generated release branch won't be merged back to master, but it does. Short term solution is to apply the same strategy that WASM package use to use script to change back to symlink. Cons is, (1) people need to remember run test at least once before submit the PR, because CI won't do the link. (2) it's possible people may develop against pinned version if they don't run the test first.
Long term solution (hopefully will come soon), always keep symlink in development code and master branch. Have a dedicated release branch for bumping versions. Always publish from the release branch.
This change is