Issue 6598054: Rename 'all' target to 'everything'. (Closed)
DescriptionRename 'all' target to 'everything'.
So ninja doesn't warn saying that multiple rules are being generated.
R=bungeman@google.com
Patch Set 1 #Patch Set 2 : rename to everything #MessagesTotal messages: 17
What you think? Is it worth? Now that you landed https://code.google.com/p/skia/source/detail?r=5765. Thanks,
Sign in to reply to this message.
I'm not really the one to ask about changing the names of top level targets. Roping in the build guys who know more about what is being depended on. Personally, I think this target should probably be called 'most' since I think the reason it exists was to exclude the debugger from building all the time. As a note, both make and ninja gyp generators automatically create a top-level 'all' target. In make this is done with 'phony' so it doesn't matter that we also have an 'all' target. With make I think our 'all' target is effectively ignored because the generated 'all' contains everything our 'all' does, possibly plus some more. With ninja things a a bit more strict. Since there are two distinct top-level targets named 'all' (ours and the generated one) we get this warning. I'm not sure what the actual behavior of ninja is in this situation, but it does give a nice warning about it.
Sign in to reply to this message.
On 2012/10/02 17:32:47, bungeman wrote: > I'm not really the one to ask about changing the names of top level targets. Nobody is... > Roping in the build guys who know more about what is being depended on. > Personally, I think this target should probably be called 'most' since I think > the reason it exists was to exclude the debugger from building all the time. > > As a note, both make and ninja gyp generators automatically create a top-level > 'all' target. > > In make this is done with 'phony' so it doesn't matter that we also have an > 'all' target. With make I think our 'all' target is effectively ignored because > the generated 'all' contains everything our 'all' does, possibly plus some more. > > With ninja things a a bit more strict. Since there are two distinct top-level > targets named 'all' (ours and the generated one) we get this warning. I'm not > sure what the actual behavior of ninja is in this situation, but it does give a > nice warning about it. AFAIK, we added the "all" target so that developers could "make all" regardless of build system. (For example, I don't think an "all" target is automatically generated for the xcode build, or at least it wasn't at some time in the past.) But we could keep that with a different name. Maybe "everything" instead of "all"? (Target names that differ only by capitalization make me nervous.)
Sign in to reply to this message.
Changed to 'everything' per Elliot request. Please, take another look.
Sign in to reply to this message.
Mike, are you fine with this?
Sign in to reply to this message.
On 2012/10/03 15:54:29, tfarina1 wrote: > Mike, are you fine with this? I'm going to ask around at lunch today. I don't know how people will feel about the meaning of "make all" changing, and no longer working on all platforms/compilers.
Sign in to reply to this message.
No strong opinion, deferring to bungeman/epoger/borenet
Sign in to reply to this message.
Elliot, would be fine to remove gyp/all.gyp? Or is it used somewhere? There is an 'all' target defined there too.
Sign in to reply to this message.
Abandoning "make all" is abandoning the standard. If ninja doesn't like it, ninja is wrong.
Sign in to reply to this message.
On 2012/10/03 15:56:41, epoger wrote: > On 2012/10/03 15:54:29, tfarina1 wrote: > > Mike, are you fine with this? > > I'm going to ask around at lunch today. I don't know how people will feel about > the meaning of "make all" changing, and no longer working on all > platforms/compilers. This doesn't actually change the meaning of "make all," since that's defined in the top-level Makefile. The situation with that Makefile and the one generated by gyp is kind of messy; if you want to build a target from the top-level Makefile, it has to be listed as a dependency of "all" (or "everything") in skia.gyp, but nobody every builds skia.gyp's "all" (or "everything") target, since our Makefile lists the targets individually. That's a confusing situation and is probably bad, but this change doesn't modify the behavior of running "make all" inside trunk.
Sign in to reply to this message.
On 2012/10/03 16:00:05, TomH wrote: > Abandoning "make all" is abandoning the standard. > If ninja doesn't like it, ninja is wrong. I have to agree with Tom. "make all" seems pretty natural/standard. Changing to "make everything" will cause a lot of pain to other people. It seems we need a different fix for this issue.
Sign in to reply to this message.
Ben, can you please take point on this? The long and short of it is, we want "make all" to work on all platforms, all compilers. If some internal target or gypfile names change to make that happen, it's fine. I appreciate Thiago's work on this, but IMHO we need someone on the Skia development team to "own" this. "own" it into submission.
Sign in to reply to this message.
I don't have a lot of background here, but: - if this file is brought in to the main Chrome build, then it's probably confusing that building "all" on Chrome builds only all of skia. if this file is only specific to building skia outside of chrome please disregard this - both the ninja and make generators for gyp implicitly create an "all" target that builds all available code, I'm not sure what this target is needed for in that perspective - neither ninja nor make have any implicit notion of a target named 'all'; that logic, if any exists, is done by gyp. if it's really important to you that your target be named all then it might be worth hacking gyp to *not* generate its own 'all' target that conflicts with yours. (ninja complains when you have multiple rules for the same target; make silently just adds them together)
Sign in to reply to this message.
On 2012/10/03 18:20:01, evan wrote: > - if this file is brought in to the main Chrome build It isn't. > it might be worth hacking gyp to *not* generate its own > 'all' target that conflicts with yours. I tried that. For the ninja generator it's trivial, but for the make generator it means changing an awful lot of code and maybe re-thinking the way it works.
Sign in to reply to this message.
Alternative proposed at https://codereview.appspot.com/6637045/ .
Sign in to reply to this message.
Oh one more thought: there's some gyp support for a "part_of_all" flag that lets you specify whether a gyp target is built when "all" is built. (It's never made much sense to me: why would "all" not build everything?) I mention it only in case it's useful for what you're trying to do. On Mon, Oct 8, 2012 at 2:56 PM, <bungeman@google.com> wrote: > Alternative proposed at https://codereview.appspot.**com/6637045/<https://codereview.appspot.com/6637.... > > https://codereview.appspot.**com/6598054/<https://codereview.appspot.com/6598... >
Sign in to reply to this message.
I think we can deprecate this CL in favor of https://codereview.appspot.com/6651064/ ('gyp: generate "everything" and "most" targets instead of "all"')
Sign in to reply to this message.
|
