Move net_unittests and net_unittests_run inside a inside_chromium_build==1 condition.

Move net_unittests and net_unittests_run inside a inside_chromium_build==1 condition.

WebKit doesn't checkout all the necessary dependencies to be able to run
net_unittests successfully. Since this test was never run on the
build.webkit.org builders, this was never observed.

There is no reason to even build this target since it wouldn't pass anyway. So
move the 2 targets inside the inside_chromium_build==1 condition.

This will turn the Chromium Win Release builder back green.

R=rsleevi@chromium.org
BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146115

[M-A Ruel, 2012-07-10 13:56:08]

There's a probability that this change could break builds I don't yet know
about.

I moved the 2 targets around and didn't modify them. I verified gyp_chromium
sets inside_chromium_build==1 and gyp_webkit sets inside_chromium_build==0.

https://chromiumcodereview.appspot.com/10698126/diff/1/net/net.gyp
File net/net.gyp (right):

https://chromiumcodereview.appspot.com/10698126/diff/1/net/net.gyp#newcode1756
net/net.gyp:1756: ['os_posix == 1 and OS != "mac" and OS != "android"', {
One shall fix alignment for this block on day. It's been misaligned for 2 years.

[Ryan Sleevi, 2012-07-10 14:31:07]

Could you explain why this is necessary? What changed recently to break this? Is
some target explicitly doing something like trying to build net.gyp:*, even
though it can't/shouldn't be?

I can imagine more targets in net.gyp only being applicable here, so before
moving this, I'd like to understand a bit the motivation. I did see the breakage
on the WK waterfall, but I couldn't see when it actually broke.

[M-A Ruel, 2012-07-10 15:26:33]

On 2012/07/10 14:31:07, Ryan Sleevi wrote:
> Could you explain why this is necessary? What changed recently to break this?
Is
> some target explicitly doing something like trying to build net.gyp:*, even
> though it can't/shouldn't be?
> 
> I can imagine more targets in net.gyp only being applicable here, so before
> moving this, I'd like to understand a bit the motivation. I did see the
breakage
> on the WK waterfall, but I couldn't see when it actually broke.

1. The gyp generators verify that all inputs files are presents.
2. With the test isolation effort, I'm tracing all the tests to figure out the
run-time dependencies.
3. The inputs files for 'foo_run' are the *run-time* files needed to be present
to successfully run the test.
4. net_unittests was never run on WebKit buildbot and would fail if it was, due
to missing gclient dependencies.
5. I could generalize it more but it's not valuable at the moment.

[Ryan Sleevi, 2012-07-10 15:46:12]

On 2012/07/10 15:26:33, Marc-Antoine Ruel wrote:
> 1. The gyp generators verify that all inputs files are presents.
> 2. With the test isolation effort, I'm tracing all the tests to figure out the
> run-time dependencies.
> 3. The inputs files for 'foo_run' are the *run-time* files needed to be
present
> to successfully run the test.
> 4. net_unittests was never run on WebKit buildbot and would fail if it was,
due
> to missing gclient dependencies.
> 5. I could generalize it more but it's not valuable at the moment.

3) The way net_unittests is currently structured, the run-time dependencies were
meant to vary depending whether or not inside_chromium_build is true. It looks
like this may be arising because the run-time dependencies are unconditionally
reflecting the 'full' dependencies?

My concern is that inside_chromium_build feels like it's very much meant for the
'content or higher' layer, and really more of a "WebKit.gyp" layer. It doesn't
seem necessary or appropriate to move all of net_unittests within that condition
- they can and should be compilable without moving them.

From the quick scan of the build logs, am I correct that you're trying to gate
on whether or not Python is present, and using "inside_chromium_build" to
determine whether or not hermetic Python was pulled? Isn't "Is Python Present" a
guarantee, considering we're running GYP?

The dependencies on chrome/test/data/etc are only applicable for the
TestServer.py, and not even used by any unittests (directly) exercised by
net_unittests, so it seems the dependency tracking is off/incorrect (mod
http://crbug.com/119403).

[M-A Ruel, 2012-07-10 18:25:17]

On 2012/07/10 15:46:12, Ryan Sleevi wrote:
> On 2012/07/10 15:26:33, Marc-Antoine Ruel wrote:
> > 1. The gyp generators verify that all inputs files are presents.
> > 2. With the test isolation effort, I'm tracing all the tests to figure out
the
> > run-time dependencies.
> > 3. The inputs files for 'foo_run' are the *run-time* files needed to be
> present
> > to successfully run the test.
> > 4. net_unittests was never run on WebKit buildbot and would fail if it was,
> due
> > to missing gclient dependencies.
> > 5. I could generalize it more but it's not valuable at the moment.
> 
> 3) The way net_unittests is currently structured, the run-time dependencies
were
> meant to vary depending whether or not inside_chromium_build is true. It looks
> like this may be arising because the run-time dependencies are unconditionally
> reflecting the 'full' dependencies?

You think? I never built or run net_unittests from a WebKit based checkout so I
can't attest. My hypothesis is: I don't think anyone did.

> My concern is that inside_chromium_build feels like it's very much meant for
the
> 'content or higher' layer, and really more of a "WebKit.gyp" layer. It doesn't
> seem necessary or appropriate to move all of net_unittests within that
condition
> - they can and should be compilable without moving them.

It is compilable but from what I understand, it probably fails. And I couldn't
see it being run on the webkit.org buildbot. If you prefer, I'm fine with moving
only net_unittests_run in the condition.

> From the quick scan of the build logs, am I correct that you're trying to gate
> on whether or not Python is present, and using "inside_chromium_build" to
> determine whether or not hermetic Python was pulled? Isn't "Is Python Present"
a
> guarantee, considering we're running GYP?

I'd prefer to no specialize on hermetic python. For example,
src/third_party/tlslite is checked in as-is but nothing inhibit net_unittests
from eventually depending on something fetched by a gclient dependency.


> The dependencies on chrome/test/data/etc are only applicable for the
> TestServer.py, and not even used by any unittests (directly) exercised by
> net_unittests, so it seems the dependency tracking is off/incorrect (mod
> http://crbug.com/119403).

[jochen (gone - plz use gerrit), 2012-07-10 18:31:54]

for reference, here's a failing cr-win build:
http://build.webkit.org/builders/Chromium%20Win%20Release/builds/46078/steps/...

[Ryan Sleevi, 2012-07-10 18:36:05]

On 2012/07/10 18:25:17, Marc-Antoine Ruel wrote:
> On 2012/07/10 15:46:12, Ryan Sleevi wrote:
> > On 2012/07/10 15:26:33, Marc-Antoine Ruel wrote:
> > > 1. The gyp generators verify that all inputs files are presents.
> > > 2. With the test isolation effort, I'm tracing all the tests to figure out
> the
> > > run-time dependencies.
> > > 3. The inputs files for 'foo_run' are the *run-time* files needed to be
> > present
> > > to successfully run the test.
> > > 4. net_unittests was never run on WebKit buildbot and would fail if it
was,
> > due
> > > to missing gclient dependencies.
> > > 5. I could generalize it more but it's not valuable at the moment.
> > 
> > 3) The way net_unittests is currently structured, the run-time dependencies
> were
> > meant to vary depending whether or not inside_chromium_build is true. It
looks
> > like this may be arising because the run-time dependencies are
unconditionally
> > reflecting the 'full' dependencies?
> 
> You think? I never built or run net_unittests from a WebKit based checkout so
I
> can't attest. My hypothesis is: I don't think anyone did.
> 
> > My concern is that inside_chromium_build feels like it's very much meant for
> the
> > 'content or higher' layer, and really more of a "WebKit.gyp" layer. It
doesn't
> > seem necessary or appropriate to move all of net_unittests within that
> condition
> > - they can and should be compilable without moving them.
> 
> It is compilable but from what I understand, it probably fails. And I couldn't
> see it being run on the http://webkit.org buildbot. If you prefer, I'm fine
with moving
> only net_unittests_run in the condition.

Let's go with just moving net_unittest_run then, if that works.

I think the root of the issue is that GYP will try to generate .vcproj for all
targets within a GYP file, even if they're never referenced, due to the fact
that it tries to generate a .sln for each .gyp file. If the GYP MSVS generator
only generated a .sln for the .gyp file referenced, then it would never have
tried to create a .vcproj (for net_unittests or net_unittests_run), and thus
would never have warned about missing inputs.

Perhaps something to follow-up with on gyp-developer if you agree.

[M-A Ruel, 2012-07-10 18:42:36]

On 2012/07/10 18:36:05, Ryan Sleevi wrote:
> On 2012/07/10 18:25:17, Marc-Antoine Ruel wrote:
> > It is compilable but from what I understand, it probably fails. And I
couldn't
> > see it being run on the http://webkit.org buildbot. If you prefer, I'm fine
> with moving
> > only net_unittests_run in the condition.
> 
> Let's go with just moving net_unittest_run then, if that works.

Done.
 
> I think the root of the issue is that GYP will try to generate .vcproj for all
> targets within a GYP file, even if they're never referenced, due to the fact
> that it tries to generate a .sln for each .gyp file. If the GYP MSVS generator
> only generated a .sln for the .gyp file referenced, then it would never have
> tried to create a .vcproj (for net_unittests or net_unittests_run), and thus
> would never have warned about missing inputs.
> 
> Perhaps something to follow-up with on gyp-developer if you agree.

That's an interesting thought but I don't think it'd work in practice. Projects
not referenced are often important and I don't see a good automated signal to
differentiate that. It's better to just have non-important targets to _not_ be
defined with a condition. :)

