Issue metadata
Sign in to add a comment
|
Fuchsia makes src/build depend on src/tools |
||||||||||||||||||||||
Issue descriptionFuchsia introduced a new dependency to src/tools/fuchsia/fidlgen from src/build. The dependency was added here: https://crrev.com/c/1222697 It first started to become a problem in the current V8 Deps roll: https://chromium-review.googlesource.com/c/v8/v8/+/1295790 See failed compile: https://ci.chromium.org/p/v8/builders/luci.v8.try/v8_fuchsia_rel_ng/b8931891488544978208 More dependencies reaching out of src/build make src/build hard to share across projects. I see these options: 1. Figure out if V8 needs fidlgen at all, and if not, put it behind a flag that can be turned off in V8 builds. 2. Remove the dependency to src/tools. E.g. move relevant files to src/build. 3. (only if the other 2 aren't feasible) Make a subtreed mirror for src/tools/fuchsia so that V8 can depend on it via DEPS file. P1 since this blocks V8 deps update.
,
Oct 23
Re #1: To unblock the V8 roll, we could simply disable JS generation in fidl_library() until we've cleaned things up? Moving the FIDL/JS stuff into //build SGTM, or we could consider up-streaming it to be part of the Fuchsia SDK...
,
Oct 23
Upstreaming would be more work, because we added dependencies on base. I will prepare a move CL. If that's going to take > 30min, I'll disable it and turn off the tests.
,
Oct 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9870446b832d97d5c51e432962d3ac5c9602245c commit 9870446b832d97d5c51e432962d3ac5c9602245c Author: Scott Graham <scottmg@chromium.org> Date: Tue Oct 23 21:44:51 2018 fuchsia: Move fidlgen_js to //build to remove dep from //build->//tools This is to unblock the v8 roll, which consumes //build separately from Chromium. TBRing for DEPS include_rules change, because it's a straight move, not a new addition. TBR: jochen@chromium.org Bug: 898088 Change-Id: Ic93764169680fc4e0680cc28db327a04a59750c8 Reviewed-on: https://chromium-review.googlesource.com/c/1297078 Commit-Queue: Scott Graham <scottmg@chromium.org> Reviewed-by: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#602114} [modify] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/BUILD.gn [modify] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/config/fuchsia/fidl_library.gni [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/BUILD.gn [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/DEPS [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/fidl.py [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/gen.py [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/runtime/fidl.mjs [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/runtime/zircon.cc [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/runtime/zircon.h [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/test/fidlgen_js_unittest.cc [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/test/simple.fidl [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/third_party/__init__.py [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/third_party/enum34/LICENSE [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/third_party/enum34/README.chromium [rename] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/build/fuchsia/fidlgen_js/third_party/enum34/__init__.py [modify] https://crrev.com/9870446b832d97d5c51e432962d3ac5c9602245c/testing/buildbot/gn_isolate_map.pyl
,
Oct 24
After rolling forward, the old problem is gone, but a few new ones appeared: https://chromium-review.googlesource.com/c/v8/v8/+/1297681 Could you take a look? Do we need to patch this on V8 side somehow? I see two sorts of errors: - A bunch of NotImplementedErrors thrown from the fidlgen_js/gen.py script. - And this: ../../third_party/fuchsia-sdk/sdk/pkg/images_cpp/include/lib/images/cpp/images.h:8:10: fatal error: 'fuchsia/images/cpp/fidl.h' file not found #include <fuchsia/images/cpp/fidl.h>
,
Oct 24
(Sorry, have been in and out the last couple days.) It looks like all of the SDKs FIDL is being compiled for JS, but I'm not sure why. The fidl_library() template is supposed to only default to building bindings "cpp" unless "js" is added to languages. I'm looking into it now, but I'm not sure how to build v8 vs. this roll so might take me a bit.
,
Oct 24
Oh, I wonder if it's because v8 builds as plain `ninja` rather than a particular set of targets. We haven't been able to build like that for Fuchsia, so perhaps some non-referenced targets are causing problems. I should be able to repro that by building a specific target...
,
Oct 24
Yeah, this https://chromium-review.googlesource.com/c/chromium/src/+/1298223 will fix some of those errors I believe (the NotImplementeds). I'm less certain about these though. It's possible that the broken generation caused the process to error out, but it doesn't exactly look like that. FAILED: obj/third_party/fuchsia-sdk/sdk/scenic_cpp/commands.o /b/swarming/w/ir/cache/goma/client/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/third_party/fuchsia-sdk/sdk/scenic_cpp/commands.o.d -DV8_DEPRECATION_WARNINGS -DDCHECK_ALWAYS_ON=1 -DUSE_AURA=1 -DUSE_OZONE=1 -DNO_TCMALLOC -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DFUCHSIA_SDK_VERSION=05a1d1f99b5b8b4b8edb3097564b0653ec0f52bd -DCR_CLANG_REVISION=\"344066-1\" -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS -DCR_LIBCXX_REVISION=344254 -DCR_LIBCXXABI_REVISION=344215 -D_LIBCPP_ENABLE_NODISCARD -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../.. -Igen -I../../third_party/fuchsia-sdk/sdk/pkg/scenic_cpp/include -I../../third_party/fuchsia-sdk/sdk/pkg/fidl_cpp/include -I../../third_party/fuchsia-sdk/sdk/pkg/fidl_cpp_sync/include -I../../third_party/fuchsia-sdk/sdk/pkg/fidl/include -I../../third_party/fuchsia-sdk/sdk/pkg/async/include -I../../third_party/fuchsia-sdk/sdk/pkg/fit/include -I../../third_party/fuchsia-sdk/sdk/pkg/zx/include -I../../third_party/fuchsia-sdk/sdk/pkg/async-default/include -I../../third_party/fuchsia-sdk/sdk/pkg/images_cpp/include -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -fcolor-diagnostics -fmerge-all-constants -Xclang -mllvm -Xclang -instcombine-lower-dbg-declare=0 -no-canonical-prefixes -fcomplete-member-pointers --target=x86_64-fuchsia -fno-sanitize=safe-stack -m64 -march=x86-64 -Wall -Werror -Wextra -Wimplicit-fallthrough -Wthread-safety -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-unneeded-internal-declaration -Wno-undefined-var-template -Wno-null-pointer-arithmetic -Wno-ignored-pragma-optimize -Wno-defaulted-function-deleted -Oz -fno-ident -fdata-sections -ffunction-sections -fomit-frame-pointer -g1 -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang enforce-in-thirdparty-webkit -Xclang -plugin-arg-find-bad-constructs -Xclang check-enum-max-value -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -std=c++14 -fno-exceptions -fno-rtti -nostdinc++ -isystem../../buildtools/third_party/libc++/trunk/include -isystem../../buildtools/third_party/libc++abi/trunk/include --sysroot=../../third_party/fuchsia-sdk/sdk/arch/x64/sysroot -fvisibility-inlines-hidden -c ../../third_party/fuchsia-sdk/sdk/pkg/scenic_cpp/commands.cc -o obj/third_party/fuchsia-sdk/sdk/scenic_cpp/commands.o In file included from ../../third_party/fuchsia-sdk/sdk/pkg/scenic_cpp/commands.cc:5: ../../third_party/fuchsia-sdk/sdk/pkg/scenic_cpp/include/lib/ui/scenic/cpp/commands.h:10:10: fatal error: 'fuchsia/images/cpp/fidl.h' file not found #include <fuchsia/images/cpp/fidl.h> ^~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. We probably need to fix `all` for Chromium Fuchsia to make this more obvious.
,
Oct 24
Oh, actually I can repro something similar in Chromium with this command line: ninja -C out/fuch -j1000 obj/third_party/fuchsia-sdk/sdk/scenic_cpp/host_memory.o ninja: Entering directory `out/fuch' [0->1/1 ~1] CXX obj/third_party/fuchsia-sdk/sdk/scenic_cpp/host_memory.o FAILED: obj/third_party/fuchsia-sdk/sdk/scenic_cpp/host_memory.o /usr/local/google/home/scottmg/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/third_party/fuchsia-sdk/sdk/scenic_cpp/host_memory.o.d -DV8_DEPRECATION_WARNINGS -DDCHECK_ALWAYS_ON=1 -DUSE_AURA=1 -DUSE_OZONE=1 -DNO_TCMALLOC -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DFUCHSIA_SDK_VERSION=05a1d1f99b5b8b4b8edb3097564b0653ec0f52bd -DCR_CLANG_REVISION=\"344066-1\" -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS -DCR_LIBCXX_REVISION=344254 -DCR_LIBCXXABI_REVISION=344215 -D_LIBCPP_ENABLE_NODISCARD -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -I../.. -Igen -I../../third_party/fuchsia-sdk/sdk/pkg/scenic_cpp/include -I../../third_party/fuchsia-sdk/sdk/pkg/fidl_cpp/include -I../../third_party/fuchsia-sdk/sdk/pkg/fidl_cpp_sync/include -I../../third_party/fuchsia-sdk/sdk/pkg/fidl/include -I../../third_party/fuchsia-sdk/sdk/pkg/async/include -I../../third_party/fuchsia-sdk/sdk/pkg/fit/include -I../../third_party/fuchsia-sdk/sdk/pkg/zx/include -I../../third_party/fuchsia-sdk/sdk/pkg/async-default/include -I../../third_party/fuchsia-sdk/sdk/pkg/images_cpp/include -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -fcolor-diagnostics -fmerge-all-constants -Xclang -mllvm -Xclang -instcombine-lower-dbg-declare=0 -no-canonical-prefixes -fcomplete-member-pointers --target=x86_64-fuchsia -fno-sanitize=safe-stack -m64 -march=x86-64 -Wall -Werror -Wextra -Wimplicit-fallthrough -Wthread-safety -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-unneeded-internal-declaration -Wno-undefined-var-template -Wno-null-pointer-arithmetic -Wno-ignored-pragma-optimize -Wno-defaulted-function-deleted -O0 -fno-omit-frame-pointer -g2 -ggnu-pubnames -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang enforce-in-thirdparty-webkit -Xclang -plugin-arg-find-bad-constructs -Xclang check-enum-max-value -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -std=c++14 -fno-exceptions -fno-rtti -nostdinc++ -isystem../../buildtools/third_party/libc++/trunk/include -isystem../../buildtools/third_party/libc++abi/trunk/include --sysroot=../../third_party/fuchsia-sdk/sdk/arch/x64/sysroot -fvisibility-inlines-hidden -c ../../third_party/fuchsia-sdk/sdk/pkg/scenic_cpp/host_memory.cc -o obj/third_party/fuchsia-sdk/sdk/scenic_cpp/host_memory.o In file included from ../../third_party/fuchsia-sdk/sdk/pkg/scenic_cpp/host_memory.cc:5: In file included from ../../third_party/fuchsia-sdk/sdk/pkg/scenic_cpp/include/lib/ui/scenic/cpp/host_memory.h:14: In file included from ../../third_party/fuchsia-sdk/sdk/pkg/scenic_cpp/include/lib/ui/scenic/cpp/resources.h:8: ../../third_party/fuchsia-sdk/sdk/pkg/scenic_cpp/include/lib/ui/scenic/cpp/session.h:8:10: fatal error: 'fuchsia/images/cpp/fidl.h' file not found #include <fuchsia/images/cpp/fidl.h> ^~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. ninja: build stopped: subcommand failed. So I think this might actually be unrelated to my breakage, but is instead different breakage. (yay!) Possibly due to https://chromium.googlesource.com/chromium/src/third_party/fuchsia-sdk/+log/bac0433..3014f28 Kevin? I'm going to try to hack an all build now, and will take an action item to get Chromium to have a bot that builds all targets if it's reasonably possible.
,
Oct 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5261741e6a05338095fcf39a4cb9b9467afd1f4a commit 5261741e6a05338095fcf39a4cb9b9467afd1f4a Author: Scott Graham <scottmg@chromium.org> Date: Wed Oct 24 18:00:23 2018 fuchsia: Don't define fidl js (or cpp) compile action unless necessary Bug: 898088 Change-Id: Iaf7c0013ff0c7fa64f008023619962526bc41b81 Reviewed-on: https://chromium-review.googlesource.com/c/1298223 Reviewed-by: Wez <wez@chromium.org> Commit-Queue: Wez <wez@chromium.org> Commit-Queue: Scott Graham <scottmg@chromium.org> Cr-Commit-Position: refs/heads/master@{#602394} [modify] https://crrev.com/5261741e6a05338095fcf39a4cb9b9467afd1f4a/build/config/fuchsia/fidl_library.gni
,
Oct 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e2569db721bb3bd6b12991d581c60933bf555f30 commit e2569db721bb3bd6b12991d581c60933bf555f30 Author: Kevin Marshall <kmarshall@chromium.org> Date: Wed Oct 24 18:26:51 2018 Fuchsia: Generate deps entries for SDK cc_source_lib's "fidl_deps". Fuchsia SDK source library manifests have a special entry just for FIDL dependencies that is separate from the other dependencies. Use it! Bug: 898088 Change-Id: Iab2fdbdefbb3b6cbf169aac81860e698108bd2a6 Reviewed-on: https://chromium-review.googlesource.com/c/1297467 Reviewed-by: Scott Graham <scottmg@chromium.org> Commit-Queue: Kevin Marshall <kmarshall@chromium.org> Cr-Commit-Position: refs/heads/master@{#602403} [modify] https://crrev.com/e2569db721bb3bd6b12991d581c60933bf555f30/third_party/fuchsia-sdk/gen_build_defs.py
,
Oct 24
@scottmg - do you mean an "all" build on Fuchsia? We should be building all on most if not all of the other platforms already.
,
Oct 24
Yes, I meant an all build on Fuchsia in particular. Wez pointed me to bug 808287 , looks like it's a bit of work still unfortunately so I'll poke at some of those. machenbach also just retriggered the roll attempt for the immediate problem.
,
Oct 24
The latest roll attempt failed https://chromium-review.googlesource.com/c/v8/v8/+/1298008/2 because git clone https://chromium.googlesource.com/chromium/src/third_party/fuchsia-sdk hasn't mirrored yet (bleh). I don't know how often that happens. I made the same changes locally and confirmed in a `fetch v8` tree with the same-as-bot args, that building all will succeed on Fuchsia after that's mirrored.
,
Oct 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/664dd09a707cf0d8e44a1f54acecfe67107d831d commit 664dd09a707cf0d8e44a1f54acecfe67107d831d Author: Scott Graham <scottmg@chromium.org> Date: Wed Oct 24 20:04:45 2018 fuchsia: Fix #include for ntohl/htonl Bug: 808287 , 898088 Change-Id: I7aa9dd73aa405e32a46118dfe8474c7849471caf Reviewed-on: https://chromium-review.googlesource.com/c/1297460 Reviewed-by: Nathan Parker <nparker@chromium.org> Commit-Queue: Scott Graham <scottmg@chromium.org> Cr-Commit-Position: refs/heads/master@{#602417} [modify] https://crrev.com/664dd09a707cf0d8e44a1f54acecfe67107d831d/components/safe_browsing/db/v4_rice.cc
,
Oct 25
https://chromium-review.googlesource.com/c/v8/v8/+/1298391 looks like it will pass the high bar of the CQ. (At least on Fuchsia!)
,
Oct 25
It does! Thanks for fixing! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by scottmg@chromium.org
, Oct 23Status: Started (was: Untriaged)