Issue 6566059: Ninja --generator-output to match other generators. (Closed)
|
Created:
11 years, 6 months ago by bungeman of chrome Modified:
11 years, 6 months ago CC:
gyp-developer_googlegroups.com Base URL:
http://gyp.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionNinja --generator-output to match other generators.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix absolute path handling as well. #Patch Set 3 : Works with Tests and Skia. #
Total comments: 2
MessagesTotal messages: 11
This grew out of some discussion at https://codereview.appspot.com/6455075/ . This change allows Skia to build with ninja using skia_gyp with minimal modification (a change to use a relative path for the output directory is still need). The change itself is to use --generator-output to calculate the output directory as other generators appear to do.
Sign in to reply to this message.
Thanks! https://codereview.appspot.com/6566059/diff/1/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): https://codereview.appspot.com/6566059/diff/1/pylib/gyp/generator/ninja.py#ne... pylib/gyp/generator/ninja.py:1306: config_name)) What's the error message if generator-output is set to an absolute path? Something understandable? Could this convert absolute paths to relative paths automatically (by making them relative to options.toplevel_dir below for example)? The following is for my future self, not for you: Other generators have a build directory (out/ for make, xcodebuild/ for xcode, build/ for msvs) where build output appears (output_dir) and by default wrote build files into the source tree. Make would write a "Makefile" into the project root and then .mk files next to all gyp files. xcode and msvs both wrote their files next to the .gyp files. Some people didn't like this and added generator_output, which changes where project files are generated, but which doesn't affect where object files and built projects appear. (However, at least for make, output_dir seems to be relative to generator_output. This is confusing to at least some folks, https://github.com/joyent/node/blob/master/tools/gyp_node seems to think -Goutput_dir is relative to the project root, not to generator-output) For ninja, the generated ninja files are always in the build directory, so --generator-output is only here to be more similar to the make generator. I think it's worthwhile to make this change.
Sign in to reply to this message.
With Patch Set 2, Skia's gyp_skia needs no modification to run the ninja generator. This patch appears to take care of both the absolute path issue and the --generator-output issue. https://codereview.appspot.com/6566059/diff/1/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): https://codereview.appspot.com/6566059/diff/1/pylib/gyp/generator/ninja.py#ne... pylib/gyp/generator/ninja.py:1306: config_name)) On 2012/09/27 06:28:10, thakis wrote: > What's the error message if generator-output is set to an absolute path? Well, before it was impossible to get this far if generator-output was specified, so no existing clients will be broken if generator-output is set to an absolute path. There has never been an error if output_dir was an absolute path either. The doc for os.path.join says 'If any component is an absolute path, all previous components are thrown away, and joining continues,' so absolute paths should not cause any new issue here. > Could this convert absolute paths to relative paths automatically (by making them relative to options.toplevel_dir below for example)? Sure, why not. Currently the only place options.toplevel_dir is really being used is to find the relative bath of build files. Seems logical that the build_dir should get the same treatment. I'll write that up. >The following is for my future self, not for you: Perhaps some of this should be a comment here?
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
Unfortunately, this is breaking a test.
Sign in to reply to this message.
On 2012/09/29 00:16:11, bungeman wrote: > Unfortunately, this is breaking a test. (You can run `gcl try changename` to do a trybot run with results appearing on the review.) Which test?
Sign in to reply to this message.
Patch Set 2 was breaking toplevel-tests (where top-level=..). I believe Patch Set 3 addresses this correctly. Also added are some explanatory comments, without which I was constantly confusing what what relative to what while writing this. Patch Set 3 is building Skia and passing tests.
Sign in to reply to this message.
lgtm Do you need me to land this? Can you send tryjobs? https://codereview.appspot.com/6566059/diff/9001/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): https://codereview.appspot.com/6566059/diff/9001/pylib/gyp/generator/ninja.py... pylib/gyp/generator/ninja.py:1301: # generator_dir: relative path from pwd to where make puts build files. # This is only to make migrating from make to ninja easier, ninja doesn't put anything there.
Sign in to reply to this message.
I can commit. I didn't start this CL with gcl, so I'm not sure if they will turn up here, but I've kicked off some tryjobs which can be seen at http://build.chromium.org/p/tryserver.nacl/waterfall . https://codereview.appspot.com/6566059/diff/9001/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): https://codereview.appspot.com/6566059/diff/9001/pylib/gyp/generator/ninja.py... pylib/gyp/generator/ninja.py:1301: # generator_dir: relative path from pwd to where make puts build files. On 2012/10/02 05:32:47, thakis wrote: > # This is only to make migrating from make to ninja easier, ninja doesn't put > anything there. Done.
Sign in to reply to this message.
On 2012/10/02 05:51:48, bungeman of chrome wrote: > I can commit. I didn't start this CL with gcl, so I'm not sure if they will turn > up here, but I've kicked off some tryjobs which can be seen at > http://build.chromium.org/p/tryserver.nacl/waterfall . If the test passes locally and you have the commit bit, you can land and watch go/gypbot afterwards -- should be fine. Thanks for working on this! > > https://codereview.appspot.com/6566059/diff/9001/pylib/gyp/generator/ninja.py > File pylib/gyp/generator/ninja.py (right): > > https://codereview.appspot.com/6566059/diff/9001/pylib/gyp/generator/ninja.py... > pylib/gyp/generator/ninja.py:1301: # generator_dir: relative path from pwd to > where make puts build files. > On 2012/10/02 05:32:47, thakis wrote: > > # This is only to make migrating from make to ninja easier, ninja doesn't put > > anything there. > > Done.
Sign in to reply to this message.
|
