gn check issues: //skia/* |
||||
Issue descriptionWith the GN check improvements introduced in crbug.com/794926 new issues are being reported for the //skia/* targets (it was previously clean). The issues are related to dependencies for the skia_opts_* targets.
,
Jan 10 2018
I'm not quite sure I understand. I don't see any changes referenced in crbug.com/794926 , and I just built gn from a Chromium checkout at 353219b1d808a8980af93eff5412ec2863557e35 and ran $ out/release/gn check out/release/ //skia/* > Header dependency check OK is this specific to a particular configuration or is there some CL which needs to be applied to see this issue? Do you have any further details of the errors you're seeing?
,
Jan 11 2018
The issues will show up once the CL for 794926 is integrated - hopefully this week. Sorry about the confusion, I wasn't sure whether I should write the task description in future or in present tense.
,
Jan 11 2018
,
Jan 11 2018
To reproduce * apply https://chromium-review.googlesource.com/c/chromium/src/+/827014 * build gn: ninja -C out/release/ gn * run check: out/release/gn check --force out/release/ //skia/* This produces the output ERROR at //third_party/skia/src/opts/SkOpts_sse41.cpp:8:11: Include not allowed. #include "SkOpts.h" ^------- It is not in any dependency of //skia:skia_opts_sse41 The include file is in the target(s): //skia:skia which should somehow be reachable. ___________________ ERROR at //third_party/skia/src/opts/SkOpts_avx.cpp:9:11: Include not allowed. #include "SkOpts.h" ^------- It is not in any dependency of //skia:skia_opts_avx The include file is in the target(s): //skia:skia which should somehow be reachable. ___________________ ERROR at //third_party/skia/src/opts/SkOpts_sse42.cpp:8:11: Include not allowed. #include "SkOpts.h" ^------- It is not in any dependency of //skia:skia_opts_sse42 The include file is in the target(s): //skia:skia which should somehow be reachable. ___________________ ERROR at //third_party/skia/src/opts/SkOpts_ssse3.cpp:8:11: Include not allowed. #include "SkOpts.h" ^------- It is not in any dependency of //skia:skia_opts_sse3 The include file is in the target(s): //skia:skia which should somehow be reachable. ___________________ ERROR at //third_party/skia/src/opts/SkBitmapProcState_opts_SSSE3.cpp:10:11: Include not allowed. #include "SkPaint.h" ^-------- It is not in any dependency of //skia:skia_opts_sse3 The include file is in the target(s): //skia:skia which should somehow be reachable. The issue appears to be that SkOpts.h is now found on the include path (because skia_opts_* configs pull it in), but since skia_opts_* targets don't depend on anything and don't list SkOpts.h as a source it's not considered a valid include.
,
Jan 11 2018
The only reason skia_opts_* are in separate source_sets is to be compiled with different flags. They are conceptually the all part of the skia target. It appears that allow_circular_includes_from = [ ":skia_opts" ] is already being used to indicate this, but it appears this now needs to be extended to all of the other skia_opts_* targets.
,
Jan 11 2018
This is a fairly horrible thing to try, but it appears that allow_circular_includes_from is not transitive. In this case we have something like
component("skia") {
sources = [ "//third_party/skia/src/core/SkOpts.h" ]
allow_circular_includes_from = [ ":skia_opts" ]
deps = [ ":skia_opts" ]
}
source_set("skia_opts") {
deps = [ ":skia_opts_avx" ]
}
source_set("skia_opts_avx") {
source = [ "<file which includes SkOpts.h>" ]
}
and thought perhaps it would be clever to try to add
allow_circular_includes_from = [ ":skia_opts_avx" ]
to skia_opts. It appears a target may use allow_circular_includes_from to say "that is part of me", but that does not necessarily make those parts part of any parent who did the same to the current target.
As a result we will probably need something somewhat more invasive.
,
Jan 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0a7ce7463ceae66f14cbacabd236fea7f56b5bf1 commit 0a7ce7463ceae66f14cbacabd236fea7f56b5bf1 Author: Mike Klein <mtklein@chromium.org> Date: Thu Jan 11 20:54:12 2018 opt Skia out of header includes checks Since we're upstream of Chromium, it's a pain to maintain skia/BUILD.gn, and we really don't care much about include checks. These opts targets are really just part of :skia. Bug: 800761 Change-Id: If4561a2145311f803641918899a346ab68c898be Reviewed-on: https://chromium-review.googlesource.com/861847 Reviewed-by: Ben Wagner <bungeman@chromium.org> Commit-Queue: Mike Klein <mtklein@chromium.org> Cr-Commit-Position: refs/heads/master@{#528748} [modify] https://crrev.com/0a7ce7463ceae66f14cbacabd236fea7f56b5bf1/skia/BUILD.gn
,
Jan 11 2018
please reopen if that didn't fix this. |
||||
►
Sign in to add a comment |
||||
Comment 1 by msimoni...@opera.com
, Jan 10 2018