[GN] Make android special treatments explicit, instead of relying on target's name ending with '_module'. |
|||||
Issue description
As part of a IWYU cleanup, I added a seemingly innocent dependency to a WebRTC target [1].
It boils down to:
rtc_test("rtc_media_unittests") {
testonly = true
deps = [
"../modules/video_capture:video_capture_module"
[...]
}
A subsequent:
% gn gen out/android --args='target_cpu="arm" target_os="android"'
led to:
ERROR Unresolved dependencies.
//media:rtc_media_unittests_apk__build_config(//build/toolchain/android:android_clang_arm)
needs //modules/video_capture:video_capture_module__build_config(//build/toolchain/android:android_clang_arm)
An investigation from mbonadei@ revealed that this behavior was triggered by the name of the dependency ending with "_module" [2].
Indeed, renaming it fixes the issue.
We believe this is quite surprising and error prone.
The feature requested here is to replace this suffix matching by an explicit trigger (e.g. an attribute of the target).
Thank you for your attention!
[1] https://webrtc-review.googlesource.com/c/src/+/111965/1/media/BUILD.gn#565
[2] https://cs.chromium.org/chromium/src/build/config/android/rules.gni
,
Nov 27
> shrinking the whitelist of java names The doc says: # These identify targets that have .build_config files Should we read "that **must** have .build_config files"? Otherwise why not relying on the existence of .build_config directly?
,
Nov 27
I don't think there's a difference between "have .build_config files" and "must have .build_config files". The whitelist allows write_build_config() targets to depend on only its child write_build_config() targets. This restriction was put in so that generate_gradle.py can quickly build all .build_config files that exist in the codebase without having to first build every target. tiborg - could you look at removing "_module" from the list? It's probably to generic of a term. Maybe use "apk_module" or "android_module"?
,
Nov 27
Yeah, can look into it. Renaming shouldn't be too bad at this point. We only have like five modules so far.
,
Dec 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cec62592a29d5d33fe9049a470dab57a247f9fb1 commit cec62592a29d5d33fe9049a470dab57a247f9fb1 Author: Andrew Grieve <agrieve@chromium.org> Date: Thu Dec 06 03:31:30 2018 Android: Rename __build_config targets to __build_config_crbug_908819 Goal is to surface give a pointer in "missing dependency" error messages about why the non-existent dependency was added. TBR=agrieve # Trivial rename Bug: 908819 Change-Id: I42a7b791658101985ec90afe85cbfe54de86b8df Reviewed-on: https://chromium-review.googlesource.com/c/1361933 Commit-Queue: agrieve <agrieve@chromium.org> Reviewed-by: Eric Stevenson <estevenson@chromium.org> Cr-Commit-Position: refs/heads/master@{#614262} [modify] https://crrev.com/cec62592a29d5d33fe9049a470dab57a247f9fb1/android_webview/glue/generate_resource_rewriter.gni [modify] https://crrev.com/cec62592a29d5d33fe9049a470dab57a247f9fb1/build/android/docs/build_config.md [modify] https://crrev.com/cec62592a29d5d33fe9049a470dab57a247f9fb1/build/android/gradle/generate_gradle.py [modify] https://crrev.com/cec62592a29d5d33fe9049a470dab57a247f9fb1/build/config/android/internal_rules.gni [modify] https://crrev.com/cec62592a29d5d33fe9049a470dab57a247f9fb1/build/config/android/rules.gni
,
Dec 7
The commit above should help the overall problem quite a bit. We should still work to remove "module" from the whitelist though.
,
Dec 14
Issue 912188 has been merged into this issue.
,
Jan 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d266dd02e8427af58cfdde1578465d785a839d22 commit d266dd02e8427af58cfdde1578465d785a839d22 Author: Tibor Goldschwendt <tiborg@chromium.org> Date: Thu Jan 10 18:09:07 2019 [bundles] Rename module targets from *_module to *_bundle_module *_module is too generic of a name and conflicts with non-module targets. Bug: 908819 Change-Id: I974bb3b95f609d08bf324d51ab221fb7fc3d1e07 Reviewed-on: https://chromium-review.googlesource.com/c/1404753 Reviewed-by: agrieve <agrieve@chromium.org> Commit-Queue: Tibor Goldschwendt <tiborg@chromium.org> Cr-Commit-Position: refs/heads/master@{#621636} [modify] https://crrev.com/d266dd02e8427af58cfdde1578465d785a839d22/build/config/android/internal_rules.gni [modify] https://crrev.com/d266dd02e8427af58cfdde1578465d785a839d22/chrome/android/BUILD.gn
,
Jan 10
The following revision refers to this bug: https://chrome-internal.googlesource.com/clank/internal/apps/+/dca88f7419f7b8075e4298f761eaca7fed9f1c5b commit dca88f7419f7b8075e4298f761eaca7fed9f1c5b Author: Tibor Goldschwendt <tiborg@google.com> Date: Thu Jan 10 20:03:36 2019
,
Jan 11
,
Jan 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/95d39d05956ef3762076cb1ec843240455788856 commit 95d39d05956ef3762076cb1ec843240455788856 Author: Tibor Goldschwendt <tiborg@chromium.org> Date: Fri Jan 11 16:50:28 2019 [modules] Remove *_module from build config whitelist Bug: 908819 Change-Id: I23a9f9f3f7439e6d34223321e93da2dee5bb6cbf Reviewed-on: https://chromium-review.googlesource.com/c/1406938 Commit-Queue: Tibor Goldschwendt <tiborg@chromium.org> Commit-Queue: agrieve <agrieve@chromium.org> Reviewed-by: agrieve <agrieve@chromium.org> Cr-Commit-Position: refs/heads/master@{#622030} [modify] https://crrev.com/95d39d05956ef3762076cb1ec843240455788856/build/config/android/internal_rules.gni
,
Jan 11
Just _module suffix should be fine now. Don't use _bundle_module suffix though, which is the suffix not for bundle modules.
,
Jan 15
The following revision refers to this bug: https://webrtc.googlesource.com/src.git/+/29cbd85ccef34430382cf9e6b9a4e91590041b23 commit 29cbd85ccef34430382cf9e6b9a4e91590041b23 Author: Mirko Bonadei <mbonadei@webrtc.org> Date: Tue Jan 15 13:25:58 2019 Fix rtc_media_unittests deps ( crbug.com/908819 is now fixed). It is now possible to have dependencies with suffix _module without clashing with the Android GN templates. Bug: chromium:908819 Change-Id: I1b34aac60af93485ce23ebce295ab97c7c163d20 Reviewed-on: https://webrtc-review.googlesource.com/c/117161 Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org> Reviewed-by: Oleh Prypin <oprypin@webrtc.org> Cr-Commit-Position: refs/heads/master@{#26265} [modify] https://crrev.com/29cbd85ccef34430382cf9e6b9a4e91590041b23/media/BUILD.gn
,
Jan 15
,
Jan 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/256f7db493ba0c6a2e9be58980280efffb4dc778 commit 256f7db493ba0c6a2e9be58980280efffb4dc778 Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Date: Wed Jan 16 02:13:53 2019 Roll src/third_party/webrtc dcfe484f2e12..ccc1b57e32bd (20 commits) https://webrtc.googlesource.com/src.git/+log/dcfe484f2e12..ccc1b57e32bd git log dcfe484f2e12..ccc1b57e32bd --date=short --no-merges --format='%ad %ae %s' 2019-01-15 mirtad@webrtc.org Poll is_hardware_accelerated from VideoEncoder instead of VideoEncoderFactory. 2019-01-15 mbonadei@webrtc.org Fix rtc_media_unittests deps ( crbug.com/908819 is now fixed). 2019-01-15 mbonadei@webrtc.org Remove comments about using std::shared_ptr. 2019-01-15 nisse@webrtc.org Delete setting of unused variable nack_window_ms 2019-01-15 ilnik@webrtc.org Revert "Ensure correct decoding for unfiltered KSVC streams" 2019-01-15 eladalon@webrtc.org Avoid repeated semi-expensive field_trials read in frame_buffer2.cc 2019-01-15 chromium-webrtc-autoroll@webrtc-ci.iam.gserviceaccount.com Roll chromium_revision 783044b798..c6a258bb5d (621838:622808) 2019-01-15 alessiob@webrtc.org RNN VAD: fix pitch gain type and change pitch period type 2019-01-15 ilnik@webrtc.org Fix fuzzer identified crash in DecodeFramesHistory 2019-01-15 mbonadei@webrtc.org Roll //third_party/ffmpeg and disable H264 when MSVC is used. 2019-01-15 nisse@webrtc.org Don't set the screenshare flag on FakeVideoCapturerWithTaskQueue 2019-01-15 benwright@webrtc.org Added JSON generator for VideoReceiveStream::Config objects. 2019-01-14 qiangchen@chromium.org Bug Fix: iOS H264 Encoder Crash Issue 2019-01-14 mbonadei@webrtc.org Remove unused methods from cricket::BaseChannel. 2019-01-14 benwright@webrtc.org Move VideoStreamReceiver JSON configuration parser to test source_set. 2019-01-14 titovartem@webrtc.org Introduce EncodedImageIdInjector. 2019-01-14 benwright@webrtc.org Add benwright@webrtc.org to test/fuzzers/OWNERS. 2019-01-14 mbonadei@webrtc.org Remove unneeded deps from api:call_api. 2019-01-14 saza@webrtc.org Add noise suppression settings to AudioProcessing::Config 2019-01-14 ilnik@webrtc.org Ensure correct decoding for unfiltered KSVC streams Created with: gclient setdep -r src/third_party/webrtc@ccc1b57e32bd The AutoRoll server is located here: https://autoroll.skia.org/r/webrtc-chromium-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux_chromium_archive_rel_ng;luci.chromium.try:mac_chromium_archive_rel_ng BUG= chromium:908819 ,chromium:912122,chromium:None,chromium:921933,chromium:921935,chromium:912122 TBR=webrtc-chromium-sheriffs-robots@google.com Change-Id: I7068ef84a375456cd908cf3041393a4fe562513e Reviewed-on: https://chromium-review.googlesource.com/c/1412801 Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#623039} [modify] https://crrev.com/256f7db493ba0c6a2e9be58980280efffb4dc778/DEPS |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by agrieve@google.com
, Nov 27