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

34 views
Skip to first unread message

Simon Que

unread,
Nov 22, 2015, 1:30:03 AM11/22/15
to Chromium-dev, joc...@chromium.org, w...@chromium.org, inf...@chromium.org
I submitted this CL on Friday and it is now breaking on Windows:
https://codereview.chromium.org/986503002/

Here is the build output:

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:

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

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

Simon

Sigbjorn Finne

unread,
Nov 22, 2015, 2:17:15 AM11/22/15
to sq...@chromium.org, Chromium-dev, joc...@chromium.org, w...@chromium.org, inf...@chromium.org
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

unread,
Nov 22, 2015, 12:55:56 PM11/22/15
to sq...@chromium.org, Chromium-dev, joc...@chromium.org, w...@chromium.org, inf...@chromium.org


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:

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.


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

unread,
Nov 22, 2015, 3:45:14 PM11/22/15
to Thiago Farina, Chromium-dev, joc...@chromium.org, w...@chromium.org, inf...@chromium.org
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

unread,
Nov 22, 2015, 5:37:10 PM11/22/15
to sq...@chromium.org, Thiago Farina, Chromium-dev, joc...@chromium.org, w...@chromium.org, inf...@chromium.org
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

unread,
Nov 23, 2015, 12:46:38 PM11/23/15
to Simon Que, Chromium-dev, Jochen Eisinger, Will Harris, Abhishek Arya
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:

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:

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:

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

unread,
Nov 24, 2015, 10:19:33 AM11/24/15
to tha...@chromium.org, Simon Que, Chromium-dev, Jochen Eisinger, Will Harris, Abhishek Arya
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

unread,
Nov 24, 2015, 10:39:21 AM11/24/15
to stuart...@chromium.org, tha...@chromium.org, Simon Que, Chromium-dev, Jochen Eisinger, Will Harris, Abhishek Arya
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

unread,
Nov 24, 2015, 10:54:53 AM11/24/15
to Colin Blundell, Stuart Morgan, Simon Que, Chromium-dev, Jochen Eisinger, Will Harris, Abhishek Arya
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

unread,
Nov 24, 2015, 11:04:57 AM11/24/15
to Colin Blundell, Stuart Morgan, Nico Weber, Simon Que, Chromium-dev, Jochen Eisinger, Will Harris, Abhishek Arya
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

unread,
Nov 24, 2015, 11:49:13 AM11/24/15
to tha...@chromium.org, Colin Blundell, Stuart Morgan, Simon Que, Chromium-dev, Jochen Eisinger, Will Harris, Abhishek Arya
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

unread,
Nov 24, 2015, 11:51:37 AM11/24/15
to Brett Wilson, Colin Blundell, Stuart Morgan, Simon Que, Chromium-dev, Jochen Eisinger, Will Harris, Abhishek Arya
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

unread,
Nov 24, 2015, 11:55:26 AM11/24/15
to Brett Wilson, tha...@chromium.org, Colin Blundell, Stuart Morgan, Simon Que, Chromium-dev, Jochen Eisinger, Will Harris, Abhishek Arya
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

unread,
Nov 24, 2015, 2:45:18 PM11/24/15
to Dirk Pranke, Brett Wilson, tha...@chromium.org, Colin Blundell, Stuart Morgan, Simon Que, Chromium-dev, Jochen Eisinger, Will Harris, Abhishek Arya
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.
Reply all
Reply to author
Forward
0 new messages