Code review - Issue 6651064: gyp: generate "everything" and "most" targets instead of "all"https://codereview.appspot.com/2012-10-25T16:30:48+00:00rietveld
Message from unknown
2012-10-12T18:36:41+00:00epogerurn:md5:2499fd7eb0a963a1331c989e2319208c
Message from unknown
2012-10-12T18:43:51+00:00epogerurn:md5:862856e4f74d6bc19429d9e338c571bf
Message from epoger@google.com
2012-10-12T18:51:13+00:00epogerurn:md5:6eab701cdf2093ff4e8207f9f10e8fc1
At patchset 2, I have confirmed that this works properly on my Mac. The toplevel "make" commands listed in https://sites.google.com/site/skiadocs/user-documentation/quick-start-guides/mac work exactly as before: "make gm", "make bench", "make all", "make clean", etc.
I believe it will work on Linux, but I haven't tried yet.
I know this will require some changes to make.py to keep the Windows build working, but I would like to get the CL reviewed as it stands now and go from there.
Message from borenet@google.com
2012-10-12T19:00:07+00:00EricBurn:md5:1c71552d04718e83688476fef8dbff87
+djsollen@
https://codereview.appspot.com/6651064/diff/2001/skia.gyp
File skia.gyp (right):
https://codereview.appspot.com/6651064/diff/2001/skia.gyp#newcode14
skia.gyp:14: 'target_name': 'most',
Will we get in trouble for having multiple targets with the same name?
Message from epoger@google.com
2012-10-12T19:06:27+00:00epogerurn:md5:23f30b1a868d85e4c391698095f76eb1
https://codereview.appspot.com/6651064/diff/2001/skia.gyp
File skia.gyp (right):
https://codereview.appspot.com/6651064/diff/2001/skia.gyp#newcode14
skia.gyp:14: 'target_name': 'most',
On 2012/10/12 19:00:07, EricB wrote:
> Will we get in trouble for having multiple targets with the same name?
Do you mean the appearance of "most" targets, both here and in gyp/most.gyp? (And same for "everything")
I think we could avoid that, and still keep full functionality, by changing the guts of this file to read:
'targets': [
{
'target_name': 'alltargets',
'type': 'none',
'dependencies': [
'gyp/everything.gyp:everything',
'gyp/most.gyp:most',
],
},
],
Message from unknown
2012-10-12T19:08:26+00:00epogerurn:md5:6e6052dd38fe7c48c69fbf2eda6770a7
Message from borenet@google.com
2012-10-12T19:10:43+00:00EricBurn:md5:42930c2089609762aaee14c23a822d30
https://codereview.appspot.com/6651064/diff/2001/skia.gyp
File skia.gyp (right):
https://codereview.appspot.com/6651064/diff/2001/skia.gyp#newcode14
skia.gyp:14: 'target_name': 'most',
On 2012/10/12 19:06:28, epoger wrote:
> On 2012/10/12 19:00:07, EricB wrote:
> > Will we get in trouble for having multiple targets with the same name?
>
> Do you mean the appearance of "most" targets, both here and in gyp/most.gyp?
> (And same for "everything")
>
> I think we could avoid that, and still keep full functionality, by changing the
> guts of this file to read:
>
> 'targets': [
> {
> 'target_name': 'alltargets',
> 'type': 'none',
> 'dependencies': [
> 'gyp/everything.gyp:everything',
> 'gyp/most.gyp:most',
> ],
> },
> ],
>
I think that's better. If I recall correctly, the catalyst of this whole discussion was the fact that ninja was complaining about multiple "all" targets, although maybe that only applies to targets in the same directory?
Message from tfarina@chromium.org
2012-10-12T19:18:51+00:00tfarina1urn:md5:699c06282357bbb0cf354195874c788c
https://codereview.appspot.com/6651064/diff/2001/gyp/everything.gyp
File gyp/everything.gyp (right):
https://codereview.appspot.com/6651064/diff/2001/gyp/everything.gyp#newcode13
gyp/everything.gyp:13: 'target_name': 'everything',
is this needed? if you want to build the debugger can't you just say:
ninja -C out/Debug debugger?
Message from tfarina@chromium.org
2012-10-12T19:24:35+00:00tfarina1urn:md5:ed3287d3eb36c0e1ab48dc234cc3e213
The problem I see here is that skia is fragmented in a bunch of small static_libraries (animator, core, effects, freetype, gpu, opts, images, ports, utils, views, etc...)
What we need something like:
{
'target_name': 'All',
'type': 'none',
'dependencies': [
animator.gyp:animator,
views.gyp:views,
core.gyp:core,
etc,
],
},
Message from tfarina@chromium.org
2012-10-12T19:26:25+00:00tfarina1urn:md5:a624722b4879e8d5bd5435796d3c85cd
moving myself to the CC list again.
Message from epoger@google.com
2012-10-12T19:26:34+00:00epogerurn:md5:b8940a1870a9f0742e24210b230b32b2
https://codereview.appspot.com/6651064/diff/2001/gyp/everything.gyp
File gyp/everything.gyp (right):
https://codereview.appspot.com/6651064/diff/2001/gyp/everything.gyp#newcode13
gyp/everything.gyp:13: 'target_name': 'everything',
On 2012/10/12 19:18:51, tfarina1 wrote:
> is this needed? if you want to build the debugger can't you just say:
>
> ninja -C out/Debug debugger?
Something like that. But again, I'm trying to keep it where you can just run "make all" from trunk and BOOM it builds everything.
https://codereview.appspot.com/6651064/diff/2001/skia.gyp
File skia.gyp (right):
https://codereview.appspot.com/6651064/diff/2001/skia.gyp#newcode14
skia.gyp:14: 'target_name': 'most',
On 2012/10/12 19:10:44, EricB wrote:
> On 2012/10/12 19:06:28, epoger wrote:
> > On 2012/10/12 19:00:07, EricB wrote:
> > > Will we get in trouble for having multiple targets with the same name?
> >
> > Do you mean the appearance of "most" targets, both here and in gyp/most.gyp?
> > (And same for "everything")
> >
> > I think we could avoid that, and still keep full functionality, by changing
> the
> > guts of this file to read:
> >
> > 'targets': [
> > {
> > 'target_name': 'alltargets',
> > 'type': 'none',
> > 'dependencies': [
> > 'gyp/everything.gyp:everything',
> > 'gyp/most.gyp:most',
> > ],
> > },
> > ],
> >
>
> I think that's better. If I recall correctly, the catalyst of this whole
> discussion was the fact that ninja was complaining about multiple "all" targets,
> although maybe that only applies to targets in the same directory?
I made the change in patchset 3, and it seems to work fine.
Message from tfarina@chromium.org
2012-10-12T19:33:28+00:00tfarina1urn:md5:5ad2257da92b7022a4c1c0137962aaaa
https://codereview.appspot.com/6651064/diff/2001/gyp/most.gyp
File gyp/most.gyp (right):
https://codereview.appspot.com/6651064/diff/2001/gyp/most.gyp#newcode30
gyp/most.gyp:30: # Local Variables:
Unrelated to this patch, but still...
Can we removed this in a follow up? See the following patches for a background.
http://codereview.chromium.org/7310019
http://codereview.chromium.org/7300011
Message from borenet@google.com
2012-10-12T19:34:56+00:00EricBurn:md5:197840a2ec9453537223eb392082dcaa
This is looking good at patch set 3. I don't like that we have to use names like "everything" and "most" in addition to "all," but I guess it's necessary.
Message from unknown
2012-10-18T03:46:15+00:00epogerurn:md5:fa8c6a205a7b25f25f302feaf051355d
Message from unknown
2012-10-18T04:51:57+00:00epogerurn:md5:56f97297013cb8c89ab55214228c4509
Message from epoger@google.com
2012-10-18T05:03:48+00:00epogerurn:md5:b2d9ec835ee1aa4fbf46fc1a9382a4b1
Ready for review at patchset 5!
In patchset 4, I updated my local source tree, resolving conflicts that came from https://code.google.com/p/skia/source/detail?r=5936 .
In patchset 5, I added changes to make.py to keep the Windows toplevel "make" build in sync with the other platforms.
As of patchset 5, I have confirmed that "make", "make all", "make most", "make clean" all behave as expected on Linux, Mac, and Windows.
NOTE: With this change, "make all" actually attempts to build everything! "make all" fails on my Windows desktop, because I don't have QT installed... so some users who are used to "make all" succeeding may now see it fail. I think this means that the BuildAllOtherTargets step will start failing on some of our buildbots... we may want to replace "make all" in that step with "make most" (or just "make", which has the same effect), or else install QT on all the buildbots...
Message from borenet@google.com
2012-10-18T12:02:20+00:00EricBurn:md5:6192b557aaf2d89f45552d85ad81c155
On 2012/10/18 05:03:48, epoger wrote:
> Ready for review at patchset 5!
>
> In patchset 4, I updated my local source tree, resolving conflicts that came
> from https://code.google.com/p/skia/source/detail?r=5936 .
>
> In patchset 5, I added changes to make.py to keep the Windows toplevel "make"
> build in sync with the other platforms.
>
> As of patchset 5, I have confirmed that "make", "make all", "make most", "make
> clean" all behave as expected on Linux, Mac, and Windows.
>
> NOTE: With this change, "make all" actually attempts to build everything! "make
> all" fails on my Windows desktop, because I don't have QT installed... so some
> users who are used to "make all" succeeding may now see it fail. I think this
> means that the BuildAllOtherTargets step will start failing on some of our
> buildbots... we may want to replace "make all" in that step with "make most" (or
> just "make", which has the same effect), or else install QT on all the
> buildbots...
LGTM. FWIW, since we're adding "most" and "everything", I think we should be building them on the bots, if maybe not at first. That would include the debugger, which we really should be building. I guess that means we need to install QT on the bots, *or* we have one bot (on each platform?) which builds the debugger so that not every machine needs QT. Here's my proposed plan:
1. Write up a quick change to run "make most" instead of "make all" on the bots.
2. Commit both changes at the "same time" and restart the master to pick them up.
3. Decide which machines should have QT installed, install it, and add "make everything" to some set of bots.
Message from epoger@google.com
2012-10-18T13:26:47+00:00epogerurn:md5:2a059cb737170e6732a3b81efc01e9bb
On 2012/10/18 12:02:20, EricB wrote:
> On 2012/10/18 05:03:48, epoger wrote:
> > Ready for review at patchset 5!
> >
> > In patchset 4, I updated my local source tree, resolving conflicts that came
> > from https://code.google.com/p/skia/source/detail?r=5936 .
> >
> > In patchset 5, I added changes to make.py to keep the Windows toplevel "make"
> > build in sync with the other platforms.
> >
> > As of patchset 5, I have confirmed that "make", "make all", "make most", "make
> > clean" all behave as expected on Linux, Mac, and Windows.
> >
> > NOTE: With this change, "make all" actually attempts to build everything!
> "make
> > all" fails on my Windows desktop, because I don't have QT installed... so some
> > users who are used to "make all" succeeding may now see it fail. I think this
> > means that the BuildAllOtherTargets step will start failing on some of our
> > buildbots... we may want to replace "make all" in that step with "make most"
> (or
> > just "make", which has the same effect), or else install QT on all the
> > buildbots...
>
> LGTM. FWIW, since we're adding "most" and "everything", I think we should be
> building them on the bots, if maybe not at first. That would include the
> debugger, which we really should be building. I guess that means we need to
> install QT on the bots, *or* we have one bot (on each platform?) which builds
> the debugger so that not every machine needs QT. Here's my proposed plan:
>
> 1. Write up a quick change to run "make most" instead of "make all" on the bots.
>
> 2. Commit both changes at the "same time" and restart the master to pick them
> up.
>
> 3. Decide which machines should have QT installed, install it, and add "make
> everything" to some set of bots.
Sounds good. I'll actually stretch it out a tiny bit more, just to avoid race conditions... I will knock out steps 1-3 as quickly as possible; #4 will happen "some other time". OK?
1. Write up CL #2 that adds a "most" target to the build, and commit it.
2. Write up CL #3 that changes the bots' BuildAllOtherTargets step to BuildMost, commit it, and restart the master.
3. Commit CL #1 (this one) that changes "make all" to chain to "make everything"
4 (later on). Decide which machines should have QT installed, install it, and add "make
everything" to some set of bots.
Message from borenet@google.com
2012-10-18T13:29:00+00:00EricBurn:md5:8ff1dfd0056eb5491069dc5f1fc642ee
SGTM
On Thu, Oct 18, 2012 at 9:26 AM, <epoger@google.com> wrote:
> On 2012/10/18 12:02:20, EricB wrote:
>
>> On 2012/10/18 05:03:48, epoger wrote:
>> > Ready for review at patchset 5!
>> >
>> > In patchset 4, I updated my local source tree, resolving conflicts
>>
> that came
>
>> > from https://code.google.com/p/**skia/source/detail?r=5936<https://code.google.com/p/skia/source/detail?r=5936>.
>> >
>> > In patchset 5, I added changes to make.py to keep the Windows
>>
> toplevel "make"
>
>> > build in sync with the other platforms.
>> >
>> > As of patchset 5, I have confirmed that "make", "make all", "make
>>
> most", "make
>
>> > clean" all behave as expected on Linux, Mac, and Windows.
>> >
>> > NOTE: With this change, "make all" actually attempts to build
>>
> everything!
>
>> "make
>> > all" fails on my Windows desktop, because I don't have QT
>>
> installed... so some
>
>> > users who are used to "make all" succeeding may now see it fail. I
>>
> think this
>
>> > means that the BuildAllOtherTargets step will start failing on some
>>
> of our
>
>> > buildbots... we may want to replace "make all" in that step with
>>
> "make most"
>
>> (or
>> > just "make", which has the same effect), or else install QT on all
>>
> the
>
>> > buildbots...
>>
>
> LGTM. FWIW, since we're adding "most" and "everything", I think we
>>
> should be
>
>> building them on the bots, if maybe not at first. That would include
>>
> the
>
>> debugger, which we really should be building. I guess that means we
>>
> need to
>
>> install QT on the bots, *or* we have one bot (on each platform?) which
>>
> builds
>
>> the debugger so that not every machine needs QT. Here's my proposed
>>
> plan:
>
> 1. Write up a quick change to run "make most" instead of "make all" on
>>
> the bots.
>
> 2. Commit both changes at the "same time" and restart the master to
>>
> pick them
>
>> up.
>>
>
> 3. Decide which machines should have QT installed, install it, and add
>>
> "make
>
>> everything" to some set of bots.
>>
>
> Sounds good. I'll actually stretch it out a tiny bit more, just to
> avoid race conditions... I will knock out steps 1-3 as quickly as
> possible; #4 will happen "some other time". OK?
>
> 1. Write up CL #2 that adds a "most" target to the build, and commit it.
>
> 2. Write up CL #3 that changes the bots' BuildAllOtherTargets step to
> BuildMost, commit it, and restart the master.
>
> 3. Commit CL #1 (this one) that changes "make all" to chain to "make
> everything"
>
> 4 (later on). Decide which machines should have QT installed, install
>
> it, and add "make
> everything" to some set of bots.
>
> https://codereview.appspot.**com/6651064/<https://codereview.appspot.com/6651064/>
>
Message from unknown
2012-10-23T15:09:13+00:00epogerurn:md5:ea24e1b4289a2fd1f80ae6fcb9ffe826
Message from unknown
2012-10-23T15:26:52+00:00epogerurn:md5:542376b9df201968bdd25693a89ffc0b
Message from epoger@google.com
2012-10-23T15:30:32+00:00epogerurn:md5:fc5a30d107f59ed8e830abd7244e60af
Eric / any other interested parties: please look at patchset 7 and the comments below. I think it's ready to commit now...
On 2012/10/18 13:26:47, epoger wrote:
> Sounds good. I'll actually stretch it out a tiny bit more, just to avoid race
> conditions... I will knock out steps 1-3 as quickly as possible; #4 will happen
> "some other time". OK?
>
> 1. Write up CL #2 that adds a "most" target to the build, and commit it.
#1 committed as https://code.google.com/p/skia/source/detail?r=5999
> 2. Write up CL #3 that changes the bots' BuildAllOtherTargets step to BuildMost,
> commit it, and restart the master.
#2 committed as https://code.google.com/p/skia/source/detail?r=6000
> 3. Commit CL #1 (this one) that changes "make all" to chain to "make everything"
#3 is ready to commit (IMHO) at patchset 7 in this CL.
> 4 (later on). Decide which machines should have QT installed, install it, and
> add "make
> everything" to some set of bots.
#4: filed https://code.google.com/p/skia/issues/detail?id=948 ('add "make all" (and thus QT libraries) to some or all buildbots') to track
Message from borenet@google.com
2012-10-23T15:54:34+00:00EricBurn:md5:25859b4a449f1abadd86c94e44904019
On 2012/10/23 15:30:32, epoger wrote:
> Eric / any other interested parties: please look at patchset 7 and the comments
> below. I think it's ready to commit now...
>
> On 2012/10/18 13:26:47, epoger wrote:
> > Sounds good. I'll actually stretch it out a tiny bit more, just to avoid race
> > conditions... I will knock out steps 1-3 as quickly as possible; #4 will
> happen
> > "some other time". OK?
> >
> > 1. Write up CL #2 that adds a "most" target to the build, and commit it.
>
> #1 committed as https://code.google.com/p/skia/source/detail?r=5999
>
> > 2. Write up CL #3 that changes the bots' BuildAllOtherTargets step to
> BuildMost,
> > commit it, and restart the master.
>
> #2 committed as https://code.google.com/p/skia/source/detail?r=6000
>
> > 3. Commit CL #1 (this one) that changes "make all" to chain to "make
> everything"
>
> #3 is ready to commit (IMHO) at patchset 7 in this CL.
>
> > 4 (later on). Decide which machines should have QT installed, install it, and
> > add "make
> > everything" to some set of bots.
>
> #4: filed https://code.google.com/p/skia/issues/detail?id=948 ('add "make all"
> (and thus QT libraries) to some or all buildbots') to track
LGTM at patch set 7.
Message from unknown
2012-10-25T16:30:48+00:00epogerurn:md5:d2ebc7d6e7b73821bda605791d535940