Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Convert repository to Bazel #10
base: master
Are you sure you want to change the base?
Convert repository to Bazel #10
Changes from all commits
9a0050f
75b809c
cae3fb4
7f6d722
f887245
a3d3c60
e1caaac
46085c5
9a8e502
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this seems expensive and sad and maybe wrong.
on most projects we recommend Bazel just being an npm dependency in the package.json, assuming that developers on the project want to approach it as they would any frontend project.
Also you want to ensure that developers have the same version of Bazel as the CI. things are changing quickly enough that version skew will be a frequent breakage for devs
if you really want to use Bazel natively installed on the machine for some reason, then ideally you would do that in the docker image that the VM is booted with, not do a network fetch and install in each build, it's slow.
This is a lot of the motivation for us moving to CircleCI where you can pick your base docker image.
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.
Why would it be wrong? I do check the checksum. Also, a yarn install would also fetch Bazel from the network. Though I agree with you that developers may be using a different version.
What would be the alternative workflow? Is this the list below what you suggest?
yarn
to install dependenciesyarn run bazel
whenever we want to call BazelThis file was deleted.
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.
in some repos we do some stripping of this thing.
We typically use an
npm_package
target so that the thing that's distributed on npm isn't identical to the sources. And that way we don't check in any generated artifacts, which are confusing if the repo is in a state where the sources are updated but the generated artifacts are notThere 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.
FYI, I get 404 for https://bazelbuild.github.io/rules_nodejs/npm_package/npm_package.html and https://github.com/bazelbuild/rules_nodejs/blob/master/docs/npm_package/npm_package.html, the top 2 results for [bazel npm_package]
I never quite know where to look for rules documentation. I finally found it at https://bazelbuild.github.io/rules_nodejs/Built-ins.html. However, it doesn't really say what
npm_package
does. This is another example where an end-to-end tutorial could have helped a lot.I ended up digging the source code, which has more useful documentation: https://github.com/bazelbuild/rules_nodejs/blob/master/internal/npm_package/npm_package.bzl
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.
On our specific use case, the reason for this repo is to share code between outline-server and outline-client. We don't really publish this as a public npm, but we do use it as a npm dependency, using a github specification.
I'm converting outline-server to Bazel, and having this repo as Bazel will probably help, since I'm still having issues with ShadowsocksConfig there.
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.
How do I strip the /// header? Is that an option in the build rule?