Experimental okbazel support #251

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
6 participants
@aj-michael

aj-michael commented Nov 21, 2016

See #232 for context

Documentation for this change is in the commit description of a5a7f9e and in a separate buildSrc/src/main/groovy/com/uber/okbuck/experimental/bazel/README.md

Sorry for the delay on sending this, I had a bit of difficultly getting the cache to work correctly due to some differences between how Buck and Bazel handle resource dependencies.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Nov 21, 2016

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Nov 21, 2016

CLA assistant check
All committers have signed the CLA.

@aj-michael

This comment has been minimized.

Show comment
Hide comment
@aj-michael

aj-michael Nov 21, 2016

Hmmm, I looked through the Travis CI failure and it doesn't appear to be related to this set of commits.

Hmmm, I looked through the Travis CI failure and it doesn't appear to be related to this set of commits.

@hzsweers

This comment has been minimized.

Show comment
Hide comment
@hzsweers

hzsweers Nov 21, 2016

Thanks! FYI - @kageiit is in Hawaii this week on vacation, so probably won't get reviewed until next week.

Thanks! FYI - @kageiit is in Hawaii this week on vacation, so probably won't get reviewed until next week.

@aj-michael

This comment has been minimized.

Show comment
Hide comment
@aj-michael

aj-michael Nov 21, 2016

Ok, thanks for the heads up 😃

Ok, thanks for the heads up 😃

@kageiit

This comment has been minimized.

Show comment
Hide comment
@kageiit

kageiit Nov 30, 2016

Collaborator

Ill take a look by end of this week. Can you check the travis failure?

Collaborator

kageiit commented Nov 30, 2016

Ill take a look by end of this week. Can you check the travis failure?

@aj-michael

This comment has been minimized.

Show comment
Hide comment
@aj-michael

aj-michael Dec 2, 2016

Sorry for the delayed response, I've been busy this week. I've looked at the Travis failure and it appears unrelated to this change? I'm not sure how to re-trigger Travis to run to check if it is just a flake. As far as I know, my change should not impact any tests as they don't use the OkBazel plugin.

Sorry for the delayed response, I've been busy this week. I've looked at the Travis failure and it appears unrelated to this change? I'm not sure how to re-trigger Travis to run to check if it is just a flake. As far as I know, my change should not impact any tests as they don't use the OkBazel plugin.

@kageiit

This comment has been minimized.

Show comment
Hide comment
@kageiit

kageiit Dec 2, 2016

Collaborator

I restarted the build. But for future reference, you can close and reopen a PR to have travis rekick your build

Collaborator

kageiit commented Dec 2, 2016

I restarted the build. But for future reference, you can close and reopen a PR to have travis rekick your build

