Why is my ChromeOS-only code being built for Windows?

[Simon Que]

I submitted this CL on Friday and it is now breaking on Windows:

https://codereview.chromium.org/986503002/

Here is the build output:

http://build.chromium.org/p/chromium.lkgr/builders/Win%20x64/builds/1964/steps/compile/logs/stdio

Is Windows using a GYP or GN build? I think it is GYP since GN is only recommended for Linux.

The file that has a build error, leak_detector_impl.cc, is included in the GYP/GN build as a part of a module, leak_detector:

https://codereview.chromium.org/986503002/diff/2120001/components/metrics.gypi

https://codereview.chromium.org/986503002/diff/2120001/components/metrics/BUILD.gn

This module is a dependency of the leak detector unit tests module, which is defined as ChromeOS-only:

https://codereview.chromium.org/986503002/diff/2120001/components/components_tests.gyp?context=10&column_width=80&tab_spaces=8

https://codereview.chromium.org/986503002/diff/2120001/components/metrics/BUILD.gn

Am I missing something here? Do modules get compiled and discarded even if nothing depends on them?

Simon

[Sigbjorn Finne]

Hi,

that bot builds target 'all';
https://code.google.com/p/chromium/issues/detail?id=451558#c3 describes
well what ninja does for that target.

I suspect you need to wrap up
https://codereview.chromium.org/986503002/patch/2120001/2130002 inside
an OS condition; cf. metrics_serialization in metrics.gypi.

(Addressing the compilation issue per crbug.com/167187 might be worth
doing in the short-term, regardless?)

--sigbjorn

[Thiago Farina]

On Sunday, November 22, 2015, Simon Que <sq...@chromium.org> wrote:

I submitted this CL on Friday and it is now breaking on Windows:

https://codereview.chromium.org/986503002/

Here is the build output:

http://build.chromium.org/p/chromium.lkgr/builders/Win%20x64/builds/1964/steps/compile/logs/stdio

Is Windows using a GYP or GN build? I think it is GYP since GN is only recommended for Linux.

If I'm not wrong we have switched only Linux so far to GN. We are still working on the other platforms to complete the transition. Dirk can correct me if I'm wrong.

And this builder is using gyp, you can see it running gyp at http://build.chromium.org/p/chromium.lkgr/builders/Win%20x64/builds/1964/steps/runhooks/logs/stdio

gn bots have GYP_CHROMIUM_NO_ACTION=true in order to stop running gyp as part of the hooks, so it can run 'gn gen' in a later pass.

--

Thiago Farina

[Simon Que]

I added some ChromeOS-conditional wrappers to metrics.gypi and metrics/BUILD.gn.

However, I'm getting compile errors in unrelated code when building the "all" target, e.g.

../../mojo/shell/application_manager_apptest_driver.cc:85:9: error: too many arguments to function call, expected 2, have 3

../../mojo/shell/application_manager_apptest_target.cc:28:10: error: no matching member function for call to 'ConnectToService'

../../ui/events/ozone/device/udev/device_manager_udev.cc:93:40: error: no member named 'WatchFileDescriptor' in 'base::MessageLoopForUI'

Nonetheless here is my fix for review: https://codereview.chromium.org/1468873002/

[Dirk Pranke]

On Sun, Nov 22, 2015 at 12:43 PM, Simon Que <sq...@chromium.org> wrote:

I added some ChromeOS-conditional wrappers to metrics.gypi and metrics/BUILD.gn.

However, I'm getting compile errors in unrelated code when building the "all" target, e.g.

../../mojo/shell/application_manager_apptest_driver.cc:85:9: error: too many arguments to function call, expected 2, have 3

../../mojo/shell/application_manager_apptest_target.cc:28:10: error: no matching member function for call to 'ConnectToService'

../../ui/events/ozone/device/udev/device_manager_udev.cc:93:40: error: no member named 'WatchFileDescriptor' in 'base::MessageLoopForUI'

Those are unrelated, as you say. I have a fixed posted for them elsewhere.

-- Dirk

[Nico Weber]

On Sun, Nov 22, 2015 at 1:28 AM, Simon Que <sq...@chromium.org> wrote:

I submitted this CL on Friday and it is now breaking on Windows:

https://codereview.chromium.org/986503002/

Here is the build output:

http://build.chromium.org/p/chromium.lkgr/builders/Win%20x64/builds/1964/steps/compile/logs/stdio

Is Windows using a GYP or GN build? I think it is GYP since GN is only recommended for Linux.

The file that has a build error, leak_detector_impl.cc, is included in the GYP/GN build as a part of a module, leak_detector:

https://codereview.chromium.org/986503002/diff/2120001/components/metrics.gypi

https://codereview.chromium.org/986503002/diff/2120001/components/metrics/BUILD.gn

In general, targets that should build only on one platform should be defined in an "if (that_platform)" block. Else they will get built when you ask ninja to build all targets it knows about, and it's a goal to have all targets that exist on each platform compile fine on all platforms. (And if your chromeos-only target is in an if(is_chromeos) block, then it'll only exist for chromeos builds.)

 

This module is a dependency of the leak detector unit tests module, which is defined as ChromeOS-only:

https://codereview.chromium.org/986503002/diff/2120001/components/components_tests.gyp?context=10&column_width=80&tab_spaces=8

