-
Couldn't load subscription status.
- Fork 62
Functional databinding #340
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
base: main
Are you sure you want to change the base?
Conversation
62ce51c to
c5fe956
Compare
| output_dir + "/android/databinding/layouts/DataBindingInfo.java", | ||
| ) | ||
| _utils.copy_file(ctx, annotation_template, annotation_out) | ||
| ctx.actions.symlink(output = annotation_out, target_file = annotation_template) |
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.
If this is part of your other PR for fixing copy_file() for Mac, let me take another look there and see what we can do to fix things.
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.
My other PR doesn't fix copy_file for macOS; it just adds an example to surface that it is broken on macOS. This is where I actually incorporated the fix.
| # Outputs of the Data Binding annotation processor. | ||
| br_out = ctx.actions.declare_file( | ||
| output_dir + "bin-files/%s-br.bin" % java_package, | ||
| output_dir + "bin-files/%s--br.bin" % java_package, |
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 the double dash?
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.
I'm not entirely sure, but this is necessary on the version of android_tools currently pulled. (Presumably output by the data binding compiler.) If I override android_tools with a new version (e.g. Grab has one here), then I no longer need the double dash.
| if zipinfo -t "$1"; then | ||
| ORDERED_LIST=`(unzip -l "$1" | sed -e '1,3d' | head -n -2 | tr -s " " | cut -d " " -f5)` | ||
| ORDERED_LIST=`(zipinfo -1 "$1" | sort)` |
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 relies on zipinfo being installed on every executor machine. I'm not sure if that's the case for the Google RBE machines (zip and unzip are certainly installed). Is this a crucial change?
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.
zipinfo should already be installed given that the previous line invokes it. If it weren't, this would fail before getting here.
Some change needs to be made to this line, because head -n -2 isn't portable. head -n -2 could be replaced by tail -r | tail -n +3 | tail -r, but even that isn't strictly portable. It does appear to be more portable than head -n -2 though, because it works with BSD.
But understanding the purpose of this line, zipinfo -1 makes the most sense. And because it's used in the previous line, I think it's safe.
| if [ -f "$FILE" ]; then | ||
| sed -i 's/Databinding\\-processed\\-resources/databinding\\-processed\\-resources/g' "$FILE" | ||
| LC_ALL=C sed -i.bak 's/Databinding\\-processed\\-resources/databinding\\-processed\\-resources/g' "$FILE" | ||
| rm "${FILE}.bak" |
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.
If you're deleting the .bak file anyway, do you need -i.bak for the sed?
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.
Yes, this is described in my summary: BSD sed requires passing an argument to the -i flag for a file extension to use when writing a back-up. An invocation without a back-up must be explicitly signaled by using an empty argument, i.e. sed -i '' file. However, a spaced argument to -i on GNU sed is interpreted as the next positional argument to sed, which is an error. The only cross-platform way to accomplish this appears to be to combine it into one. This then needs to be removed. (From StackOverflow)
7408b25 to
1b3de35
Compare
As illustrated in bazelbuild#223, BSD `cp` does not support `--reflink=auto` flag. These functions are only used in the data binding rules, so we can actually just remove them. Instead, we can use symlinks in the data binding actions. n.b. Solely replacing the calls with symlinks and leaving the originals is something I'm not entirely opposed to. I do think it is a bad idea, however, because it is not cross-platform-compatible. It leaves the door open for future usages to call these copy functions and break things downstream for non-Linux users. However, if reverting utils.bzl is the only way to get this merged in, I'll do it. stacked-branch: databinding-symlinks
Without these prefixes, `StarlarkProcessDatabinding` fails with errors
about missing classes. These are all embedded in android_tools.jar, but
don't get pulled into the final jar.
```
ERROR: /Users/p/Code/bazel/rules_android/examples/databinding/java/com/basicapp/BUILD:10:16: Processing data binding failed: (Exit 1): java failed: error executing StarlarkProcessDatabinding command (from target //java/com/basicapp:basic_lib)
(cd /private/var/tmp/_bazel_p/828537623772ec0b3f803c69d3e68e34/sandbox/darwin-sandbox/211/execroot/_main && \
exec env - \
external/rules_java~~toolchains~remotejdk11_macos_aarch64/bin/java -Xms4G -Xmx4G -XX:+ExitOnOutOfMemoryError -jar bazel-out/darwin_arm64-opt-exec-ST-d57f47055a04/bin/external/rules_android~/src/tools/java/com/google/devtools/build/android/ResourceProcessorBusyBox_deploy.jar --tool PROCESS_DATABINDING -- --output_resource_directory bazel-out/darwin_arm64-fastbuild-android-ST-80dd4ae33d52/bin/java/com/basicapp/Databinding-processed-resources/basic_lib --resource_root java/com/basicapp/res --dataBindingInfoOut bazel-out/darwin_arm64-fastbuild-android-ST-80dd4ae33d52/bin/java/com/basicapp/databinding/basic_lib/layout-info.zip --appId com.basicapp --useDataBindingAndroidX true)
Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
Dec 26, 2024 4:38:32 PM com.google.devtools.build.android.ResourceProcessorBusyBox processRequest
SEVERE: Error during processing
java.lang.RuntimeException: java.lang.NoClassDefFoundError: org/antlr/v4/runtime/ParserRuleContext
at com.google.devtools.build.android.AndroidResourceProcessor.processDataBindings(AndroidResourceProcessor.java:318)
at com.google.devtools.build.android.AndroidDataBindingProcessingAction.main(AndroidDataBindingProcessingAction.java:113)
at com.google.devtools.build.android.ResourceProcessorBusyBox$Tool$13.call(ResourceProcessorBusyBox.java:140)
at com.google.devtools.build.android.ResourceProcessorBusyBox.processRequest(ResourceProcessorBusyBox.java:238)
at com.google.devtools.build.android.ResourceProcessorBusyBox.main(ResourceProcessorBusyBox.java:175)
Caused by: java.lang.NoClassDefFoundError: org/antlr/v4/runtime/ParserRuleContext
at android.databinding.tool.LayoutXmlProcessor.<init>(LayoutXmlProcessor.java:52)
at android.databinding.AndroidDataBinding.createXmlProcessor(AndroidDataBinding.kt:188)
at android.databinding.AndroidDataBinding.processResources(AndroidDataBinding.kt:113)
at android.databinding.AndroidDataBinding.doRun(AndroidDataBinding.kt:107)
at com.google.devtools.build.android.AndroidDataBindingWrapper.doRun(AndroidDataBindingWrapper.java:33)
at com.google.devtools.build.android.AndroidResourceProcessor.processDataBindings(AndroidResourceProcessor.java:316)
... 4 more
Caused by: java.lang.ClassNotFoundException: org.antlr.v4.runtime.ParserRuleContext
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581)
at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:527)
... 10 more
Exception in thread "main" java.lang.RuntimeException: java.lang.NoClassDefFoundError: org/antlr/v4/runtime/ParserRuleContext
at com.google.devtools.build.android.AndroidResourceProcessor.processDataBindings(AndroidResourceProcessor.java:318)
at com.google.devtools.build.android.AndroidDataBindingProcessingAction.main(AndroidDataBindingProcessingAction.java:113)
at com.google.devtools.build.android.ResourceProcessorBusyBox$Tool$13.call(ResourceProcessorBusyBox.java:140)
at com.google.devtools.build.android.ResourceProcessorBusyBox.processRequest(ResourceProcessorBusyBox.java:238)
at com.google.devtools.build.android.ResourceProcessorBusyBox.main(ResourceProcessorBusyBox.java:175)
Caused by: java.lang.NoClassDefFoundError: org/antlr/v4/runtime/ParserRuleContext
at android.databinding.tool.LayoutXmlProcessor.<init>(LayoutXmlProcessor.java:52)
at android.databinding.AndroidDataBinding.createXmlProcessor(AndroidDataBinding.kt:188)
at android.databinding.AndroidDataBinding.processResources(AndroidDataBinding.kt:113)
at android.databinding.AndroidDataBinding.doRun(AndroidDataBinding.kt:107)
at com.google.devtools.build.android.AndroidDataBindingWrapper.doRun(AndroidDataBindingWrapper.java:33)
at com.google.devtools.build.android.AndroidResourceProcessor.processDataBindings(AndroidResourceProcessor.java:316)
... 4 more
Caused by: java.lang.ClassNotFoundException: org.antlr.v4.runtime.ParserRuleContext
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581)
at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:527)
... 10 more
options ProcessXmlOptions{appId='com.basicapp', resInput=java/com/basicapp/res, resOutput=bazel-out/darwin_arm64-fastbuild-android-ST-80dd4ae33d52/bin/java/com/basicapp/Databinding-processed-resources/basic_lib/java/com/basicapp/res, layoutInfoOutput=/var/folders/mn/gbs17z997jvgrk53m0g2fgrh0000gn/T/android_data_binding_layout_info_tmp3836744348019465023, zipLayoutInfo=false, useAndroidX=true, enableDebugLogs=false}
Target //java/com/basicapp:basic_app failed to build
ERROR: /Users/p/Code/bazel/rules_android/examples/databinding/java/com/basicapp/BUILD:10:16 Compiling Android Resources in java/com/basicapp/basic_lib_symbols/symbols.zip failed: (Exit 1): java failed: error executing StarlarkProcessDatabinding command (from target //java/com/basicapp:basic_lib)
(cd /private/var/tmp/_bazel_p/828537623772ec0b3f803c69d3e68e34/sandbox/darwin-sandbox/211/execroot/_main && \
exec env - \
external/rules_java~~toolchains~remotejdk11_macos_aarch64/bin/java -Xms4G -Xmx4G -XX:+ExitOnOutOfMemoryError -jar bazel-out/darwin_arm64-opt-exec-ST-d57f47055a04/bin/external/rules_android~/src/tools/java/com/google/devtools/build/android/ResourceProcessorBusyBox_deploy.jar --tool PROCESS_DATABINDING -- --output_resource_directory bazel-out/darwin_arm64-fastbuild-android-ST-80dd4ae33d52/bin/java/com/basicapp/Databinding-processed-resources/basic_lib --resource_root java/com/basicapp/res --dataBindingInfoOut bazel-out/darwin_arm64-fastbuild-android-ST-80dd4ae33d52/bin/java/com/basicapp/databinding/basic_lib/layout-info.zip --appId com.basicapp --useDataBindingAndroidX true)
```
stacked-branch: databinding_exec_jar-fixes
This adds a flag for AndroidX and wires it through to the processing
steps. Additionally, the shell command run to fix databinding compiled
resources was not cross-platform:
* BSD `head` won't take a negative int as its argument (GNU does).
The better way to accomplish this goal here (listing all files in
the archive) is just to use `zipinfo(1)`. `zipinfo -1` shows in its
man page to be intended for exactly this purpose.
* BSD `sed` requires passsing an argument to the `-i` flag for a file
extension to use when writing a back-up. A file without a back-up
must be explicitly signaled by using an empty argument, i.e.
`sed -i '' file`. However, a spaced argument to `-i` on GNU sed is
interpreted as the next positional argument to `sed`, which is an
error. The only cross-platform way to accomplish this appears to be
to combine it into one like `-i.bak`. This then needs to be removed.
(From [StackOverflow](https://stackoverflow.com/a/22084103/1819790))
* Using `sed` to change a string in an otherwise binary file may require
changing the localization variables. At least, it does on standard
BSD/macOS: otherwise, `sed` assumes files are encoded as text, and
the class files contain invalid bytes. `LC_ALL` overrides all
categories not set.
1b3de35 to
f055e27
Compare
f055e27 to
bb7cdb9
Compare
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.
@ted-xie FYI changes to this file are new (as of yesterday). I realized that AARs were not getting their data binding resources processed—specifically, AARs can have setter_store.json files (e.g. for data binding adapters) that were getting processed in native rules, but getting dropped on the floor here. This wasn't causing issues in the trivial data binding example here, but was preventing building our main app until I discovered.
|
Hi @ted-xie, any update on internal tests? |
|
Hi Prasanna, I haven't forgotten about your PR! It's taken quite a while to fix up to an acceptable state internally, mainly due to our complicated PR import process and some impedance mismatches w.r.t. our internal databinding tests/implementation. |
This gets databinding functioning. ...With a caveat: this does not work on Windows. Worth noting that I didn't break Windows support; it never would have worked because of all the raw shell commands. If it is a requirement to have Windows working, it will take a lot more effort. But I hope that doesn't need to block this: working on Unix is better than not working at all, anywhere. I've tried to split it up into a bunch of discrete commits. I can stack them each into a single PR if that is preferable, but I figured we would start with one PR and go from there. In order:
Use symlinks for databinding copy_{file,dir} functions
As illustrated in _copy_file and _copy_dir fail on macOS #223, BSD
cpdoes not support--reflink=autoflag. These functions are only used in the data binding rules, so we can actually just remove them and use symlinks in the data binding actions.1Include other necessary prefixes in databinding_exec.jar
Without these prefixes,
StarlarkProcessDatabindingfails with errors about missing classes.2 These are all embedded in android_tools.jar, but don't get pulled into the final jar. This in particular is what makes me think that data binding never worked for anyone as-is.Fix databinding templates and resources for AndroidX
AFAICT, the AndroidX flag isn't respected anywhere; instead, it is just hard-coded as false here. This flips it to true. Additionally, this requires a change in the template. If this actually does need to be customizable, I can wire up some of the flags accordingly. But, given that data binding just didn't work on any platform (without the previous commit), I can't imagine it is too controversial to set androidx=true.
Additionally, the shell command run to fix databinding compiled resources was not cross-platform:
headwon't take a negative int as its argument (GNU does). The better way to accomplish this goal here (listing all files in the archive) is just to usezipinfo(1).zipinfo -1shows in its man page to be intended for exactly this purpose.sedrequires passsing an argument to the-iflag for a file extension to use when writing a back-up. A file without a back-up must be explicitly signaled by using an empty argument, i.e.sed -i '' file. However, a spaced argument to-ion GNU sed is interpreted as the next positional argument tosed, which is an error. The only cross-platform way to accomplish this appears to be to combine it into one like-i.bak. This then needs to be removed. (From StackOverflow)sedto change a string in an otherwise binary file may require changing the localization variables. At least, it does on standard BSD/macOS: otherwise,sedassumes files are encoded as text, and the class files contain invalid bytes.LC_ALLoverrides all categories not set.Add example that builds
Add CI target for said example
Footnotes
n.b. Solely replacing the calls with symlinks and leaving the originals is something I'm not entirely opposed to. I do think it is a bad idea, however, because it is not cross-platform-compatible. It leaves the door open for future usages to call these copy functions and break things downstream for non-Linux users. However, if reverting utils.bzl is the only way to get this merged in, I'll do it. ↩