New issue
Advanced search Search tips

Issue 725877 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: ----



Sign in to add a comment

check_gn_headers failing on 2 builders

Project Member Reported by vitaliii@chromium.org, May 24 2017

Issue description

check_gn_headers failing on 2 builders

Builders failed on: 
- Linux Tests: 
  https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests
- Linux Tests (dbg)(1): 
  https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29%281%29



 
Labels: Pri-1
The error message is:
======================
Running ['/usr/bin/python', '/mnt/data/b/c/b/Linux_Tests__dbg__1_/src/build/check_gn_headers.py', '--out-dir', u'/b/c/b/Linux_Tests__dbg__1_/src/out/Debug', '--whitelist', '/mnt/data/b/c/b/Linux_Tests__dbg__1_/src/build/check_gn_headers_whitelist.txt', '--json', '/tmp/tmprzfr7z'] in '/mnt/data/b/c/b/Linux_Tests__dbg__1_/src' (env: None)
ninja: error: toolchain.ninja:63: loading 'obj/apps/apps.ninja': No such file or directory
subninja obj/apps/apps.ninja
                            ^ near here
Process Process-1:
Traceback (most recent call last):
  File "/usr/lib/python2.7/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/usr/lib/python2.7/multiprocessing/process.py", line 114, in run
    self._target(*self._args, **self._kwargs)
  File "/mnt/data/b/c/b/Linux_Tests__dbg__1_/src/build/check_gn_headers.py", line 38, in GetHeadersFromNinja
    q.put(ParseNinjaDepsOutput(ninja_out))
  File "/mnt/data/b/c/b/Linux_Tests__dbg__1_/src/build/check_gn_headers.py", line 48, in ParseNinjaDepsOutput
    for line in ninja_out:
  File "/mnt/data/b/c/b/Linux_Tests__dbg__1_/src/build/check_gn_headers.py", line 35, in NinjaSource
    raise subprocess.CalledProcessError(return_code, cmd)
