-
Notifications
You must be signed in to change notification settings - Fork 10
Building this repo, passing tests? #1
Comments
Wow, this is super long. Let me first ask the quick question first: are you trying to build anything beyond bazel_build_test.sh? I don't recommend ever running beyond that and digging problems without talking to us since you might be wasting time; because if it is not there that it means you are trying to build something probably that won't build. In this particular case; I see no j2cl targets in bazel_build_test.sh and I need first to check why that's the case. |
Just checked, j2cl part is not properly open-sourced. Will re-open the bug. |
(And pls don't waste time trying to build, something went really wrong) |
Haha great, thank you! I'll go ahead and close, can re-try when it is ready? I assumed that since bazel_build_test.sh never touched the j2cl content that this was deliberate - either it wasn't "ready enough" or that I was entirely misunderstanding what the j2cl package was even for (and further that this report might be helpful in understanding later in continuing to make parts of the project accessible in open source). Your confidence in the other issue in closing made me certain that I was doing things wrong, which is why I pressed on... |
Yes, I was very confident :) Apparently there was misunderstanding happened a while back between me and Julien. This should be fixed soon. |
It looks like google/j2cl#71 was closed, but no updates in quite a while, and things still don't seem to build properly. I'm about 6 hours in to trying to get things working properly over the last week, and thought it was time to stop and write some notes. I'll distill them here into a few sections, but likely this ticket should be closed and several others opened, or maybe there is just some unsynced changes, etc - let me know how we should proceed.
--
Build failure in protoc plugin prevents building some tests, local issue?
First issue is probably a local problem, but I do not know enough about bazel to be certain. Starting from the
bazel_build_test.sh
file run as part of travisci, which I see passed, I tried to build the repo, and found that the second command failed. Specifically in theprotoc-gen-immutable_js_protobuf
plugin:Assuming this passes on a machine other than mine, it points to a local problem somehow, but I can skip building the tests in
javatests/com/google/protobuf/contrib/immutablejs/integration/
and.../protos/
to avoid this.--
Empty test JS causes all tests to fail
Building tests in
javatests/com/google/protobuf/contrib/immutablejs/runtime/
asbazel_build_test.sh
instructs does indeed pass, but of the tests themselves there are 10 failures (and I think that there are only 10). These failures may be spurious, just an indication of a setup problem, as the error appear to be that the closure test wiring is missing:There appear to be 5 test js files, and
proto_jsunit_test
declares that twoclosure_js_test
are created for each, so I think that is all 10 tests? The command I'm using to reproduce this isframed from the original
bazel build
for javatests in thebazel_build_test.sh
file. Examining the test js that is generated by the build, it looks like in ADVANCED, the entire test is compiled out -bazel-bin/javatests/com/google/protobuf/contrib/immutablejs/runtime/bytestring_test_compiled_bin.js
and the other*_compiled_bin.js
files are as well, and while the WHITESPACE_ONLY variants named*_bin.js
are not empty, they look to only contain the contents of base.js from closure-library, rather than also the test itself and the other testing wiring expected. Examining the*.js-0.param
files to see if jscomp was being invoked incorrectly somehow, or if it was missing important files has led me to nothing to point at specifically - I don't know as much as I'd like about how this part works, will continue exploring why the tests are not generating output properly after I finish this writeup.--
BUILD issues in java
Next, I avoided the tests and try to simply build all provided Java sources. While the .sh file did build the
immutablejs/...
rules, it skipped allj2cl/...
rules entirely. Likely I'm misunderstanding how this repo is meant to be used, but it appears that these packages are needed to have actual j2cl/java that can be used, rather than just generate .js files? Up until this point I was ready to believe that my own local bazel 2.2.0 install (the required version) might have some severe problems, but from digging into the issues below I don't think to repo is ready. If on the other hand none of this code is required to use j2cl-protobuf, I'm going to look a little silly for having done all this work... if that's the case, perhaps this directjava/com/google/protobuf/contrib/j2cl/options/BUILD
includes this line:load("//java/com/google/protobuf/contrib/gwt:GwtProtoLibrary.bzl", "GwtProtoLibrary")
. That file doesn't seem to correspond to a package - perhaps this line shouldn't have been synced, or thegwt/
package is missing? Commenting out thisload()
and theGwtProtoLibrary()
invocation at the end of the file lets us ignore this at least, thejs_enum-gwt
rule doesn't ever seem to be referenced anyway?load("@bazel_tools//tools/build_defs/proto/cpp:cc_proto_library.bzl", "cc_proto_library")
load results in an error. The correct path appears to beload("@rules_cc//cc:defs.bzl", "cc_proto_library")
, with no other changes to WORKSPACE or other rules.java/com/google/protobuf/contrib/j2cl/runtime/BUILD
has this load:load("//third_party/java/j2cl:j2cl_library.bzl", "j2cl_library")
. My understanding of bazel says that this should point to a directory java/j2cl within third_party, but nothing exists there today. As mentioned, by bazel knowledge is weak, but taking cues from the earlier cross-repo locas, it appears this line can be replaced with insteadload("@com_google_j2cl//build_defs:rules.bzl", "j2cl_library")
.java/com/google/protobuf/contrib/j2cl/BUILD
java/com/google/protobuf/contrib/j2cl/BUILD
, theload("@bazel_tools//tools/build_defs/testing:bzl_library.bzl", "bzl_library")
doesn't resolve. It appears this is meant to point at https://github.com/bazelbuild/bazel-skylib/blob/master/bzl_library.bzl, perhaps referenced as simply asload("@bazel_skylib//:bzl_library.bzl", "bzl_library")
?cc_api_version = 2
passed to the proto_library() rule doesn't seem to be valid in open source? It doesn't seem to be a feature of newer bazel either as far as I can tell at https://docs.bazel.build/versions/master/be/protocol-buffer.html#proto_library. So far I am just commenting it out.cc_api_version
arg needs to be removed fromjava/com/google/protobuf/contrib/j2cl/options/BUILD
's proto_library invocation.Okay, at this point all the load()s seem to work and the rule declarations themselves are valid.
java/com/google/protobuf/contrib/j2cl/runtime/BUILD
'sruntime
rule allows visibility to//java/com/google/protobuf/contrib/gwt:__pkg__
, as above withGwtProtoLibrary
. Commenting this out seems to resolve it.java/com/google/protobuf/contrib/j2cl/options/BUILD
has ajs_enum
proto_library rule and ajs_enum_cc_proto
cc_proto_library rule. Both have a compatible_with attr with labels that point to//buildenv/target:non_prod
within this repository, but the files are missing? My understanding of proto_library is that this can be removed for the moment at least.js_enum
rule also has a dependency on//net/proto2/proto:descriptor
, which also does not exist. Perhaps this is a general rule that can be referenced in@rules_cc
or something? My knowledge of protobuf doesn't extend far into bazel, but I'm pretty sure that removing this is going to cause a problem later, sincejava/com/google/protobuf/contrib/j2cl/options/js_enum.proto
has an import on a very similar looking path.java/com/google/protobuf/contrib/j2cl/BUILD
has the same problem. Commenting out the dependency gets bazel to move further, but will certainly cause a problem later.java/com/google/protobuf/contrib/j2cl/runtime/BUILD
the j2cl_libraryjspb-utils
has a dependency on//third_party/java/gwt:gwt-jsinterop-annotations-j2cl
. Since J2CL is already added as a repository but gwt isn't, it looks as though this could instead be@com_google_j2cl//third_party:gwt-jsinterop-annotations-j2cl
.//third_party/java/jsinterop:jsinterop-base-j2cl
doesn't exist either, but we'll need a new repository added for this, following the instructions at https://github.com/google/jsinterop-base#bazel-dependency//third_party/java/jsr305_annotations:jsr305_annotations-j2cl
: this one I am much less sure of than the other fixes - //third_party already has a jsr305 jar, but this missing target calls for one with -j2cl at the end. For the moment I replaced this with@com_google_j2cl//third_party:jsr305_annotations
runtime
rule in the same file, further up.java/com/google/protobuf/contrib/j2cl/BUILD
has the same compatible_with issue as above.java/com/google/protobuf/contrib/j2cl/generator/BUILD
! First time we've made it far enough to attempt to build this file. There are several non-existent directories that are being referenced here://java/com/google/common
should instead I think be a third_party reference to guava, there is noBUILD
file in//java/com/google/protobuf
so that dependency doesn't make sense (probably also external? but to what exactly?), along with//java/com/google/protobuf/contrib
which has no BUILD file, so I'm not sure what we're depending on there.//net/proto2/compiler/proto:plugin_java_proto
likely needs to point to the same place as the other//net/proto2
references above. Finally, the last two lines should simply be//third_party:auto_value
and//third_party:jakarta_velocity
respectively, to point to aliases that already exist in third_party/BUILD.And with that, we're on to some visiblity issues - first, my attempt above to point to
jsr305_annotations
inside of j2cl was a mistake, and while it looks less wrong to point to//third_party:jsr305_annotations
instead, this fails for the reason I had feared above: this isn't specific to J2CL:--
I'm going to stop here, will revisit as time permit to see if I can further shake this out. Let me know if I've done something tremendously stupid here in trying to get things working, or if there is a better way to get these issues resolve so we can use this in open source.
The text was updated successfully, but these errors were encountered: