Bazel deps helper scripts #19

greggdonovan · 2017-10-11T22:35:18Z

Add scripts to auto-generate the thirdparty/jvm BUILD files from the maven_deps.yaml.

These scripts handle:

checking out and updating bazel-deps
running the buildifier on the generated BUILD files
making any manual edits to the generated files via buildozer rules
a check to ensure the generated files are in sync with the yaml

The PR also runs these scripts to regenerate the thirdparty/jvm files that have been buildifier formatted.

This allows us to more easily add new dependencies to maven_deps.yaml. For example, for Scala support.

…maven_deps.yaml. These scripts handle: - checking out and updating bazel-deps - running the buildifier on the generated BUILD files - making any manual edits to the generated files via buildozer rules - a check to ensure the generated files are in sync with the yaml

…er-ed

bazel-io · 2017-10-11T22:35:20Z

Can one of the admins verify this patch?

petroseskinder · 2017-10-11T22:54:16Z

@greggdonovan can you add a line checking if buildifier is installed (and installing it otherwise)? I ran into this issue when running generate.sh on a machine without buildifier.

greggdonovan · 2017-10-11T23:18:00Z

@petroseskinder Good idea, thanks. I should do the same with buildozer, as well, I think.

I'll update the PR.

cgrushko · 2017-10-12T00:00:20Z

ok to test

cgrushko

Thanks!
I hope that we can one day drop these scripts and upstream all of its goodies :)

cgrushko · 2017-10-12T00:01:57Z

.gitignore

@@ -43,3 +43,5 @@ hs_err_pid*

 # Ignore mac attributes files
 *.DS_STORE
+# Ignore bazel-deps resolverCache. See: https://github.com/johnynek/bazel-deps#options


do you have experience with bazel_output_base from that link?

We use bazel_output_base internally. @kevingessner added it here. I like it and I think it fits in with Bazel conventions nicely (e.g. removed with bazel clean --expunge). I can switch it in the PR if you like.

mmm... actually let's leave it like that, in case it turns out that it's useful to only have Bazel stuff in bazel-out.

cgrushko · 2017-10-12T00:07:18Z

dev-scripts/dependencies/add-dep.sh

+# shellcheck disable=SC1090
+. "$SCRIPT_DIR/setup.sh"
+
+"${BAZEL_DEPS_DIR}/gen_maven_deps.sh" add-dep --deps "${ROOT_DIR}/maven_deps.yaml" --lang "$lang" "${coords[@]}"


Wat. It already supports consuming maven dependencies?

I gather the reason for using this script instead of calling bazel-deps directly is because it formats and handles AutoValue?

Yes. Plus making sure you have a checked out version bazel-deps.

+1 to the goal of removing the scripts and using bazel-deps via a WORKSPACE rule of some sort.

gen_maven_deps.sh comes with bazel-deps and supports converting maven coords into dependencies.yaml. This wrapper script makes sure that bazel-deps is installed (setup.sh), updates dependencies.yaml, and then generates the new third_party code. It makes dep additions and updates a one-step process instead of 2 or 3. Switching to a workspace rule would take care of the first, but I still think it's a good workflow to have changing dependencies.yaml and regenerating the thrid_party files be a single command; maybe we could add that to bazel-deps.

Yeah, sounds reasonable to ask bazel-deps to also regenerate the BUILD files.

cgrushko · 2017-10-12T00:09:28Z

dev-scripts/dependencies/check-deps.sh

+then
+    echo
+    echo "* * *"
+    echo "Generated dependency files are not up to date.  Run \`dev-scripts/dependencies/generate.sh\` and commit the changes."


is this something that you found useful in general with bazel-deps?
If so, let's open an Issue in bazel-deps and see what Oscar thinks.
If he's open to the idea, let's put a # TODO here to delete the file once the upstream issue is resolved?

Yes, its occasionally useful. We have check-deps.sh run in our CI so that someone doesn't update maven_deps.yaml without running the generation step and checking in the thirdparty BUILD files. It's saved us a couple of times so far.

Are you thinking something like "${BAZEL_DEPS_DIR}/gen_maven_deps.sh check -r ${SCRIPT_DIR} -s thirdparty/workspace.bzl -d maven_deps.yaml?

yep, something like that.

cgrushko · 2017-10-12T00:11:17Z

dev-scripts/dependencies/setup.sh