CalledProcessError: Command '['ninja', '-C', '/b/c/b/Linux_Tests__dbg__1_/src/out/Debug', '-t', 'deps']' returned non-zero exit status 1
===============
Cc: wychen@chromium.org
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/56982 contains only 2 CL and one of them touches build/check_gn_headers.py (https://codereview.chromium.org/2628543003).
I am reverting the CL.
Project Member

Comment 5 by bugdroid1@chromium.org, May 24 2017

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

commit 5eba554ee38b7a6f78f54f1826fbd279f948c982
Author: vitaliii <vitaliii@chromium.org>
Date: Wed May 24 12:04:00 2017

Revert of Check missing headers in the build on CQ (patchset #8 id:160001 of https://codereview.chromium.org/2628543003/ )

Reason for revert:
This CL likely breaks |check_gn_headers| step. See  crbug.com/725877  for details.

Original issue's description:
> Check missing headers in the build on CQ
>
> This script is run on the following bots on CQ:
> - linux_chromium_dbg_ng
> - linux_chromium_rel_ng
>
> Note that this script can only be added to bots that have "all"
> in "additional_compile_targets".
>
> BUG=661774
>
> Review-Url: https://codereview.chromium.org/2628543003
> Cr-Commit-Position: refs/heads/master@{#474218}
> Committed: https://chromium.googlesource.com/chromium/src/+/ded5e27012a9b0d50de4b80a16e024fe5106b160

TBR=dpranke@chromium.org,thakis@chromium.org,wychen@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=661774, 725877 

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

[modify] https://crrev.com/5eba554ee38b7a6f78f54f1826fbd279f948c982/build/check_gn_headers.py
[delete] https://crrev.com/de1925cd7cf533069b328ce5b8e4ec3863650364/build/check_gn_headers_whitelist.txt
[modify] https://crrev.com/5eba554ee38b7a6f78f54f1826fbd279f948c982/testing/buildbot/chromium.linux.json
[delete] https://crrev.com/de1925cd7cf533069b328ce5b8e4ec3863650364/testing/scripts/check_gn_headers.py

Comment 7 by wychen@chromium.org, May 24 2017

Thanks for reverting the CL for me. The checking is supposed to be run only on builders, not testers. I'll study what went wrong.
Project Member

Comment 8 by bugdroid1@chromium.org, May 25 2017

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

commit 2f5a1ee159209aace78614d6012c361de5c3d2c3
Author: wychen <wychen@chromium.org>
Date: Thu May 25 05:45:30 2017

(reland) Check missing headers in the build on CQ

This script is run on the following bots on CQ:
- linux_chromium_dbg_ng
- linux_chromium_rel_ng

Note that this script can only be added to bots that have "all"
in "additional_compile_targets".

BUG=661774,  725877 

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

[modify] https://crrev.com/2f5a1ee159209aace78614d6012c361de5c3d2c3/build/check_gn_headers.py
[add] https://crrev.com/2f5a1ee159209aace78614d6012c361de5c3d2c3/build/check_gn_headers_whitelist.txt
[modify] https://crrev.com/2f5a1ee159209aace78614d6012c361de5c3d2c3/testing/buildbot/chromium.linux.json
[add] https://crrev.com/2f5a1ee159209aace78614d6012c361de5c3d2c3/testing/scripts/check_gn_headers.py

Comment 9 by guidou@chromium.org, May 25 2017

Status: Assigned (was: Fixed)
This is still failing on linux_chromium_rel_ng:

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng
Project Member

Comment 10 by bugdroid1@chromium.org, May 25 2017

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

commit f9662f54a78b4eaaa0126c9726d2b67328488bef
Author: wychen <wychen@chromium.org>
Date: Thu May 25 17:43:10 2017

Revert of (reland) Check missing headers in the build on CQ (patchset #3 id:40001 of https://codereview.chromium.org/2903733004/ )

Reason for revert:
Hit assertion in bots.  crbug.com/725877 

Original issue's description:
> (reland) Check missing headers in the build on CQ
>
> This script is run on the following bots on CQ:
> - linux_chromium_dbg_ng
> - linux_chromium_rel_ng
>
> Note that this script can only be added to bots that have "all"
> in "additional_compile_targets".
>
> BUG=661774,  725877 
>
> Review-Url: https://codereview.chromium.org/2903733004
> Cr-Commit-Position: refs/heads/master@{#474570}
> Committed: https://chromium.googlesource.com/chromium/src/+/2f5a1ee159209aace78614d6012c361de5c3d2c3

TBR=dpranke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=661774,  725877 

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

[modify] https://crrev.com/f9662f54a78b4eaaa0126c9726d2b67328488bef/build/check_gn_headers.py
[delete] https://crrev.com/a98ce9b5af5aaeb2e4998663610afa34f157b9e3/build/check_gn_headers_whitelist.txt
[modify] https://crrev.com/f9662f54a78b4eaaa0126c9726d2b67328488bef/testing/buildbot/chromium.linux.json
[delete] https://crrev.com/a98ce9b5af5aaeb2e4998663610afa34f157b9e3/testing/scripts/check_gn_headers.py

Cc: -wychen@chromium.org vitaliii@chromium.org
Owner: wychen@chromium.org
Status: Started (was: Assigned)
Thanks for reporting this, guidou!

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux%2Flinux_chromium_rel_ng%2F464556%2F%2B%2Frecipes%2Fsteps%2Fcheck_gn_headers__with_patch_%2F0%2Fstdout

Running ['/usr/bin/python', '/b/c/b/linux/src/build/check_gn_headers.py', '--out-dir', u'/b/c/b/linux/src/out/Release', '--whitelist', '/b/c/b/linux/src/build/check_gn_headers_whitelist.txt', '--json', '/tmp/tmp_l8XHP'] in '/b/c/b/linux/src' (env: None)
Traceback (most recent call last):
  File "/b/c/b/linux/src/build/check_gn_headers.py", line 192, in <module>
    sys.exit(main())
  File "/b/c/b/linux/src/build/check_gn_headers.py", line 150, in main
    'Found non-existing files in ninja deps'
AssertionError: Found non-existing files in ninja deps
Cc: dpranke@chromium.org
This 'Found non-existing files in ninja deps' assertion can be triggered if the build is dirty. This could happen if the analyze step says "No compile necessary".

If no compilation is done, we should skip the GN header checking. Is there a way to directly know whether the analyze step says "No compile necessary"? Otherwise doing a ninja dry run would do, but takes longer.
If the build is dirty because "No compile necessary", the result of GN checker cannot be trusted. In this case, two possibilities have been observed on the bots.

1) 'Found non-existing files in ninja deps' assertion is triggered. Have no JSON output.

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux%2Flinux_chromium_rel_ng%2F464451%2F%2B%2Frecipes%2Fsteps%2Fcheck_gn_headers__with_patch_%2F0%2Fstdout

2) The assertion is not triggered, and the JSON output contains wrong data.

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux%2Flinux_chromium_rel_ng%2F464453%2F%2B%2Frecipes%2Fsteps%2Fcheck_gn_headers__with_patch_%2F0%2Fstdout

Both errors can be avoided if we skip GN header checker on "No compile necessary".
Even if "build all" was run, it's still possible to trigger the assertion.
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/464487

This is surprising. I've never encountered this in my local build.

My speculation is that ninja doesn't always clear its database when the source .d files are gone. This is to say, for files that matters, the deps info is correct, but it's also possible to contain outdated entries that no longer matters for the build.
If analyze said no compile was necessary, I wouldn't expect us to run any tests, including checking the gn headers, so there may be a bug there.

I have no explanation for the situation in #c14, we probably need more data ...
I think it still makes sense to run some tests even if no compilation is needed.

For example, checkperms should still run. I'm not so sure about checkdeps. check_gn_headers shouldn't run.

In order to get more data, I'll revise check_gn_headers to just print things and return 0 the next time.
Possibly, though we should be careful to make sure that we can't make analyze report the right thing, first.
Analyze step doesn't always report the correct results due to underspecified header files in GN, which is exactly what we are working to eliminate. Therefore, "no compile necessary" can sometimes be fake, and in that case, we couldn't get useful information from check_gn_headers in that particular job.
Hm. Good point :).
Another issue. Some generated files are wrongly reported:
out/Release/gen/library_loaders/libgio.h
out/Release/gen/library_loaders/libspeechd.h
out/Release/gen/library_loaders/libudev0.h
out/Release/gen/library_loaders/libudev1.h

This is a recent regression. OUT_DIR is usually not in the #include filename, but in this case it's hardcoded by the generator.
Labels: -Sheriff-Chromium
-Sheriff-Chromium since this is being taken care of.
All the known issues have been addressed in https://codereview.chromium.org/2911543002#ps100001 and https://codereview.chromium.org/2904073003/#ps90001 except for one thing:

'Found non-existing files in ninja deps' can still happen after 'build all'. This could be a bug or feature request for ninja, but we'll need to get more data. With these two CLs above, when this happens, the script would print the offending files and fail the step. If I understand how trybots work correct, the result should be something like:
Build successful failed check_gn_header (with patch) failed check_gn_header (without patch)

I plan to land the enabling json config in the morning some day next week, so that I can revert it if anything goes wrong.
Cc: -vitaliii@chromium.org
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 3 2017

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

commit 0735fd76fd89d25935fd27d1d985ca0f3457a18a
Author: wychen <wychen@chromium.org>
Date: Sat Jun 03 07:53:26 2017

Make check_gn_headers.py more robust on the bots

Check whether the build is dirty first, and abort when dirty. This can
be skipped by using option --skip-dirty-check.

Dump JSON output on error, so that the result is still valid.

Filter out generated headers more aggressively.

Report non-existing headers in JSON as well.

BUG= 725877 

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

[modify] https://crrev.com/0735fd76fd89d25935fd27d1d985ca0f3457a18a/build/check_gn_headers.py
[modify] https://crrev.com/0735fd76fd89d25935fd27d1d985ca0f3457a18a/build/check_gn_headers_unittest.py

Project Member

Comment 25 by bugdroid1@chromium.org, Jun 5 2017

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

commit f7d9e91e64165b26cbfd95d29c33d4d6f0b46126
Author: wychen <wychen@chromium.org>
Date: Mon Jun 05 19:35:17 2017

Add scripts for running GN missing header checker on CQ

This relands part of crrev.com/2903733004, but not enabled.

The enabling config would be landed in a separate CL.

BUG=661774,  725877 

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

[add] https://crrev.com/f7d9e91e64165b26cbfd95d29c33d4d6f0b46126/build/check_gn_headers_whitelist.txt
[add] https://crrev.com/f7d9e91e64165b26cbfd95d29c33d4d6f0b46126/testing/scripts/check_gn_headers.py

Project Member

Comment 26 by bugdroid1@chromium.org, Jun 5 2017

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

commit 00f2b7ed28e3d1d864943497d6aa865ef1cde5e2
Author: wychen <wychen@chromium.org>
Date: Mon Jun 05 20:00:39 2017

Enable checking GN missing headers on CQ and waterfall

This relands the rest of crrev.com/2903733004.

Step "check_gn_headers" is enabled on the following bots on CQ:
- linux_chromium_dbg_ng
- linux_chromium_rel_ng

And also on the following builders on waterfall:
- Linux Builder
- Linux Builder (dbg)

Note that this script can only be added to bots that have "all"
in "additional_compile_targets".

BUG=661774,  725877 

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

[modify] https://crrev.com/00f2b7ed28e3d1d864943497d6aa865ef1cde5e2/testing/buildbot/chromium.linux.json

Components: Build
Besides wrongly skipped compilation by "analyze" step due to underspecified header files, there's another possibility that "compile" is run but the OUT_DIR is still dirty.

In this tryjob, only "components/translate/core/browser/ranker_model_loader_unittest.cc" is touched, and even though linux_chromium_rel_ng has "additional_compile_targets": "all", the output still doesn't have "all". I guess this is an optimization for test-only changes.

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/471860

{
  "compile_targets": [
    "components:components_unittests"
  ],
  "status": "Found dependency",
  "test_targets": [
    "components_unittests"
  ]
}

This slightly increases the probability a culprit CL can pass CQ, but should be fine.
The culprit CLs slipped in due to inaccurate "analyze" results would eventually be caught in the waterfall, since there's no "analyze" step in the waterfall bots. If this happens, these CLs would be in the good hands of Sheriffs.

The bots in CQ and waterfall look fine this time, but I haven't seen "Found non-existing files in ninja deps" error yet. Probably this only happens in the test-only changes so that "build all" didn't actually happen.
Status: Fixed (was: Started)
Looks stable enough. Closing it.
Project Member

Comment 30 by bugdroid1@chromium.org, Jun 16 2017

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

commit d8b1ec3835eee2daddcae4c292f9a87683c20001
Author: wychen <wychen@chromium.org>
Date: Fri Jun 16 23:33:16 2017

Revert of Enable checking GN missing headers on CQ and waterfall (patchset #3 id:40001 of https://codereview.chromium.org/2910603002/ )

Reason for revert:
The false negative rate of step check_gn_headers on CQ is higher than expected. Disable it to ease waterfall breakage.

BUG=733500

Original issue's description:
> Enable checking GN missing headers on CQ and waterfall
>
> This relands the rest of crrev.com/2903733004.
>
> Step "check_gn_headers" is enabled on the following bots on CQ:
> - linux_chromium_dbg_ng
> - linux_chromium_rel_ng
>
> And also on the following builders on waterfall:
> - Linux Builder
> - Linux Builder (dbg)
>
> Note that this script can only be added to bots that have "all"
> in "additional_compile_targets".
>
> BUG=661774,  725877 
>
> Review-Url: https://codereview.chromium.org/2910603002
> Cr-Commit-Position: refs/heads/master@{#477058}
> Committed: https://chromium.googlesource.com/chromium/src/+/00f2b7ed28e3d1d864943497d6aa865ef1cde5e2

TBR=dpranke@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=661774,  725877 

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

[modify] https://crrev.com/d8b1ec3835eee2daddcae4c292f9a87683c20001/testing/buildbot/chromium.linux.json

Sign in to add a comment