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

Issue 611898 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug
Team-Accessibility

Blocked on:
issue 612382



Sign in to add a comment

ax_enums.h build flake

Project Member Reported by est...@chromium.org, May 13 2016

Issue description

seems that some mixture of hard_dependency, dependencies, export_dependent_settings, etc. is missing:

[4958/33267] CXX obj\ui\app_list\presenter\test\app_list_presenter_test_support.app_list_presenter_impl_test_api.obj
FAILED: obj/ui/app_list/presenter/test/app_list_presenter_test_support.app_list_presenter_impl_test_api.obj 
ninja -t msvc -e environment.x86 -- C:\b\build\slave\cache\cipd\goma/gomacc "C:\b\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\ui\app_list\presenter\test\app_list_presenter_test_support.app_list_presenter_impl_test_api.obj.rsp /c ..\..\ui\app_list\presenter\test\app_list_presenter_impl_test_api.cc /Foobj\ui\app_list\presenter\test\app_list_presenter_test_support.app_list_presenter_impl_test_api.obj /Fdobj\ui\app_list\presenter\app_list_presenter_test_support.cc.pdb 
c:\b\build\slave\win\build\src\ui\views\view.h(22): fatal error C1083: Cannot open include file: 'ui/accessibility/ax_enums.h': No such file or directory

https://build.chromium.org/p/chromium/builders/Win/builds/43375
 
Cc: -michae...@chromium.org
i merely fixed a comma issue here, ¯\_(ツ)_/¯

Comment 2 by tapted@chromium.org, May 16 2016

I think the problem here is that both export_dependent_settings and hard_dependency is required in all targets that have a public header including ax_enums.h

Since views/view.h #inclues ax_enum.h, to fix this "nicely", views.gyp:views would need to be a hard_dependency for everything that depends on it. That's perhaps overkill (e.g. it might reduce build parallelism).

The "easy" fix: just have app_list_presenter.gyp depend on ax_gen

The *best* fix, I think is to have ax_enums.idl generate `enum class` rather than merely `enum`. Then view.h can forward-declare ui::AXEvent and not have to include ax_enums.h at all.

Comment 3 by tapted@chromium.org, May 17 2016

Cc: aboxhall@chromium.org dtseng@chromium.org dmazz...@chromium.org
 Issue 510339  has been merged into this issue.

Comment 4 by tapted@chromium.org, May 17 2016

Labels: OS-All
Owner: tapted@chromium.org
Status: Started (was: Assigned)
This also came up in  Issue 536641 

The ideal fix (i.e. forward-declare an enum class) is in  Issue 612382 , but it needs time to cook.

Compiler flakes are pretty annoying, so I've made https://codereview.chromium.org/1984153002 to hopefully make this current collection of flakes go away.
Project Member

Comment 5 by bugdroid1@chromium.org, May 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93fa270c60338957b15cefb20e5553fb67cb2ba3

commit 93fa270c60338957b15cefb20e5553fb67cb2ba3
Author: tapted <tapted@chromium.org>
Date: Tue May 17 07:25:32 2016

Workaround for flaky ax_enums.h errors when compiling under app_list_presenter

ui/views/view.h #includes ax_enums.h and app_list_presenter #includes
view.h. This means ax_enums.h needs to exist before anything in
app_list_presenter is compiled, even though app_list_presenter doesn't
use anything from ax_enums.h.

An alternative might be to make views.gyp:views a hard_dependency of
everything that depends on it. However, it seems unfair to force that,
since it's really only ax_gen that is the "hard" dependency, and it's
already correctly marked as a hard_dependency.

Exploring a nicer fix in  http://crbug.com/612382 , which involves view.h
simply not #including ax_enums.h

BUG= 611898 