+    buildozer 'add deps //external:jar/com/google/auto/value/auto_value' //thirdparty/jvm/com/google/auto/value:auto_value_plugin
+
+    # The generated BUILD files are not well-formatted. Run the buildifier on them.
+    find "${ROOT_DIR}/thirdparty/jvm" -name "BUILD" -exec "buildifier" -showlog -mode=fix {} +


Can you add a
# TODO(https://github.com/johnynek/bazel-deps/issues/73): Drop once issue is fixed.

cgrushko · 2017-10-12T00:11:44Z

dev-scripts/dependencies/setup.sh

+
+    "$BAZEL_DEPS_DIR/gen_maven_deps.sh" generate --repo-root "$ROOT_DIR" --sha-file "thirdparty/workspace.bzl" --deps maven_deps.yaml
+
+    # Manually add the AutoValue plugin. Otherwise, everything can be auto-generated from the YAML.


Can you add a
# TODO(https://github.com/johnynek/bazel-deps/issues/62): Drop once issue is fixed.

cgrushko · 2017-10-12T00:13:07Z

dev-scripts/dependencies/setup.sh

+
+SCRIPT_DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+ROOT_DIR=$(cd "${SCRIPT_DIR}/../.." && pwd)
+BAZEL_DEPS_DIR="$ROOT_DIR/../bazel-deps"


that'll land the new repo outside of the current tree?
feels awkward, though landing it inside the tree is also awkward. So I don't know :)

Managing external repos outside of bazel feels really awkward. I don't have a great solution.

Do you or @kevingessner or @johnynek have a link to the issue or issues preventing us from using bazel-deps as a repository rule? It'd be nice to track them. I see bazel #1673 but perhaps there's more to it.

#1673 is the only one I know of. Even if it's in the workspace, it'll be a little bit of wrangling to get it running correctly -- IIRC bazel-deps comes with the gen_maven_deps.sh wrapper script because it doesn't work with bazel run. So we'll need to get the various paths right. Doable but may be a little tricky.

cgrushko · 2017-10-12T00:15:27Z

thirdparty/workspace.bzl

@@ -1,4 +1,16 @@
-def maven_dependencies(callback):
+def declare_maven(hash):


can you delete load.bzl and the reference to it in WORKSPACE, now that it's not needed anymore?

👍 Updated the PR.

greggdonovan · 2017-10-12T01:31:52Z

Re: The buildozer and buildifier dependencies. How should we be having folks install them? For OSX there is a homebrew package for buildifier. We can add one for buildozer. I'm not sure if we hope to have these tools in standard yum/deb repos for linux.

Are we better off exiting with the proper instructions for installing them or better off doing something like:
command -v buildozer >/dev/null 2>&1 || { echo >&2 "buildozer is missing; installing via go get."; go get github.com/bazelbuild/buildtools/buildozer; } ?

The devtools script handles this via having buildifier as a submodule and building from source if missing, but maybe that's not the right approach for an end-user oriented tool?

cgrushko · 2017-10-12T15:42:06Z

I filed bazelbuild/bazel#3895 after chatting with @damienmg . There's a tentative agreement, but I'm told it's low priority.

BFG uses Buildozer to create the BUILD files, so it's actually a prerequisite. I'll update README.md with instructions.

So, I think it's ok to exit with instructions (pointing at README.md should be fine)

We can drop our workaround when annotation processor support and buildifier formatting are integrated into bazel-deps.

…o is not installed. TODO: Remove this once bazelbuild/bazel#3895 is resolved and we can provide a better method for installing these tools.

cgrushko

That's awesome, thanks!

cgrushko · 2017-10-13T16:29:14Z

Feel free to merge (you should have Write permissions).

cgrushko · 2017-10-27T21:47:10Z

Ah, I ran into some trouble with setup.sh - I tried using it in another repo, and it clashes with BFG. Might be worth to change BAZEL_DEPS_DIR to point inside the repo
(i.e. BAZEL_DEPS_DIR="$ROOT_DIR/bazel-deps")

greggdonovan · 2017-10-29T19:22:50Z

@cgrushko That's a good point -- I'll update in a PR. I can also add in removals of the workaround to bazel-deps#73 and bazel-deps#85.

I should be back to the meat of the Scala work this week, too. Sorry -- had to finish up some other work.

kevingessner · 2017-10-30T13:39:22Z

I'm hoping to work on bazelbuild/bazel#1673 this week, which should make setup.sh unnecessary. I'll let you know if I can make progress!

greggdonovan · 2017-10-31T01:52:39Z

