Analyzer thinks iossim.mm change does not warrant recompilation of iossim target |
||||||||||||||||
Issue descriptionThis CL makes a minimal change to iossim.mm: https://codereview.chromium.org/2520993005 And this try job skipped compilation: https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/112115
,
Nov 25 2016
Seems a deps issue in BUILD.
,
Nov 25 2016
iossim is not a dependency of //:gn_all just a data_deps of some of them. dpranke: do you know if analyze respects data_deps?
,
Nov 25 2016
Yes, it's supposed to respect data_deps, and I believe it does. I think in this case, though, analyze may have gotten confused because the target is part of the host toolchain. I'll take a look at this to see what's going on
,
Jan 15 2017
,
Jun 8 2017
https://chromium-review.googlesource.com/c/527320/ is another CL making a change to iossim.mm, which should cause all simulator tests to be rerun but instead ios-simulator skipped all tests. Should we add an exclusion if we can't figure out why data_deps aren't being respected in this case?
,
Jun 9 2017
As I expected, analyze is only looking for files in the default toolchain. I don't remember why I wrote things that way, unfortunately. It looks simple enough to remove that restriction, so I'm testing that now and we'll see what happens.
,
Jun 9 2017
,
Jun 9 2017
,
Jun 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/21886543a0cbedeb869fb68944e13ef613cedceb commit 21886543a0cbedeb869fb68944e13ef613cedceb Author: Dirk Pranke <dpranke@chromium.org> Date: Fri Jun 09 20:08:39 2017 Fix a bug in `gn analyze` for host-only file mods. If you modified a file that only affected targets defined in a non-default toolchain (e.g., like the host toolchain), `gn analyze` would skip over it and hence not do the right thing. For example, if you modified //testing/iossim/iossim.mm, analyze would think that no compile was needed, when really you'd want to recompile the simulator and re-run every test. Unfortunately, I don't remember why I wrote the code the way I did, but looking at it now I can't think of a reason not to just look in every toolchain. BUG=667989 Change-Id: I8e1935d7dc23bf49fa87e191c1a136a5492d3678 Reviewed-on: https://chromium-review.googlesource.com/528527 Reviewed-by: smut <smut@chromium.org> Reviewed-by: Brett Wilson <brettw@chromium.org> Commit-Queue: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#478388} [modify] https://crrev.com/21886543a0cbedeb869fb68944e13ef613cedceb/tools/gn/analyzer.cc
,
Jun 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf0dae2525be222e524a7fcb8e6e370165ea1441 commit bf0dae2525be222e524a7fcb8e6e370165ea1441 Author: Dirk Pranke <dpranke@chromium.org> Date: Fri Jun 23 20:20:42 2017 Revert "Fix a bug in `gn analyze` for host-only file mods." This reverts commit 21886543a0cbedeb869fb68944e13ef613cedceb. Reason for revert: It looks like `gn analyze` returns targets in other toolchains in the label format `foo(//bar)` and MB isn't smart enough to turn those into the correct ninja outputs, and so running `gn analyze` on all does the wrong thing and compiles fail :(. See crbug.com/736215 for more. Original change's description: > Fix a bug in `gn analyze` for host-only file mods. > > If you modified a file that only affected targets defined in a > non-default toolchain (e.g., like the host toolchain), `gn analyze` > would skip over it and hence not do the right thing. For example, > if you modified //testing/iossim/iossim.mm, analyze would think > that no compile was needed, when really you'd want to recompile > the simulator and re-run every test. > > Unfortunately, I don't remember why I wrote the code the way I did, > but looking at it now I can't think of a reason not to just look in > every toolchain. > > BUG=667989 > > Change-Id: I8e1935d7dc23bf49fa87e191c1a136a5492d3678 > Reviewed-on: https://chromium-review.googlesource.com/528527 > Reviewed-by: smut <smut@chromium.org> > Reviewed-by: Brett Wilson <brettw@chromium.org> > Commit-Queue: Dirk Pranke <dpranke@chromium.org> > Cr-Commit-Position: refs/heads/master@{#478388} TBR=brettw@chromium.org,smut@google.com,dpranke@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 667989 Change-Id: I4042f17d185709edfca75987f1a32c864c2ea4ab Reviewed-on: https://chromium-review.googlesource.com/546679 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#482003} [modify] https://crrev.com/bf0dae2525be222e524a7fcb8e6e370165ea1441/tools/gn/analyzer.cc
,
Jun 23 2017
Turns out there was a reason I wrote things the way I did :). I need to look at fixing this in a slightly different way, working on that now.
,
Jun 30 2017
,
Jun 30 2017
,
Sep 2 2017
,
Dec 20 2017
,
Dec 22 2017
,
Dec 22 2017
Moved all Infra>Client>iOS bugs to Infra>Client>Chrome + OS-iOS.
,
Apr 20 2018
,
Jun 2 2018
Friendly ping. This is a blocking bug on cit-pm-53. Please update need and priority accordingly.
,
Jan 10
This bug had an unsupported status. Updating to Untriaged so someone will reevaluate.
,
Jan 10
,
Jan 10
Looks like dirk has the most context here given #10,#11,#12. |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by benhenry@google.com
, Nov 23 2016