[Ryan Sleevi, 2012-07-10 19:50:18]

On 2012/07/10 18:42:36, Marc-Antoine Ruel wrote:
> On 2012/07/10 18:36:05, Ryan Sleevi wrote:
> > On 2012/07/10 18:25:17, Marc-Antoine Ruel wrote:
> > > It is compilable but from what I understand, it probably fails. And I
> couldn't
> > > see it being run on the http://webkit.org buildbot. If you prefer, I'm
fine
> > with moving
> > > only net_unittests_run in the condition.
> > 
> > Let's go with just moving net_unittest_run then, if that works.
> 
> Done.
>  
> > I think the root of the issue is that GYP will try to generate .vcproj for
all
> > targets within a GYP file, even if they're never referenced, due to the fact
> > that it tries to generate a .sln for each .gyp file. If the GYP MSVS
generator
> > only generated a .sln for the .gyp file referenced, then it would never have
> > tried to create a .vcproj (for net_unittests or net_unittests_run), and thus
> > would never have warned about missing inputs.
> > 
> > Perhaps something to follow-up with on gyp-developer if you agree.
> 
> That's an interesting thought but I don't think it'd work in practice.
Projects
> not referenced are often important and I don't see a good automated signal to
> differentiate that. It's better to just have non-important targets to _not_ be
> defined with a condition. :)

With ninja/make/xcode, it's not an issue. With MSVS, ./build/gyp_chromium builds
all.gyp, so it would have .vcproj for all (wildcard reachable) targets.
Hopefully if/when we switch to winja, this problem will all go away.

[Ryan Sleevi, 2012-07-10 19:50:21]

lgtm

[commit-bot: I haz the power, 2012-07-10 19:57:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/10698126/8001

[commit-bot: I haz the power, 2012-07-10 20:02:05]

Try job failure for 10698126-8001 (retry) on mac_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-11 13:23:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/10698126/1004

[commit-bot: I haz the power, 2012-07-11 14:43:54]

Change committed as 146115