Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13100003:
Made the default build target "most" which currently contains everything except (Closed)
|
Created:
7 years, 9 months ago by Andrei Mouravski Modified:
7 years, 8 months ago CC:
reviews_dartlang.org, dgrove Visibility:
Public. |
DescriptionMade the default build target "most" which currently contains everything except
api_docs. This was adapted from the skia repo.
Committed: https://code.google.com/p/dart/source/detail?r=20818
Patch Set 1 #
Total comments: 13
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Messages
Total messages: 13 (0 generated)
I think we can trim the "most" list even more. My suggestion is to actually make "create_sdk" the default.
One general note: We have to do these sort of changes in three steps: - add a new target to gyp - change the buildbot to use that new target (and restart the buildbot) - remove the old target Otherwise the buildbot will build the wrong thing (or will fail, if a target was removed). There are currently builders which build the 'all' target! https://codereview.chromium.org/13100003/diff/1/dart.gyp File dart.gyp (right): https://codereview.chromium.org/13100003/diff/1/dart.gyp#newcode9 dart.gyp:9: 'target_name': 'everything', There should be no need for an 'everything' target. AFAIK gyp will generate automatically an 'all' target (that's the on used if you run just './tools/build.py') -- which builds all targets. https://codereview.chromium.org/13100003/diff/1/dart.gyp#newcode18 dart.gyp:18: 'target_name': 'most', The name 'most' is not really descriptive. If somebody invokes './tools/build.py most', he has no idea what targets will be built. https://codereview.chromium.org/13100003/diff/1/tests/html/html.status File tests/html/html.status (right): https://codereview.chromium.org/13100003/diff/1/tests/html/html.status#newcod... tests/html/html.status:116: <<<<<<< HEAD This was probably unintentional. https://codereview.chromium.org/13100003/diff/1/tools/build.py File tools/build.py (right): https://codereview.chromium.org/13100003/diff/1/tools/build.py#newcode237 tools/build.py:237: target = 'most' If somebody types in './tools/build.py -mrelease' he expects everything to get build. Why do you want to change that behaviour?
On 2013/03/27 10:21:20, kustermann wrote: > One general note: We have to do these sort of changes in three steps: > - add a new target to gyp I imagine this meant reverting build.py, right?
https://codereview.chromium.org/13100003/diff/1/dart.gyp File dart.gyp (right): https://codereview.chromium.org/13100003/diff/1/dart.gyp#newcode9 dart.gyp:9: 'target_name': 'everything', On 2013/03/27 10:21:21, kustermann wrote: > There should be no need for an 'everything' target. AFAIK gyp will generate > automatically an 'all' target (that's the on used if you run just > './tools/build.py') -- which builds all targets. I was following the example of skia: https://code.google.com/p/skia/source/browse/trunk/gyp/?r=6409 I realize 'all' runs all targets, but it's named differently on MacOS, which is silly, so this solves that problem... sort of. https://codereview.chromium.org/13100003/diff/1/dart.gyp#newcode18 dart.gyp:18: 'target_name': 'most', On 2013/03/27 10:21:21, kustermann wrote: > The name 'most' is not really descriptive. If somebody invokes > './tools/build.py most', he has no idea what targets will be built. Is 'default' better? https://codereview.chromium.org/13100003/diff/1/tests/html/html.status File tests/html/html.status (right): https://codereview.chromium.org/13100003/diff/1/tests/html/html.status#newcod... tests/html/html.status:116: <<<<<<< HEAD On 2013/03/27 10:21:21, kustermann wrote: > This was probably unintentional. Aye. https://codereview.chromium.org/13100003/diff/1/tools/build.py File tools/build.py (right): https://codereview.chromium.org/13100003/diff/1/tools/build.py#newcode237 tools/build.py:237: target = 'most' On 2013/03/27 10:21:21, kustermann wrote: > If somebody types in './tools/build.py -mrelease' he expects everything to get > build. Why do you want to change that behaviour? This was a personal request from Dan, which is why I cc'ed him. He wanted api_docs removed from the default build to make it faster, which is why I commented that I'd prefer to make the default just the create_sdk (and some others) so it's faster. Actually, I'd really like our gyp targets to be cleaned up so that the dependencies are as minimal as possible. Having all gyp files in a common folder would be nice, too, but no hurry.
https://codereview.chromium.org/13100003/diff/1/tools/build.py File tools/build.py (right): https://codereview.chromium.org/13100003/diff/1/tools/build.py#newcode237 tools/build.py:237: target = 'most' Yes - this was primarily for the VM team, but also to speed up our build process in general. No one looks for these docs or uses them from their builds, so removing them from the normal build will make people's lives better. On 2013/03/27 17:45:40, Andrei Mouravski wrote: > On 2013/03/27 10:21:21, kustermann wrote: > > If somebody types in './tools/build.py -mrelease' he expects everything to get > > build. Why do you want to change that behaviour? > > This was a personal request from Dan, which is why I cc'ed him. He wanted > api_docs removed from the default build to make it faster, which is why I > commented that I'd prefer to make the default just the create_sdk (and some > others) so it's faster. > > Actually, I'd really like our gyp targets to be cleaned up so that the > dependencies are as minimal as possible. Having all gyp files in a common folder > would be nice, too, but no hurry.
https://codereview.chromium.org/13100003/diff/1/tools/build.py File tools/build.py (right): https://codereview.chromium.org/13100003/diff/1/tools/build.py#newcode237 tools/build.py:237: target = 'most' To which I ask, do most people need the editor? Or samples? Etc. On 2013/03/28 02:30:11, dgrove wrote: > Yes - this was primarily for the VM team, but also to speed up our build process > in general. No one looks for these docs or uses them from their builds, so > removing them from the normal build will make people's lives better. > > On 2013/03/27 17:45:40, Andrei Mouravski wrote: > > On 2013/03/27 10:21:21, kustermann wrote: > > > If somebody types in './tools/build.py -mrelease' he expects everything to > get > > > build. Why do you want to change that behaviour? > > > > This was a personal request from Dan, which is why I cc'ed him. He wanted > > api_docs removed from the default build to make it faster, which is why I > > commented that I'd prefer to make the default just the create_sdk (and some > > others) so it's faster. > > > > Actually, I'd really like our gyp targets to be cleaned up so that the > > dependencies are as minimal as possible. Having all gyp files in a common > folder > > would be nice, too, but no hurry. >
https://codereview.chromium.org/13100003/diff/1/tools/build.py File tools/build.py (right): https://codereview.chromium.org/13100003/diff/1/tools/build.py#newcode237 tools/build.py:237: target = 'most' probably not, but this step takes so long that it stands out. If you do a debug-mode build, this step easily takes 5+ minutes. On 2013/03/28 15:06:23, Andrei Mouravski wrote: > To which I ask, do most people need the editor? Or samples? Etc. > > On 2013/03/28 02:30:11, dgrove wrote: > > Yes - this was primarily for the VM team, but also to speed up our build > process > > in general. No one looks for these docs or uses them from their builds, so > > removing them from the normal build will make people's lives better. > > > > On 2013/03/27 17:45:40, Andrei Mouravski wrote: > > > On 2013/03/27 10:21:21, kustermann wrote: > > > > If somebody types in './tools/build.py -mrelease' he expects everything to > > get > > > > build. Why do you want to change that behaviour? > > > > > > This was a personal request from Dan, which is why I cc'ed him. He wanted > > > api_docs removed from the default build to make it faster, which is why I > > > commented that I'd prefer to make the default just the create_sdk (and some > > > others) so it's faster. > > > > > > Actually, I'd really like our gyp targets to be cleaned up so that the > > > dependencies are as minimal as possible. Having all gyp files in a common > > folder > > > would be nice, too, but no hurry. > > >
Ping. Dan and I are in agreement about this, so I'm just looking for an LGTM to start step one: add target to gyp.
lgtm with comments. It would be nice if you could send out an email saying that we no longer build the api documentation by default -- just to make people aware. https://codereview.chromium.org/13100003/diff/1/dart.gyp File dart.gyp (right): https://codereview.chromium.org/13100003/diff/1/dart.gyp#newcode9 dart.gyp:9: 'target_name': 'everything', On 2013/03/27 17:45:40, Andrei Mouravski wrote: > On 2013/03/27 10:21:21, kustermann wrote: > > There should be no need for an 'everything' target. AFAIK gyp will generate > > automatically an 'all' target (that's the on used if you run just > > './tools/build.py') -- which builds all targets. > > I was following the example of skia: > https://code.google.com/p/skia/source/browse/trunk/gyp/?r=6409 > I realize 'all' runs all targets, but it's named differently on MacOS, which is > silly, so this solves that problem... sort of. I wasn't aware that it's named differently on Mac. So please keep the everything target you had earlier (this way we don't need to pass 'All' on mac on 'all' on the other platforms in the buildbot configuration). https://codereview.chromium.org/13100003/diff/1/dart.gyp#newcode18 dart.gyp:18: 'target_name': 'most', On 2013/03/27 17:45:40, Andrei Mouravski wrote: > On 2013/03/27 10:21:21, kustermann wrote: > > The name 'most' is not really descriptive. If somebody invokes > > './tools/build.py most', he has no idea what targets will be built. > > Is 'default' better? I like 'default'. https://codereview.chromium.org/13100003/diff/7001/dart.gyp File dart.gyp (right): https://codereview.chromium.org/13100003/diff/7001/dart.gyp#newcode23 dart.gyp:23: 'packages', You could sort these dependencies alphabetically. https://codereview.chromium.org/13100003/diff/7001/dart.gyp#newcode24 dart.gyp:24: ], I would remove 'dartc_bot' and 'dart2js_bot' here.
Thanks. Submitting. https://codereview.chromium.org/13100003/diff/7001/dart.gyp File dart.gyp (right): https://codereview.chromium.org/13100003/diff/7001/dart.gyp#newcode23 dart.gyp:23: 'packages', On 2013/04/02 10:34:25, kustermann wrote: > You could sort these dependencies alphabetically. Done. https://codereview.chromium.org/13100003/diff/7001/dart.gyp#newcode24 dart.gyp:24: ], On 2013/04/02 10:34:25, kustermann wrote: > I would remove 'dartc_bot' and 'dart2js_bot' here. Done.
Message was sent while issue was closed.
Committed patchset #3 manually as r20818 (presubmit successful).
Message was sent while issue was closed.
We (ricow, ahe and I) had some discussions about the change to tools/build.py to use the 'default' target. We eventually agreed on a) Maintaining an 'everything' target will not work (people will forget to update it when they add/rename new targets ...). b) We want build.py to build everything. I reverted my change to build.py and peter submitted a change to exclude api_docs in the build step if we're building in debug mode -- which will help the VM team. I hope that's OK with you.
Message was sent while issue was closed.
On 2013/04/09 17:29:41, kustermann wrote: > We (ricow, ahe and I) had some discussions about the change to tools/build.py to > use the 'default' target. We eventually agreed on > > a) Maintaining an 'everything' target will not work (people will forget to > update it when they add/rename new targets ...). > > b) We want build.py to build everything. > > I reverted my change to build.py and peter submitted a change to exclude > api_docs in the build step if we're building in debug mode -- which will help > the VM team. > > I hope that's OK with you. That is totally acceptable to me. |