Review-Url: https://codereview.chromium.org/1984153002
Cr-Commit-Position: refs/heads/master@{#394074}

[modify] https://crrev.com/93fa270c60338957b15cefb20e5553fb67cb2ba3/ui/app_list/presenter/BUILD.gn
[modify] https://crrev.com/93fa270c60338957b15cefb20e5553fb67cb2ba3/ui/app_list/presenter/app_list_presenter.gyp

Comment 6 by tapted@chromium.org, May 18 2016

Labels: -Pri-1 Pri-2
cycle times are slow. But no new flakes since 

https://build.chromium.org/p/chromium/builders/Win/builds/43456

which was at r394023; before #c5 landed.


The latest flake seems to be another mojo one in https://build.chromium.org/p/chromium/builders/Win/builds/43482

FAILED: obj/chrome/browser/media/router/mojo/media_router_test_support.media_router_mojo_test.obj 
ninja -t msvc -e environment.x86 -- C:\b\build\slave\cache\cipd\goma/gomacc "C:\b\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\chrome\browser\media\router\mojo\media_router_test_support.media_router_mojo_test.obj.rsp /c ..\..\chrome\browser\media\router\mojo\media_router_mojo_test.cc /Foobj\chrome\browser\media\router\mojo\media_router_test_support.media_router_mojo_test.obj /Fdobj\chrome\browser\media\router\media_router_test_support.cc.pdb 
c:\b\build\slave\win\build\src\services\shell\public\cpp\connect.h(10): fatalerror C1083: Cannot open include file: 'services/shell/public/interfaces/interface_provider.mojom.h': No such file or directory

moving to p2 and leaving open until I address the TODOs in #c5

Comment 8 by tapted@chromium.org, May 20 2016

Blockedon: 612382
The flake in #c7 is hopefully addressed by https://codereview.chromium.org/1997853002/ (landed in r394888 -> https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/19742). Things are green since then.

There's progress in  Issue 612382  as well. And http://crrev.com/1988613002 should resolve/isolate/prevent the particular flakes that ocurred in the initial report and in #c7.

Comment 9 by tapted@chromium.org, May 26 2016

Ugh. The app_list_presenter flake happened again at r395937 in https://build.chromium.org/p/chromium/builders/Win%20x64/builds/1001/ this time due to test targets. Workaroundy fix: https://codereview.chromium.org/2012773003/

There's also news in  Issue 612382  -- exploring a way to fix this "faster" by enforcing enums to be int-sized rather than enum classes. This could get the resolution out with a lot less churn. See https://codereview.chromium.org/1999633002/ for how that could look.

But it's also worth noting https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookbook.md#hard_dependency

It says
  GYP code sometimes sets

    'hard_dependency': 1,
  to indicate that the current target must be build before its dependents. GN can deduce this internally, so you can ignore this directive.


So maybe the GN migration will also resolve the flakes (hooray!). But there may still be a lesser concern about complicated chains of hard dependencies creating bottlenecks in the build.


There was also a surprising number of things wanting an ex_enums.h #include for IWYU -- https://codereview.chromium.org/1996893002/ 

Also one header where the way to forward declare wasn't clear - https://codereview.chromium.org/1999633002/diff/100001/content/public/common/common_param_traits_macros.h . It raises the possibility that addressing these flakes purely by forward-declaring may be a dead end :(
For the last bit, you could have just manually defined FOO_LAST values in a regular header, and then static_assert that these are the same as the autogenerated _LAST fields in some cc file.
Probably not surprising, but I like https://codereview.chromium.org/1999633002/ :-)
GYP is getting more rare so I'm not personally concerned about having too many hard deps in GYP these days.
Project Member

Comment 13 by bugdroid1@chromium.org, May 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1bce9d0653131ac106bd31550b65e6672ecc1b8

commit f1bce9d0653131ac106bd31550b65e6672ecc1b8
Author: tapted <tapted@chromium.org>
Date: Thu May 26 08:45:11 2016

Add ax_gen dependency to app_list_presenter test targets

These targets all have files that #include view.h, which #includes
ax_enums.h. Therefore, building these targets can not begin until
ax_enums.h has been generated. Otherwise the compile can fail.

The dependency was added to the non-test app_list_presenter target in
r394074. That helped, but it didn't do enough.

Example: https://build.chromium.org/p/chromium/builders/Win%20x64/builds/1001/

BUG= 611898 

Review-Url: https://codereview.chromium.org/2012773003
Cr-Commit-Position: refs/heads/master@{#396150}

[modify] https://crrev.com/f1bce9d0653131ac106bd31550b65e6672ecc1b8/ui/app_list/presenter/BUILD.gn
[modify] https://crrev.com/f1bce9d0653131ac106bd31550b65e6672ecc1b8/ui/app_list/presenter/app_list_presenter.gyp

Status: Fixed (was: Started)
I think GN has resolved this for the most part. There's still  Issue 612382 , which I think still makes sense, but there were some use cases that meant it could never fully avoid targets requiring a transitive dependency on ax_enums.h. (So it's not useful for this Issue).

Sign in to add a comment