sleevi@sleevi3:~/development/chromium/src$ cat out/Debug/build.ninja
ninja_required_version = 1.7.2

rule gn
  command = ../../buildtools/linux64/gn --root=../.. -q gen .
  description = Regenerating ninja files

out/Debug/build.ninja has a newer timestamp than BUILD.gn. BUILD.gn got updated when you synced. build.ninja's timestamp should only be updated by `gn gen`, but that didn't run since your dir is so empty. What updated build.ninja's timestamp? Can you re-repro under strace to find out?

sleevi@sleevi3:~/development/chromium/src$ cat out/Debug/build.ninja.d
build.ninja: nonexistant_file.gn

Aha, `gn clean` updates the timestamp of `build.ninja`, which causes things to not work. Maybe we can find out why that happens; that seems incorrect to me...

Aha, it's because gn clobbers the old build.ninja with one that's almost completely empty. That seems both unnecessary, and wrong (due to the timestamp change).

Interestingly, my out/Release/build.ninja contains substantially different content than my other out/*/build.ninja files (all of which are the same).

Namely:
 cat Release/build.ninja
rule gn
  command = [path-to-home]/development/chromium/src/buildtools/linux64/gn --root=[path-to-home]/development/chromium/src -q gen .
  description = Regenerating ninja files

build build.ninja: gn
  generator = 1
  depfile = build.ninja.d


That is, both an absolute path and an explicit build step for build.ninja

I did just that and passed "-d explain" to ninja. The output was less than helpful:
$ ninja -d explain -C out/Debug-iphonesimulator -n
ninja: Entering directory `out/Debug-iphonesimulator'
ninja: no work to do.

Hum. This looks like there is no rule defined in the build.ninja file. Let's have a look:
$ cat out/Debug-iphonesimulator/build.ninja
ninja_required_version = 1.7.2

rule gn
  command = ../../buildtools/mac/gn --root=../.. -q gen .
  description = Regenerating ninja file

So, no rule. Let's compare this to the build.ninja file that is generated by gn gen:

$ head out/Debug-iphonesimulator/build.ninja
ninja_required_version = 1.7.2

rule gn
  command = ../../buildtools/mac/gn --root=../.. -q gen .
  description = Regenerating ninja files

build build.ninja: gn
  generator = 1
  depfile = build.ninja.d


If clobber uses "gn clean", then the build.ninja is created from the old one by removing everything after the third blank line (look at ExtractGNBuildCommands in tools/gn/command_clean.cc). The script build/landmines.py uses however build/clobber.py that drop everything after the second blank line (extract_gn_build_commands in build/clobber.py). So it looks like the clobber.py script is broken and should either be rewritten to use "gn clean" or to use the same logic.

https://chromium-review.googlesource.com/c/chromium/src/+/1001902 is a fix for build/clobber.py script.

`gn clean` was added in https://codereview.chromium.org/909103003 ; it's been writing the default build.ninja since the start.

Aha, it just clones what the landmines code now at https://cs.chromium.org/chromium/src/build/clobber.py?q=clobber.py&sq=package:chromium&dr&l=92 does, so we have the bug in two places.

The python version is from https://chromium.googlesource.com/chromium/src/+/f5b2cda7e05f7ffa4ac57b6a00d9cd52c1e0ac87%5E%21/#F0

So I think this has been broken for a long time. I think the Right Fix is to not touch build.ninja at all since then we rerun gen due to the timestamp.

Otherwise, the default build.ninja should add a non-existent input for the gen step so that we _always_ call `gn gen` after a landmine...oh, someone else already had this idea, there's a dep on nonexistant_file.gn both in `gn clean` (why it works in my test there) and in clobber.py. That's even mentioned in comment 3! So why didn't that work?

Oh, the build.ninja in comment 1 looks like it's missing the actual `build build.ninja:` line. Since it has the required_version thingy at the top it's not the fallback file from clobber.py:

    else:
      # Couldn't parse the build.ninja file, write a default thing.
      f.write('''rule gn
command = gn -q gen //out/%s/
description = Regenerating ninja files

build build.ninja: gn
generator = 1
depfile = build.ninja.d
''' % (os.path.split(build_dir)[1]))


Aha!

def extract_gn_build_commands(build_ninja_file):
  """Extracts from a build.ninja the commands to run GN.

  The commands to run GN are the gn rule and build.ninja build step at the
  top of the build.ninja file. We want to keep these when deleting GN builds
  since we want to preserve the command-line flags to GN.

  On error, returns the empty string."""
  result = ""
  with open(build_ninja_file, 'r') as f:
    # Read until the second blank line. The first thing GN writes to the file
    # is the "rule gn" and the second is the section for "build build.ninja",
    # separated by blank lines.
    num_blank_lines = 0
    while num_blank_lines < 2:
      line = f.readline()
      if len(line) == 0:
        return ''  # Unexpected EOF.
      result += line
      if line[0] == '\n':
        num_blank_lines = num_blank_lines + 1
  return result


This is not true, we need 3 blank lines, here's `gn gen` output:

"""
ninja_required_version = 1.7.2

rule gn
  command = ../../buildtools/mac/gn --root=../.. -q gen .
  description = Regenerating ninja files

build build.ninja: gn
  generator = 1
  depfile = build.ninja.d
"""


So should be easy to fix clobber.py to get this right.

Dirk fixed this for gn in https://chromium.googlesource.com/chromium/src/+/7a152fc8b6727dfbf1616043f314d99079605e45 but nobody thought of fixing clobber.py.

Short-term fix: s/2/3/ in clobber.py. I'll make a CL for that. Better fix: Make clobber.py call `gn clean`.

Looks like sdefresne was a bit faster :-)

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

commit dfbbade3b1b603083944c0fe1945ff3842526a1d
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Mon Apr 09 15:32:48 2018

Fix src/build/clobber.py

The build.ninja file generated by ninja has changed and now
include a "ninja_required_version" statement. To keep the
rule to generate the build.ninja file, all the line up to
the third blank line should be kept (this is what "gn clean"
does).

Bug:  830627 
Change-Id: I493cc39f484c6da8a0784d9cf2b45337cb4672d3
Reviewed-on: https://chromium-review.googlesource.com/1001902
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549181}
[modify] https://crrev.com/dfbbade3b1b603083944c0fe1945ff3842526a1d/build/clobber.py

Regarding comment #9, I agree that clobber.py should just invoke "gn clean" (to avoid to duplicate the logic). Do you want me to do this or have you already started?

I *think* there was a reason we didn't use gn clean, but I'm not sure what it was at the moment, am trying to remember. It's possible the code was written when we couldn't assume GN (might still have GYP). 

re: not touching build.ninja at all ... Is it safe to assume that even if build.ninja references subninjas that no longer exist, that won't break things (i.e., ninja will still reinvoke gn). It's possible that that was the concern and why we do clobber the file, but maybe the concern was misplaced.

The support for gn build dir was added in https://codereview.chromium.org/709813003. There is no explanation why it is not just using "gn clean". The code was not changed significantly since then (just extracted and fixed to avoid flakyness on Windows AFAICT).

non-existent subninjas will stop ninja from loading the manifest, so that would be a problem -- but why would the old ninja files have disappeared? But the current "dep on non-existent file" thing is probably safer, we can just keep that now that it actually works.

sdefresne: I haven't started, feel free :-)

I guess I know why build/clobber.py (originally build/landmines.py) does not invoke "gn clean". The clobber steps is the first hook and happens before fetching the "gn" binary from google storage.

So this mean that we could determine we need to run a clobber but cannot as "gn" binary is not available. So, there are some options:
1. keep current code with both clean implementation (in gn and build/clobber.py)
2. change build/clobber.py to invoke gn if available, or to just delete the directory otherwise
3. split landmine logic in two hooks, one determining whether the clobber is necessary, another performing the clobber and run the second part after fetching gn
4. move landmine hook after the hook fetching gn binary from google_storage
5. ?

Ah right. Annoying. 3 sounds cleanest, but by now this is complex enough that I'd probably just say mark this fixed and move on :-P

> why would the old ninja files have disappeared?

Because `gn clean` tries to delete as much as we can to ensure that nothing is stale (which, IIRC, `ninja -t clean` won't do and so we can't use it directly).

It used to actually `rm -fr $root_build_dir`, but that caused problems by invalidating the directory. I'm not sure if there's a better alternative than what we do now.

I would be *very* reluctant to do (3) and split landmines into two phases. That would significantly complicate how we reason about hooks, which are currently all independent tasks that are mostly order-independent.

This is arguing for the sake of it since I said "current approach good" and "I'd just close this" (doing this now), but

$ ack 'after|before' DEPS 
    # Case-insensitivity for the Win SDK. Must run before win_toolchain below.
    # Update the Windows toolchain if necessary.  Must run before 'clang' below.
    # Note: On Win, this should run after win_toolchain, as it may use it.
    # Should run after the clang hook.
  # Pull GN binaries. This needs to be before running GYP below.

...that "independent hooks" ship has sailed a while ago.

Agreed. Marking as fixed.

  # Pull GN binaries. This needs to be before running GYP below.
                                                      ^~~

This is probably a stale comment :-)

It's almost like there's a pattern to those order-sensitive hooks, like they relate to one person's particular project. Hmm ... :).

At any rate, yes, I agree there are really ordering issues now (and likely always will be), but even that's separate from breaking something up that needs to run in two different linked hooks.

That's because I know how to grep for those :-) I know that various android hooks also need to be run in a specific order ('cause I broke something when I tried reordering for something), but they appear to not have comments calling this out.