https://codereview.chromium.org/986503002/diff/2120001/components/metrics/BUILD.gn

Am I missing something here? Do modules get compiled and discarded even if nothing depends on them?

Simon

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Stuart Morgan]

On Mon, Nov 23, 2015 at 9:45 AM Nico Weber <tha...@chromium.org> wrote:

it's a goal to have all targets that exist on each platform compile fine on all platforms.

Is there actually consensus on that at this point? I remember during all the iOS gyp change upstreaming getting pushback from people about moving targets into !iOS blocks, with people explicitly arguing that it didn't matter if building all targets on iOS failed because nobody should do that, and they should instead build a root target like All.

(I personally agree with you, it's just not clear to me that it's a project policy.)

-Stuart

[Colin Blundell]

I have also gotten strong feedback that having "ninja -C out/Debug" continue to work across all platforms was actually a *non-goal* in general, and that changing gypfiles solely for the purpose of preserving that feature on a specific platform (iOS in this case) was discouraged.

--

[Nico Weber]

Huh, if anyone disagrees with this, please talk to me :-)

(Or, Stuart, Colin: Let me know who pushed back, I'd like to talk to them.)

Having a project that actually compiles seems like the lowest level on a project's hierarchy of needs.

[Marc-Antoine Ruel]

2015-11-24 10:37 GMT-05:00 Colin Blundell <blun...@chromium.org>:

I have also gotten strong feedback that having "ninja -C out/Debug" continue to work across all platforms was actually a *non-goal* in general, and that changing gypfiles solely for the purpose of preserving that feature on a specific platform (iOS in this case) was discouraged.

lolwat

Redirect the person to Nico for a nice explanation and escalate to me for a more direct approach, if ever necessary.

M-A

[Brett Wilson]

On Tuesday, November 24, 2015, Nico Weber <tha...@chromium.org> wrote:

Huh, if anyone disagrees with this, please talk to me :-)

(Or, Stuart, Colin: Let me know who pushed back, I'd like to talk to them.)

Having a project that actually compiles seems like the lowest level on a project's hierarchy of needs.

Yeah, we broke this in the GN build accidentally and people were very confused.

However, if iOS people are aaying this, I suspect the genesis of this feeling was that we didn't want to has iOS conditionals around absolutely everything because of the messy way that iOS integrates (at least historically).

All main desktop builds should build everything properly. Whether I think we should do this for iOS will depend on what such a world looks like. Hopefully with the build cleanup and componentization work this will be reasonable.

Brett

[Nico Weber]

On Tue, Nov 24, 2015 at 11:47 AM, Brett Wilson <bre...@chromium.org> wrote:

On Tuesday, November 24, 2015, Nico Weber <tha...@chromium.org> wrote:

Huh, if anyone disagrees with this, please talk to me :-)

(Or, Stuart, Colin: Let me know who pushed back, I'd like to talk to them.)

Having a project that actually compiles seems like the lowest level on a project's hierarchy of needs.

Yeah, we broke this in the GN build accidentally and people were very confused.

However, if iOS people are aaying this, I suspect the genesis of this feeling was that we didn't want to has iOS conditionals around absolutely everything because of the messy way that iOS integrates (at least historically).

All main desktop builds should build everything properly. Whether I think we should do this for iOS will depend on what such a world looks like. Hopefully with the build cleanup and componentization work this will be reasonable.

Note that there's a right way and a wrong way to do these conditional targets.

The wrong way is to make the whole target conditional. Now you need to update everything depending on the target and make that conditional too. This gets very messy as you have to touch all targets depending on the conditional target.

The right way is to always have target foo and conditionally either make it a real target with sources or if it shouldn't exist make it a type: none target (or an empty group in gn). This way, everything can keep depending on it unconditionally and only the definition of the target gets a bit more complicated.

With the second approach, making targets conditional on ios (or elsewhere) shouldn't impose a high cost.

[Dirk Pranke]

On Tue, Nov 24, 2015 at 8:47 AM, Brett Wilson <bre...@chromium.org> wrote:

On Tuesday, November 24, 2015, Nico Weber <tha...@chromium.org> wrote:

Huh, if anyone disagrees with this, please talk to me :-)

(Or, Stuart, Colin: Let me know who pushed back, I'd like to talk to them.)

Having a project that actually compiles seems like the lowest level on a project's hierarchy of needs.

Yeah, we broke this in the GN build accidentally and people were very confused.

However, if iOS people are aaying this, I suspect the genesis of this feeling was that we didn't want to has iOS conditionals around absolutely everything because of the messy way that iOS integrates (at least historically).

All main desktop builds should build everything properly. Whether I think we should do this for iOS will depend on what such a world looks like. Hopefully with the build cleanup and componentization work this will be reasonable.

I don't think we're too far from having 'all' build on iOS in a GN build, and it should either work everywhere else now, or just have a couple of minor bugs keeping it from working. Due to the different way that GYP builds defined 'all', I think was harder to keep true w/ GYP.

I think we should try to make 'all' work everywhere, because (as Brett says) it's very confusing if you get random errors by accident.

-- Dirk

[Colin Blundell]

My expectation is that the combination of componentization/unforking and the modular nature of GN files will make the task of keeping "all" buildable for iOS much less dissimilar from that task on other platforms in the medium term.