Add an npm package output #114
Conversation
| name = "bazelisk-darwin", | ||
| out = "bazelisk-darwin_amd64", | ||
| embed = [":go_default_library"], | ||
| goarch = "amd64", |
There was a problem hiding this comment.
I didn't know that one could create platform-specific go_binary targets - that's much better than my existing approach in build.sh! :) Thanks!
There was a problem hiding this comment.
Yeah I figured you might want to refactor that script too. If nothing else, this allows you to build all the architectures in parallel, even if you still loop through them to place them in a bin directory
bazelisk.js
Outdated
| if (arch == undefined || platform == undefined) { | ||
| console.error(`FATAL: Your platform/architecture combination ${ | ||
| os.platform()} - ${os.arch()} is not yet supported. | ||
| Follow install instructions at https://github.com/bazelbuild/bazel-watcher/blob/master/README.md to compile for your system.`); |
There was a problem hiding this comment.
How are the bazel-watcher install instructions related to building Bazel on an unsupported architecture? 🤔
There was a problem hiding this comment.
Oops this is some leftover copy-pasta. Fixed
build.sh
Outdated
| # Googlers: you should npm login using the go/npm-publish service: | ||
| # $ npm login --registry https://wombat-dressing-room.appspot.com | ||
| # TODO: also publish the release to npm. | ||
| # ./bazelisk run --config=release //:npm_package.publish -- --access=public --tag latest --registry $REGISTRY |
There was a problem hiding this comment.
Does the "TODO" mean that this is a manual step, or is there still something missing before we can publish to npm?
There was a problem hiding this comment.
It just needs some manual testing. I could do that myself, but maybe it's better for you or Florian to do the first release to make sure permissions are right.
There was a problem hiding this comment.
I removed the comment, but it means after merging this, you'll trip over any problems with npm publishing the next time you run it.
(Also I notice the build.sh script doesn't actually do a publish step yet, so maybe this isn't the right place for this command to be added? I didn't see any other release instructions where it belongs instead)
| @@ -0,0 +1,8 @@ | |||
| { | |||
| "name": "@bazel/bazelisk", | |||
| "version": "0.0.0-PLACEHOLDER", | |||
There was a problem hiding this comment.
How is the "version" here replaced with the correct version?
There was a problem hiding this comment.
It's a feature of the pkg_npm rule: https://github.com/bazelbuild/rules_nodejs/blob/master/docs/Built-ins.md#replace_with_version
rules_nodejs.patch
Outdated
| @@ -0,0 +1,13 @@ | |||
| diff internal/pkg_npm/packager.js internal/pkg_npm/packager.js | |||
There was a problem hiding this comment.
I've never used pkg_npm in a repo that uses v prefix on the tags. NPM package versioning has to be purely numeric so it needs to be stripped. I'll make the same change upstream.
22dda09 to
5d12ea8
Compare
5d12ea8 to
8ee2faf
Compare
| @@ -7,3 +7,5 @@ CURRENT_TAG=$(git tag -l --points-at HEAD | head -n1) | |||
| CURRENT_COMMIT=$(git rev-parse HEAD) | |||
|
|
|||
| echo "STABLE_VERSION ${CURRENT_TAG:-$CURRENT_COMMIT}" | |||
| # rules_nodejs expects to read from volatile-status.txt | |||
| echo "BUILD_SCM_VERSION ${CURRENT_TAG:-$CURRENT_COMMIT}" | |||
There was a problem hiding this comment.
I've found it valuable to add a -dirty suffix when user changes happen. This saved me trying to debug someone's issue which would have been impossible without it
There was a problem hiding this comment.
Agreed, for Angular we use a .with-local-changes suffix in a non-pristine clone
https://github.com/angular/angular/blob/master/tools/bazel_stamp_vars.js#L52-L53
But of course that change should be made for all the stamping here, not just npm, so I don't think it should go in this PR. @philwo do you want a separate PR for that?
|
Ping! I'd like to build on top of this. If you're okay with it, I could publish the first version of the package from the last release tag. |
|
Sorry for the delay, @alexeagle! Merging this now and will try to push a first release. |
/cc @achew22