@cgrushko Cloning bazel-deps inside of the BFG repo means that its rules get run on bazel test //... and there doesn't appear to be a good solution for excluding them.

How about BAZEL_DEPS_DIR="$ROOT_DIR/../BUILD_file_generator-bazel-deps" for now to avoid conflicts until @kevingessner solves this for real with bazelbuild/bazel#1673?

greggdonovan added 2 commits October 11, 2017 18:31

Use auto-generated thirdparty/jvm BUILD files that have been buildifi…

bb45a88

…er-ed

petroseskinder requested a review from cgrushko October 11, 2017 22:50

cgrushko reviewed Oct 12, 2017

View reviewed changes

Remove load.bzl as it is handled now in workspace.bzl.

486b348

cgrushko mentioned this pull request Oct 12, 2017

Emphasize that Buildozer doesn't require a separate Buildifier binary bazelbuild/buildtools#149

Merged

greggdonovan added 2 commits October 12, 2017 13:25

Reference bazel-deps issues that we are waiting for.

b2e4ef4

We can drop our workaround when annotation processor support and buildifier formatting are integrated into bazel-deps.

Check for and install buildozer and buildifier if missing. Abort if g…

8f2f0e8

…o is not installed. TODO: Remove this once bazelbuild/bazel#3895 is resolved and we can provide a better method for installing these tools.

cgrushko approved these changes Oct 12, 2017

View reviewed changes

greggdonovan added 2 commits October 12, 2017 15:32

Merge branch 'master' into bazel_deps_helper_scripts

4a3a2d0

Document how to use the bazel-deps wrapper scripts.

4a03119

cgrushko approved these changes Oct 13, 2017

View reviewed changes

Reference check-deps issue in bazel-deps.

e3bba36

greggdonovan merged commit ce08bf1 into bazelbuild:master Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bazel deps helper scripts #19

Bazel deps helper scripts #19

greggdonovan commented Oct 11, 2017

bazel-io commented Oct 11, 2017

petroseskinder commented Oct 11, 2017

greggdonovan commented Oct 11, 2017

cgrushko commented Oct 12, 2017

cgrushko left a comment

cgrushko Oct 12, 2017

greggdonovan Oct 12, 2017

cgrushko Oct 12, 2017

cgrushko Oct 12, 2017

greggdonovan Oct 12, 2017

kevingessner Oct 12, 2017

cgrushko Oct 12, 2017

cgrushko Oct 12, 2017

greggdonovan Oct 12, 2017

cgrushko Oct 12, 2017

cgrushko Oct 12, 2017 •

edited

Loading

cgrushko Oct 12, 2017 •

edited

Loading

cgrushko Oct 12, 2017

greggdonovan Oct 12, 2017

kevingessner Oct 12, 2017

cgrushko Oct 12, 2017

greggdonovan Oct 12, 2017

greggdonovan commented Oct 12, 2017

cgrushko commented Oct 12, 2017

cgrushko left a comment

cgrushko commented Oct 13, 2017

cgrushko commented Oct 27, 2017

greggdonovan commented Oct 29, 2017

kevingessner commented Oct 30, 2017

greggdonovan commented Oct 31, 2017


		"$BAZEL_DEPS_DIR/gen_maven_deps.sh" generate --repo-root "$ROOT_DIR" --sha-file "thirdparty/workspace.bzl" --deps maven_deps.yaml

		# Manually add the AutoValue plugin. Otherwise, everything can be auto-generated from the YAML.

		@@ -1,4 +1,16 @@
		def maven_dependencies(callback):
		def declare_maven(hash):

Bazel deps helper scripts #19

Bazel deps helper scripts #19

Conversation

greggdonovan commented Oct 11, 2017

bazel-io commented Oct 11, 2017

petroseskinder commented Oct 11, 2017

greggdonovan commented Oct 11, 2017

cgrushko commented Oct 12, 2017

cgrushko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgrushko Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

cgrushko Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greggdonovan commented Oct 12, 2017

cgrushko commented Oct 12, 2017

cgrushko left a comment

Choose a reason for hiding this comment

cgrushko commented Oct 13, 2017

cgrushko commented Oct 27, 2017

greggdonovan commented Oct 29, 2017

kevingessner commented Oct 30, 2017

greggdonovan commented Oct 31, 2017

cgrushko Oct 12, 2017 •

edited

Loading

cgrushko Oct 12, 2017 •

edited

Loading