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

Issue 667989 link

Starred by 0 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Analyzer thinks iossim.mm change does not warrant recompilation of iossim target

Project Member Reported by s...@google.com, Nov 23 2016

Issue description

Comment 1 by benhenry@google.com, Nov 23 2016

Labels: Pri-2
Owner: sdefresne@chromium.org
Status: Assigned (was: Untriaged)
Seems a deps issue in BUILD.
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?
Cc: sdefresne@chromium.org
Owner: dpranke@chromium.org
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
Labels: -Pri-2 Pri-1

Comment 6 by s...@google.com, Jun 8 2017

Cc: liaoyuke@chromium.org
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?
Status: Started (was: Assigned)
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.
Labels: -Restrict-View-Google
Cc: brettw@chromium.org
Patch up for review: https://chromium-review.googlesource.com/c/528527/
Project Member

Comment 10 by bugdroid1@chromium.org, 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

Project Member

Comment 11 by bugdroid1@chromium.org, 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

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.
Labels: cit-pm-52 Type-Bug
Labels: -cit-pm-52 cit-pm-53
Labels: -Pri-1 Pri-2
Owner: liaoyuke@chromium.org
Components: Infra>Client>Chrome
Components: -Infra>Client>iOS
Moved all Infra>Client>iOS bugs to Infra>Client>Chrome + OS-iOS.
Cc: -liaoyuke@chromium.org
Owner: ----
Status: (was: Started)

Comment 20 by efoo@chromium.org, Jun 2 2018

Friendly ping. This is a blocking bug on cit-pm-53. Please update need and priority accordingly. 
This bug had an unsupported status. Updating to Untriaged so someone will reevaluate.
Status: Untriaged
Owner: dpranke@chromium.org
Status: Assigned (was: Untriaged)
Looks like dirk has the most context here given #10,#11,#12.

Sign in to add a comment