This is probably because of the way the Android templates are set up. They list the .d files as outputs of actions and use them as stamp files for dependency tracking.

This isn't really the way that Ninja expects them to work. It expects .d files to be thrown off incidentally, and it expects different files for dependency tracking.

It looks like the .d file lists a dependency from a previous build (this is expected and is kind of the point). Then the build changed such that when this old .d file is loaded, it generates a circular dependency.

Anybody can take a look of this issue? my working is blocking on it,

I think this might be a decent amount of work to fix, and I think most of the folks on the cc' line are OoO for the next couple days or thereabouts, unfortunately.

If this was really urgent that you be able to land your CL, you might be able to work around the issue with a landmine, not sure:

https://www.chromium.org/Home/chromium-clobber-landmines

but if this isn't urgent, we should try to come up with a proper fix, as landmines are painful and should be avoided.

(though an android-only landmine wouldn't be that bad).

Yeah, afaik this is a fundamental issue with how .d files work. I think a landmine is the answer.

What do we need to do to make .d files not be listed as inputs? That seems bad. Or, at least, not being able to rename things in this way seems bad.

I don't think they are listed as inputs. The problem is just that:

First build: .ninja contains coarse dependencies. More specific dependency information written to .d files.
Second build with changed .ninja: Ninja merges coarse dependencies from .ninja files with dependencies listed in .d files.

The problem is that the .d files are always one build out-of-date. This mostly works, but does occasionally hit corner cases like this.

I don't think there's a true "fix" for it without all dependency information being known at gn gen time. The .d files are an imperfect (but usually works) way to get incremental builds to work properly. 

The error from the log file is:

ninja: error: dependency cycle: obj/components/signin/core/browser/android/java__build_config.stamp -> gen/components/signin/core/browser/android/java__build_config.d -> gen/components/sync/android/sync_java.build_config -> obj/components/sync/android/sync_java__build_config.inputdeps.stamp -> obj/components/signin/core/browser/android/java__build_config.stamp

I think the only way that happens is if a *.d file is explicitly listed as a dependency for a target, and not if things are implicitly listed in deps_file's.

So, are you sure the .d isn't listed as an input?

It's true that .d files are always one build out of date, but because of the way they work that doesn't normally cause problems (files can be renamed and missing files are ignored, for example). Whatever is going on here is different.

Here's the ninja entry for it:

build gen/components/signin/core/browser/android/java__build_config.d gen/components/signin/core/browser/android/java.build_config: __components_signin_core_browser_android_java__build_config___build_toolchain_android_clang_arm__rule | obj/components/signin/core/browser/android/java__build_config.inputdeps.stamp
  depfile = gen/components/signin/core/browser/android/java__build_config.d

build obj/components/signin/core/browser/android/java__build_config.stamp: stamp gen/components/signin/core/browser/android/java__build_config.d gen/components/signin/core/browser/android/java.build_config


The GN for java__build_config resolves to an action() that declares a depfile, but has no inputs or sources.

The contents of gen/components/signin/core/browser/android/java__build_config.d contains gen/components/sync/android/sync_java.build_config, so I think that really is where the dependency is coming from.

The write_build_config() template is declaring the depsfile as an output (and that's the problem). It shouldn't do that, I think.

Ah, interesting! Never occurred to me that it shouldn't be listed under outputs (since, you know, it's an output). Does actually make some sense though.

I wonder if maybe we should just change GN to not allow depfiles to be listed as outputs (or ignore them if listed)?

After some discussion, it seems there is actually a reason to want to specify a depfile as an output (e.g. when using it as a stamp file for copy_ex). Removing all current places in Android where a depfile is unnecessarily listed as an output should be enough to stop this from happening again (since it was only happening in the first place from copy & pasting existing usages).

CL: https://codereview.chromium.org/2305283002

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/9758ae8c55783356a041a21ad31cb12a5d634907

commit 9758ae8c55783356a041a21ad31cb12a5d634907
Author: Andrew Grieve <agrieve@chromium.org>
Date: Mon Sep 05 00:51:20 2016

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

commit fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64
Author: agrieve <agrieve@chromium.org>
Date: Tue Sep 06 20:37:46 2016

GN(Android): Stop specifying depfile under outputs

Doing so is unnecessary (GN already knows that depfiles are generated by
their target), and causes the unwanted side-effect of changes to .d
files triggering rebuilds of dependent targets.

BUG= 639042 

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

[modify] https://crrev.com/fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64/android_webview/BUILD.gn
[modify] https://crrev.com/fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64/build/config/android/internal_rules.gni
[modify] https://crrev.com/fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64/build/config/android/rules.gni
[modify] https://crrev.com/fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64/chrome/android/BUILD.gn

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

commit fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64
Author: agrieve <agrieve@chromium.org>
Date: Tue Sep 06 20:37:46 2016

GN(Android): Stop specifying depfile under outputs

Doing so is unnecessary (GN already knows that depfiles are generated by
their target), and causes the unwanted side-effect of changes to .d
files triggering rebuilds of dependent targets.

BUG= 639042 

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

[modify] https://crrev.com/fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64/android_webview/BUILD.gn
[modify] https://crrev.com/fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64/build/config/android/internal_rules.gni
[modify] https://crrev.com/fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64/build/config/android/rules.gni
[modify] https://crrev.com/fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64/chrome/android/BUILD.gn

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

commit 6a8a144fdd34dcb08a14765c743d9b2abb113898
Author: lushnikov <lushnikov@chromium.org>
Date: Tue Sep 06 22:00:23 2016

Revert of GN(Android): Stop specifying depfile under outputs (patchset #2 id:20001 of https://codereview.chromium.org/2305283002/ )

Reason for revert:
Reverting due to builder compile failure:
https://build.chromium.org/p/chromium/builders/Android/builds/61823

Original issue's description:
> GN(Android): Stop specifying depfile under outputs
>
> Doing so is unnecessary (GN already knows that depfiles are generated by
> their target), and causes the unwanted side-effect of changes to .d
> files triggering rebuilds of dependent targets.
>
> BUG= 639042 
>
> Committed: https://crrev.com/fd47b4e4232b99dfb4044b79eea66a7e6b2c2a64
> Cr-Commit-Position: refs/heads/master@{#416710}

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

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

[modify] https://crrev.com/6a8a144fdd34dcb08a14765c743d9b2abb113898/android_webview/BUILD.gn
[modify] https://crrev.com/6a8a144fdd34dcb08a14765c743d9b2abb113898/build/config/android/internal_rules.gni
[modify] https://crrev.com/6a8a144fdd34dcb08a14765c743d9b2abb113898/build/config/android/rules.gni
[modify] https://crrev.com/6a8a144fdd34dcb08a14765c743d9b2abb113898/chrome/android/BUILD.gn

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

commit daef66ba0e2326d524e6429afdbb282b750c0ecc
Author: agrieve <agrieve@chromium.org>
Date: Wed Sep 07 14:10:02 2016

Reland of GN(Android): Stop specifying depfile under outputs

Previous attempt:
https://codereview.chromium.org/2318673003/ )

Reason for reland:
Now creates depfile parent directory

Doing so is unnecessary (GN already knows that depfiles are generated by
their target), and causes the unwanted side-effect of changes to .d
files triggering rebuilds of dependent targets.

TBR=michaelbai@chromium.org,dpranke@chromium.org,lushnikov@chromium.org
BUG= 639042 

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

[modify] https://crrev.com/daef66ba0e2326d524e6429afdbb282b750c0ecc/android_webview/BUILD.gn
[modify] https://crrev.com/daef66ba0e2326d524e6429afdbb282b750c0ecc/build/android/gyp/util/build_utils.py
[modify] https://crrev.com/daef66ba0e2326d524e6429afdbb282b750c0ecc/build/config/android/internal_rules.gni
[modify] https://crrev.com/daef66ba0e2326d524e6429afdbb282b750c0ecc/build/config/android/rules.gni
[modify] https://crrev.com/daef66ba0e2326d524e6429afdbb282b750c0ecc/chrome/android/BUILD.gn

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/ddea385a42e74a533142a76fb8a75fd203f549ec

commit ddea385a42e74a533142a76fb8a75fd203f549ec
Author: Andrew Grieve <agrieve@chromium.org>
Date: Wed Sep 07 14:38:40 2016

Not actually sure if the original problem description is fixed, but we at least have made a change here to stop having depfiles in outputs.

Unfortunately, this broke 'No work to do' state. If you run, for example, 'ninja -C out/Debug base:base_java' twice in a row, the second run will still do something. 
Running 'ninja -C out/Debug base:base_java -d explain' shows this:
ninja explain: expected depfile 'gen/base/base_java__build_config.d' to mention 'gen/base/base_java.build_config', got 'gen/base/base_java__build_config.d'

And a lot of other output (similar depfile errors and other non-relevant errors).

It seems that we should specify actual output files when writing depfiles via build_utils.WriteDepfile instead of depfile itself to satisfy ninja.

Aha, thanks for figuring this out. I'm guess it's what's causing https://bugs.chromium.org/p/chromium/issues/detail?id=646165

This problem looks still exist, I've rebased my CL today (https://codereview.chromium.org/2351703003/) which works well locally (I do need to rm old out/Debug to regenerate it).

One of the failed try bot: https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/143940

Sigh, I think this means the depfile refactoring to not mention itself was a bit pointless, and that my explanation in #8 actually was accurate. The change is inverting a dependency, but the .d files always being one build out-of-date cause this inversion to turn into a cycle.

I don't think it's fixable without a landmine, or by renaming one of the targets involved.

I still don't understand this. You should be able to rename things somewhat arbitrarily and have them keep working, and we shouldn't be getting cycles in our dependencies that survive the build files being regenerated.


The error from the log in #24 is:

ninja: error: dependency cycle: 

obj/components/signin/core/browser/android/java.stamp -> 
obj/components/signin/core/browser/android/java__analysis.stamp -> 
obj/components/signin/core/browser/android/java__lint.stamp -> 
gen/components/signin/core/browser/android/java__lint/result.xml -> 
lib.java/components/sync/android/sync_java.interface.jar -> 
obj/components/sync/android/sync_java__compile_java__ijar.inputdeps.stamp -> 
lib.java/components/sync/android/sync_java.jar -> 
obj/components/sync/android/sync_java__compile_java__process_prebuilt__filter.inputdeps.stamp -> 
gen/components/sync/android/sync_java__compile_java.javac.jar -> 
obj/components/sync/android/sync_java__compile_java__javac.inputdeps.stamp -> 
obj/components/signin/core/browser/android/java.stamp

This is a different cycle than the one we had before (no .d file this time). Which dependency in that
cycle is wrong?

Is this like  issue 408192  (and the github issue linked from there)?

dpranke@: signin should not depend on sync, so gen/components/signin/core/browser/android/java__lint/result.xml -> 
lib.java/components/sync/android/sync_java.interface.jar is wrong.

The only suspicious thing I see after gogerald@'s change is a dep in unittests on syncable_prefs, which depends on sync:

https://cs.chromium.org/chromium/src/components/signin/core/browser/BUILD.gn?l=164

The dependency cycle output doesn't mention syncable_prefs or test stuff though, so perhaps that's unrelated?

Comment #28: I believe we have no DEPs cycle until now. New dependency from components/sync/android/* to components/signin/core/browser/android/* is in java layer, however dependency of components/syncable_prefs is in C++ layer.

BTW: As I mentioned in comment #23, I need rm out/Debug to regenerate gn files to get rid of dependency cycle error locally after reverse the dependency. That might be interested to look.

Just starting today I'm failing to build TOT #421258 due to the dep cycle in #26. Is the workaround to kill my build directory?

"gn clean" or remove "out/*" to regenerate gn files. You only need to do it one time if you updated from old code to ToT.

or just deleting "out/Debug/gen/components/signin/" as mentioned in https://codereview.chromium.org/2351703003/

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I think we fixed this and it's safe to close.