New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 800761 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 794926



Sign in to add a comment

gn check issues: //skia/*

Project Member Reported by msimoni...@opera.com, Jan 10 2018

Issue description

With 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.
 
Description: Show this description
Cc: mtklein@chromium.org
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?
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.
Blockedon: 794926
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.
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.
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.
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Status: Fixed (was: Untriaged)
please reopen if that didn't fix this.

Sign in to add a comment