Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix bazel building #71

Merged
merged 7 commits into from May 23, 2018
Merged

Fix bazel building #71

merged 7 commits into from May 23, 2018

Conversation

coltonmorris
Copy link
Contributor

@coltonmorris coltonmorris commented May 23, 2018 •
edited

Changed Bazel to use self managed node_modules.

Now other Bazel projects can build the protoc-gen-ts plugin using Bazel!

example use case:

Workspace

git_repository(
  name = "ts_protoc_gen",
  remote = "https://github.com/improbable-eng/ts-protoc-gen",
  tag = "idk",
)

git_repository(
  name = "io_bazel_rules_go",
  commit = "6bee898391a42971289a7989c0f459ab5a4a84dd", 
  remote = "https://github.com/bazelbuild/rules_go.git",
)
load("@io_bazel_rules_go//go:def.bzl", "go_rules_dependencies", "go_register_toolchains")
go_rules_dependencies()
go_register_toolchains()

http_archive(
  name = "io_bazel_rules_webtesting",
  strip_prefix = "rules_webtesting-master",
  urls = [
    "https://github.com/bazelbuild/rules_webtesting/archive/master.tar.gz",
    ],
)
load("@io_bazel_rules_webtesting//web:repositories.bzl", "browser_repositories", "web_test_repositories")
web_test_repositories()

git_repository(
  name = "build_bazel_rules_nodejs",
  remote = "https://github.com/bazelbuild/rules_nodejs.git",
  tag = "0.9.1",
)
load("@build_bazel_rules_nodejs//:defs.bzl", "node_repositories", "yarn_install")
# Pulled from improbable-eng/ts-protoc-gen repository
node_repositories(package_json = ["@ts_protoc_gen//:package.json"])

git_repository(
    name = "build_bazel_rules_typescript",
    remote = "https://github.com/bazelbuild/rules_typescript.git",
    tag = "0.14.0",
)
load("@build_bazel_rules_typescript//:defs.bzl", "ts_setup_workspace")
ts_setup_workspace()

# installs the node_modules
yarn_install(
  # Note: this must be named 'deps' until a custom rule is written
  name = "deps",
  package_json = "@ts_protoc_gen//:package.json",
  yarn_lock = "@ts_protoc_gen//:yarn.lock",
)

Now the protoc-gen-ts binary is available using the following Label: @ts_protoc_gen//bin:protoc-gen-ts

This PR will ready the repo for a custom Bazel rule that does most of the heavy lifting.

@coltonmorris
Copy link
Contributor Author

This also indirectly fixes a bug that happens if a Bazel project tries to use this npm package (it has breaking Bazel files and a workspace in it).

jonnyreeves
jonnyreeves suggested changes May 23, 2018
View reviewed changes
Copy link
Contributor

@jonnyreeves jonnyreeves left a comment

Choose a reason for hiding this comment

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

Thank you for raising this PR, whilst we do not yet use bazel for our front end projects at Improbable we do plan to one day, and improvements like the one you have contributed here both strengthen the bazel ecosystem and move us closer to our goal ♥️

I've requested the following changes:

  1. There are many unrelated changes to the generated example code. Please can you revert these and verify that you are building with protoc 3.2.0.

  2. Please can you explain your reasoning behind switching from yarn to npm

  3. (Optional) Is there a way to improve the Travis CI build to ensure that the bazel build is generating valid artefacts to avoid mistakes like the one you had to fix?

Thanks again for your contribution

WORKSPACE Outdated
remote = "https://github.com/bazelbuild/rules_typescript.git",
tag = "0.10.0",
# installs our node_modules
yarn_install(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused by this rule as ts-protoc-gen does not use yarn to manage dependencies? I'm guessing this rule provides interoperability with npm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is interoperable.

The reason for Yarn is because rules_nodejs recommends it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revert back to npm please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed the commit to use rules_nodejs' npm_install. Works perfectly! 😝

bin/BUILD
],
entry_point = "ts_protoc_gen/src/ts_index",
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, good catch. I'm surprised this didn't fail CI, is there a way we can automatically test for errors like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually surprised it wasn't caught either, not sure why, and I don't really have the resources to tell you why.
The previous change bazel build //...:all is a good way to ensure bazel builds - it would also end up running tests you eventually create with bazel.

examples/generated/examplecom/repeated_primitive_message_pb.js Outdated
*/
proto.examplecom.RepeatedPrimitiveMessage.prototype.getMyFloatList = function() {
return /** @type {!Array.<number>} */ (jspb.Message.getRepeatedFloatingPointField(this, 2));
return /** @type {!Array<number>} */ (jspb.Message.getRepeatedFloatingPointField(this, 2));
Copy link
Contributor

Choose a reason for hiding this comment

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

Which version of protoc are you using? I ask because ideally this generated code should not have changed as it is unrelated to your bazel changes.

Our CI currently uses protoc v3.2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v3.5.1

package.json
@@ -35,7 +35,6 @@
"@types/lodash": "^4.14.106",
"@types/mocha": "^2.2.46",
"@types/node": "^7.0.52",
"@types/text-encoding": "0.0.32",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

generate.sh Outdated
@@ -6,10 +6,12 @@ set -e
EXAMPLES_GENERATED_DIR=examples/generated

echo "Ensuring we have NPM packages installed..."
npm install
# npm install
yarn
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change from npm to yarn?

I have a strong (mostly personal) for this project to remain using npm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned earlier, rules_nodejs recommends it.

jonnyreeves
jonnyreeves approved these changes May 23, 2018
View reviewed changes
Copy link
Contributor

@jonnyreeves jonnyreeves left a comment

Choose a reason for hiding this comment

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

🚀

@jonnyreeves jonnyreeves merged commit 9e74eb0 into improbable-eng:master May 23, 2018
@easyCZ
Copy link
Contributor

easyCZ commented May 23, 2018

This is awesome, thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonnyreeves jonnyreeves jonnyreeves approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@coltonmorris @easyCZ @jonnyreeves