Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(297)

Issue 6651064: gyp: generate "everything" and "most" targets instead of "all" (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by epoger
Modified:
1 year, 5 months ago
Reviewers:
bungeman, EricB, DerekS, TomH
CC:
skia-review_googlegroups.com, tfarina1
Base URL:
http://skia.googlecode.com/svn/trunk/
Visibility:
Public.

Description

gyp: generate "everything" and "most" targets instead of "all"
"make all" at the toplevel now chains to "make everything"

BUG=http://code.google.com/p/skia/issues/detail?id=932
Committed: https://code.google.com/p/skia/source/detail?r=6116

Patch Set 1 #

Patch Set 2 : #

Total comments: 7

Patch Set 3 : #

Patch Set 4 : synced_to_r5990 #

Patch Set 5 : add_make.py #

Patch Set 6 : synced_to_r6048 #

Patch Set 7 : comments_only #

Patch Set 8 : synced_to_r6115 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -86 lines) Patch
M Makefile View 1 2 3 4 5 6 7 3 chunks +24 lines, -20 lines 0 comments Download
D gyp/all.gyp View 1 2 3 4 5 6 7 1 chunk +0 lines, -41 lines 0 comments Download
A gyp/everything.gyp View 1 1 chunk +31 lines, -0 lines 0 comments Download
M make.py View 1 2 3 4 5 6 7 2 chunks +4 lines, -2 lines 0 comments Download
M skia.gyp View 1 2 3 4 5 6 7 1 chunk +3 lines, -23 lines 0 comments Download

Messages

Total messages: 16
epoger
At patchset 2, I have confirmed that this works properly on my Mac. The toplevel ...
1 year, 6 months ago #1
EricB
+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 ...
1 year, 6 months ago #2
epoger
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 ...
1 year, 6 months ago #3
EricB
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 ...
1 year, 6 months ago #4
tfarina1
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 ...
1 year, 6 months ago #5
tfarina1
The problem I see here is that skia is fragmented in a bunch of small ...
1 year, 6 months ago #6
tfarina1
moving myself to the CC list again.
1 year, 6 months ago #7
epoger
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 ...
1 year, 6 months ago #8
tfarina1
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... ...
1 year, 6 months ago #9
EricB
This is looking good at patch set 3. I don't like that we have to ...
1 year, 6 months ago #10
epoger
Ready for review at patchset 5! In patchset 4, I updated my local source tree, ...
1 year, 6 months ago #11
EricB
On 2012/10/18 05:03:48, epoger wrote: > Ready for review at patchset 5! > > In ...
1 year, 6 months ago #12
epoger
On 2012/10/18 12:02:20, EricB wrote: > On 2012/10/18 05:03:48, epoger wrote: > > Ready for ...
1 year, 6 months ago #13
EricB
SGTM On Thu, Oct 18, 2012 at 9:26 AM, <epoger@google.com> wrote: > On 2012/10/18 12:02:20, ...
1 year, 6 months ago #14
epoger
Eric / any other interested parties: please look at patchset 7 and the comments below. ...
1 year, 5 months ago #15
EricB
1 year, 5 months ago #16
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1278:e6ce13d99bf5