+
+import com.uber.okbuck.rule.BuckRule
+
+final class AndroidBinaryRule extends BuckRule {

This comment has been minimized.

@kageiit

kageiit Dec 6, 2016

Collaborator

Can we name all these classes with a Bazel prefix? This is to avoid confusing with the buck classes

@kageiit

kageiit Dec 6, 2016

Collaborator

Can we name all these classes with a Bazel prefix? This is to avoid confusing with the buck classes

This comment has been minimized.

+ private final String packageName
+ private final Set<AndroidTarget.ResBundle> resources
+
+ AndroidLibraryRule(

This comment has been minimized.

@kageiit

kageiit Dec 6, 2016

Collaborator

does bazel not support annotation processors currently? or is it done implicitly somehow?

@kageiit

kageiit Dec 6, 2016

Collaborator

does bazel not support annotation processors currently? or is it done implicitly somehow?

This comment has been minimized.

This comment has been minimized.

@aj-michael

aj-michael Dec 8, 2016

Bazel does support annotation processors. It's not done implicitly, this PR does not support it in its current state. I did this intentionally as a first step, because I believe Bazel and Buck handle annotation processors differently.

If you'd prefer to make that a requirement for the initial submission, I can work on that, but it will take some time.

@aj-michael

aj-michael Dec 8, 2016

Bazel does support annotation processors. It's not done implicitly, this PR does not support it in its current state. I did this intentionally as a first step, because I believe Bazel and Buck handle annotation processors differently.

If you'd prefer to make that a requirement for the initial submission, I can work on that, but it will take some time.

This comment has been minimized.

@kageiit

kageiit Dec 8, 2016

Collaborator

we can defer it. was just curious

@kageiit

kageiit Dec 8, 2016

Collaborator

we can defer it. was just curious

This comment has been minimized.

@aj-michael

aj-michael Dec 8, 2016

Ok, for documentations sake, the way that annotation processors work in Bazel is we create a java_plugin rule that depends on a java_library that builds that annotation processor or a java_import that wraps a prebuilt JAR. The java_plugin specifies a processor_class. Then we put the java_plugin in the plugins attribute of an android_library or java_library that it should run on.

@aj-michael

aj-michael Dec 8, 2016

Ok, for documentations sake, the way that annotation processors work in Bazel is we create a java_plugin rule that depends on a java_library that builds that annotation processor or a java_import that wraps a prebuilt JAR. The java_plugin specifies a processor_class. Then we put the java_plugin in the plugins attribute of an android_library or java_library that it should run on.

+import com.uber.okbuck.composer.AndroidBuckRuleComposer
+import com.uber.okbuck.core.model.AndroidLibTarget
+
+final class AndroidLibraryRuleComposer extends AndroidBuckRuleComposer {

This comment has been minimized.

@kageiit

kageiit Dec 6, 2016

Collaborator

I would suggest extending the AndroidBuckRuleComposer to create a BazelAndroidBuckRuleComposer which can then override the definitions of external etc. to keep the logic cleaner

@kageiit

kageiit Dec 6, 2016

Collaborator

I would suggest extending the AndroidBuckRuleComposer to create a BazelAndroidBuckRuleComposer which can then override the definitions of external etc. to keep the logic cleaner

This comment has been minimized.

@aj-michael

aj-michael Dec 6, 2016

Ah, I just wrote out a long reply about this and then I think github went down for 2 minutes.

The gist of it was, I've added a commit that introduces BazelRuleComposer. It's a trait as opposed to a subclass of BuckRuleComposer because I need to extend both AndroidBuckRuleComposer and JavaBuckRuleComposer, which each extend BuckRuleComposer.

@aj-michael

aj-michael Dec 6, 2016

Ah, I just wrote out a long reply about this and then I think github went down for 2 minutes.

The gist of it was, I've added a commit that introduces BazelRuleComposer. It's a trait as opposed to a subclass of BuckRuleComposer because I need to extend both AndroidBuckRuleComposer and JavaBuckRuleComposer, which each extend BuckRuleComposer.

+ PrintStream printer = new PrintStream(subProject.file("BUILD")) {
+ @Override
+ public void println(String s) {
+ // BUILD files are typically space-delimited. Since

This comment has been minimized.

@kageiit

kageiit Dec 6, 2016

Collaborator

Great point. I think we should allow customizing the delimiters easily. But this works for now

@kageiit

kageiit Dec 6, 2016

Collaborator

Great point. I think we should allow customizing the delimiters easily. But this works for now

This comment has been minimized.

@aj-michael

aj-michael Dec 6, 2016

The Bazel team maintains buildifier, a tool for formatting BUILD files according to the standard convention. I added this hack so that the BUILD files generated by okbuck adhere to that convention. As an alternative, we could invoke buildifier to clean up the BUILD files at the end. Although, the integration may be messy as buildifier is written in go.

@aj-michael

aj-michael Dec 6, 2016

The Bazel team maintains buildifier, a tool for formatting BUILD files according to the standard convention. I added this hack so that the BUILD files generated by okbuck adhere to that convention. As an alternative, we could invoke buildifier to clean up the BUILD files at the end. Although, the integration may be messy as buildifier is written in go.

This comment has been minimized.

@kageiit

kageiit Dec 6, 2016

Collaborator

I meant customizing delimiters in okbuck, not bazel specifically, but that works as well :)

@kageiit

kageiit Dec 6, 2016

Collaborator

I meant customizing delimiters in okbuck, not bazel specifically, but that works as well :)

+`WORKSPACE` to contain the `api_level` and `build_tools_version` from your SDK
+that you wish to use.
+
+## Setting up Bazel

This comment has been minimized.

@kageiit

kageiit Dec 6, 2016

Collaborator

I would recommend investing in a bazelw script that will download and install bazel from source similar to what buckw does. It can also run okbuck when files have changed similar to buckw. I think this is a very necessary usability feature for adoption.

@kageiit

kageiit Dec 6, 2016

Collaborator

I would recommend investing in a bazelw script that will download and install bazel from source similar to what buckw does. It can also run okbuck when files have changed similar to buckw. I think this is a very necessary usability feature for adoption.

This comment has been minimized.

@aj-michael

aj-michael Dec 8, 2016

I will look into how buckw works and respond back on this thread. However, I don't think installing bazel from source is a possibility. Bazel can only be built with a pre-existing installation of Bazel. We do this to maintain platform independence (e.g. we don't want to be shipping pre-compiled, platform-specific binaries in the Bazel source tree that would be needed to bootstrap from scratch).

Furthermore, Bazel is very carefully versioned and, since it is pre-1.0, occasionally makes breaking changes. As such, we have some users who intentionally do not update to the newest version immediately because it may require a change to their BUILD files. We keep a history of platform-specific installers here and a detailed changelog for each release here.

I agree that re-running okbuck when files have changed is important for usability.

@aj-michael

aj-michael Dec 8, 2016

I will look into how buckw works and respond back on this thread. However, I don't think installing bazel from source is a possibility. Bazel can only be built with a pre-existing installation of Bazel. We do this to maintain platform independence (e.g. we don't want to be shipping pre-compiled, platform-specific binaries in the Bazel source tree that would be needed to bootstrap from scratch).

Furthermore, Bazel is very carefully versioned and, since it is pre-1.0, occasionally makes breaking changes. As such, we have some users who intentionally do not update to the newest version immediately because it may require a change to their BUILD files. We keep a history of platform-specific installers here and a detailed changelog for each release here.

I agree that re-running okbuck when files have changed is important for usability.

This comment has been minimized.

@kageiit

kageiit Dec 8, 2016

Collaborator

we should atleast have a config in bazelw that will be able to download a particular version of bazel that is prebuilt if it is not buildable from source

@kageiit

kageiit Dec 8, 2016

Collaborator

we should atleast have a config in bazelw that will be able to download a particular version of bazel that is prebuilt if it is not buildable from source

This comment has been minimized.

@aj-michael

aj-michael Dec 12, 2016

Ok, I've added a bazelWrapper Gradle task that generates bazelw. bazelw downloads and executes the installer for bazel 0.4.2 (the most recent stable release at the time of writing). However, it is currently hardcoded to the Linux installer.

@aj-michael

aj-michael Dec 12, 2016

Ok, I've added a bazelWrapper Gradle task that generates bazelw. bazelw downloads and executes the installer for bazel 0.4.2 (the most recent stable release at the time of writing). However, it is currently hardcoded to the Linux installer.

@aj-michael

This comment has been minimized.

Show comment
Hide comment
@aj-michael

aj-michael Dec 8, 2016

I'm still working on addressing all of the comments so far, it's just a busy week for me. I will get to all of them. Are you planning to add more comments than the ones so far?

I'm still working on addressing all of the comments so far, it's just a busy week for me. I will get to all of them. Are you planning to add more comments than the ones so far?

@aj-michael

This comment has been minimized.

Show comment
Hide comment
@aj-michael

aj-michael Dec 12, 2016

Ok, I think I have now made it through all of the open comments.

Ok, I think I have now made it through all of the open comments.

+}
+
+# Run tasks before bazel command
+setupBuckRun ( ) {

This comment has been minimized.

@kageiit

kageiit Dec 12, 2016

Collaborator

Seems like a lot of common logic is reused across both wrappers. We use a common shell script internally for various wrapper scripts. Will push that change up so this script can get quite small

@kageiit

kageiit Dec 12, 2016

Collaborator

Seems like a lot of common logic is reused across both wrappers. We use a common shell script internally for various wrapper scripts. Will push that change up so this script can get quite small

This comment has been minimized.

@aj-michael

aj-michael Dec 12, 2016

Yes, the main difference between these scripts is how Buck/Bazel is installed.

@aj-michael

aj-michael Dec 12, 2016

Yes, the main difference between these scripts is how Buck/Bazel is installed.

@kageiit

This comment has been minimized.

Show comment
Hide comment
@kageiit

kageiit Dec 16, 2016

Collaborator

Can you perform a quick rebase? We made a ton of changes over the past week and want to ensure everything is in working order still

Collaborator

kageiit commented Dec 16, 2016

Can you perform a quick rebase? We made a ton of changes over the past week and want to ensure everything is in working order still

@aj-michael

This comment has been minimized.

Show comment
Hide comment
@aj-michael

aj-michael Dec 16, 2016

Ok, working on the rebase now. As expected there are some cleanups that I will need to make.

Ok, working on the rebase now. As expected there are some cleanups that I will need to make.

@aj-michael

This comment has been minimized.

Show comment
Hide comment
@aj-michael

aj-michael Dec 16, 2016

Ok, rebase is complete. I fixed some import paths and confirmed that it still works using bazelw on https://github.com/gradle/perf-android-large. Spoke to soon, a few method signatures changed that I need to update.

aj-michael commented Dec 16, 2016

Ok, rebase is complete. I fixed some import paths and confirmed that it still works using bazelw on https://github.com/gradle/perf-android-large. Spoke to soon, a few method signatures changed that I need to update.

aj-michael added some commits Nov 18, 2016

Adds Bazel (https://bazel.build) support to OkBuck
To use OkBazel in your gradle project, make sure to have the OkBuck
Gradle plugin jar as a classpath dependency and put the following in
your `build.gradle`:

    apply plugin: "com.uber.okbazel"

Then from the command line, you can create Bazel BUILD files with

    ./gradlew okbazel

This will produce several `BUILD` files for your Java and Android
projects. To build them with Bazel, download Bazel from
https://bazel.build and build your entire project with:

    bazel build ...

Or build a specific target with:

    bazel build //mySubProject:bin_myappRelease
Add some documentation about the OkBazel plugin.
I've created a README.md in the experimental/bazel folder that describes
how to use the plugin and gives some helpful links to Bazel
documentation.
Update OkBazel README.md
Added description of compiling Bazel from source and necessary flags to build OkBazel projects.
Add Bazel prefix to Rule and RuleComposer classes.
This is to disambiguate the Rule and RuleComposer classes in OkBazel
from their counterparts in OkBuck, e.g.
com.uber.okbuck.rule.android.AndroidLibraryRule and
com.uber.okbuck.experimental.bazel.BazelAndroidLibraryRule.
Introduces BazelRuleComposer.groovy.
This trait includes all Bazel-specific overrides of the base rule
composer classes used by OkBuck. Unfortunately, it doesn't work as a
subclass of BuckRuleComposer because BuckRuleComposer has subclasses
that the Bazel rule composers need to extend.
Add bazelWrapper task that generates `bazelw`.
`bazelw` watches for changes in files that require okbazel to be rerun.
It also downloads and installs bazel-0.4.2 if there is no Bazel
installation specified via the $BAZEL_BINARY environment variable or in
$PATH.
@aj-michael

This comment has been minimized.

Show comment
Hide comment
@aj-michael

aj-michael Jan 21, 2017

Hey, really sorry about the delay on this. I took a long holiday, and have been catching up on other work in the few days I've been back. I've just finished the rebase and testing to ensure that everything is still good. PTAL 😄

Hey, really sorry about the delay on this. I took a long holiday, and have been catching up on other work in the few days I've been back. I've just finished the rebase and testing to ensure that everything is still good. PTAL 😄

@kageiit

This comment has been minimized.

Show comment
Hide comment
@kageiit

kageiit Jan 25, 2017

Collaborator

Can you add

apply plugin: 'com.uber.okbazel' to okbuck build.gradle itself.

There are some issues with being able to use both plugins because the bazel plugin is trying to create the same extension as the buck plugin

Collaborator

kageiit commented Jan 25, 2017

Can you add

apply plugin: 'com.uber.okbazel' to okbuck build.gradle itself.

There are some issues with being able to use both plugins because the bazel plugin is trying to create the same extension as the buck plugin

@aj-michael

This comment has been minimized.

Show comment
Hide comment
@aj-michael

aj-michael Jan 25, 2017

I asked about sharing the plugin id back in #232 (comment) , and yes you are correct, this prevents you from using both plugins in the same project. At the time, the reason I did that was because there were several places where the plugin id was used in the codebase and was hardcoded to okbuck.

I can take another look to see what it would take to give it a separate plugin ID.

I asked about sharing the plugin id back in #232 (comment) , and yes you are correct, this prevents you from using both plugins in the same project. At the time, the reason I did that was because there were several places where the plugin id was used in the codebase and was hardcoded to okbuck.

I can take another look to see what it would take to give it a separate plugin ID.

@kageiit

This comment has been minimized.

Show comment
Hide comment
@kageiit

kageiit Aug 3, 2017

Collaborator

Closing due to inactivity. Feel free to comment anytime if you would like me to re-open this

Collaborator

kageiit commented Aug 3, 2017

Closing due to inactivity. Feel free to comment anytime if you would like me to re-open this

@kageiit kageiit closed this Aug 3, 2017

@steeve

This comment has been minimized.

Show comment
Hide comment
@steeve

steeve Mar 31, 2018

@aj-michael did you ever found a way around this? great work though :)

steeve commented Mar 31, 2018

@aj-michael did you ever found a way around this? great work though :)

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