Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23606009:
Fix eclipse generator when running on Windows. (Closed)
|
Created:
7 years, 3 months ago by Slava Chigrin Modified:
6 years ago CC:
gyp-developer_googlegroups.com Base URL:
http://gyp.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionFix eclipse generator when running on Windows.
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fix mistake in comment. #
Total comments: 4
Patch Set 3 : Don't call GetFlavor twice. #Messages
Total messages: 12 (0 generated)
This change may be helpful to those people, who prefer eclipse +ninja as build environment on Windows, rather then the Visual Studio. I am very new in GYP development, please, correct me if I made some mistakes. Thank you! https://codereview.chromium.org/23606009/diff/1/pylib/gyp/generator/eclipse.py File pylib/gyp/generator/eclipse.py (right): https://codereview.chromium.org/23606009/diff/1/pylib/gyp/generator/eclipse.p... pylib/gyp/generator/eclipse.py:71: # To determine processor word size on Windows, in addition to checking Seems, at present these lines are duplicated in ninja.py, msvs.py, and dump_dependency_json.py. If you want, I can refactor this, for example, by placing them as separate function in msvs_emulation.py. Although, may be it is better to do it as separate patch...
Thanks for your patch! The eclipse generator was written by jgreenwald, so he should take a look at this. As far as I know, it's mostly used for Android builds, and this patch makes "flavor == win" mean "building on win, but also building chrome for windows". That's probably fine though. https://codereview.chromium.org/23606009/diff/1/pylib/gyp/generator/eclipse.py File pylib/gyp/generator/eclipse.py (right): https://codereview.chromium.org/23606009/diff/1/pylib/gyp/generator/eclipse.p... pylib/gyp/generator/eclipse.py:60: # by the Windows Ninja generator. "by the Eclipse generator", right?
Thank you for review! Waiting for more comments from jgreenwald. https://codereview.chromium.org/23606009/diff/1/pylib/gyp/generator/eclipse.py File pylib/gyp/generator/eclipse.py (right): https://codereview.chromium.org/23606009/diff/1/pylib/gyp/generator/eclipse.p... pylib/gyp/generator/eclipse.py:60: # by the Windows Ninja generator. On 2013/08/31 20:18:36, Nico wrote: > "by the Eclipse generator", right? Done.
jgreenwald, could you look at these changes, please? I hope it will not take much time... Thank you in advance! On 2013/09/01 15:28:28, Slava Chigrin wrote: > Thank you for review! Waiting for more comments from jgreenwald. > > https://codereview.chromium.org/23606009/diff/1/pylib/gyp/generator/eclipse.py > File pylib/gyp/generator/eclipse.py (right): > > https://codereview.chromium.org/23606009/diff/1/pylib/gyp/generator/eclipse.p... > pylib/gyp/generator/eclipse.py:60: # by the Windows Ninja generator. > On 2013/08/31 20:18:36, Nico wrote: > > "by the Eclipse generator", right? > > Done.
Ping... Anybody could review these changes? Sorry for disturbing you... On 2013/09/05 14:51:26, Slava Chigrin wrote: > jgreenwald, could you look at these changes, please? I hope it will not take > much time... > Thank you in advance! > > On 2013/09/01 15:28:28, Slava Chigrin wrote: > > Thank you for review! Waiting for more comments from jgreenwald. > > > > https://codereview.chromium.org/23606009/diff/1/pylib/gyp/generator/eclipse.py > > File pylib/gyp/generator/eclipse.py (right): > > > > > https://codereview.chromium.org/23606009/diff/1/pylib/gyp/generator/eclipse.p... > > pylib/gyp/generator/eclipse.py:60: # by the Windows Ninja generator. > > On 2013/08/31 20:18:36, Nico wrote: > > > "by the Eclipse generator", right? > > > > Done.
Since jgreenwald isn't responding, I'll consider the eclipse generator unowned and will review this myself. Since it doesn't have any tests, breaking it is fair game :-) One question: Previously, it was possible to build projects using 'cflags' (e.g. Chrome/Android hosted on Windows – which is currently not working, but it's at least conceivable that it'd work at some point) in the eclipse generator. After this, the eclipse projects use the MS compiler flags instead. Is that something to be worried about? Is there some way to get the old behavior? If not, should there? (No idea, I use neither windows nor eclipse. If you say the patch is good as-is, I'll merge it.) https://codereview.chromium.org/23606009/diff/6001/pylib/gyp/generator/eclips... File pylib/gyp/generator/eclipse.py (right): https://codereview.chromium.org/23606009/diff/6001/pylib/gyp/generator/eclips... pylib/gyp/generator/eclipse.py:56: default_variables.setdefault('OS', gyp.common.GetFlavor(params)) pass 'flavor' as last arg here too
I am sorry, seems, I don't know all GYP features very well. How users should set environment to build Chrome/Android on Windows using eclipse generator? What should be GYP_GENERATORS variable set to, etc.? I'll verify that my changes did not broke this feature. When developing this patch I looked at ninja.py; in it WriteSourcesForArch() it also checks flavor, and uses either xcode_settings, or msvs_settings, or just cflags from config, thats why I decided to use similar approach. Thank you in advance! https://codereview.chromium.org/23606009/diff/6001/pylib/gyp/generator/eclips... File pylib/gyp/generator/eclipse.py (right): https://codereview.chromium.org/23606009/diff/6001/pylib/gyp/generator/eclips... pylib/gyp/generator/eclipse.py:56: default_variables.setdefault('OS', gyp.common.GetFlavor(params)) This method called fom gyp/__init__.py, so it signature common for many generators. Should I change this in all generators? On 2013/09/24 17:03:48, Nico wrote: > pass 'flavor' as last arg here too
https://codereview.chromium.org/23606009/diff/6001/pylib/gyp/generator/eclips... File pylib/gyp/generator/eclipse.py (right): https://codereview.chromium.org/23606009/diff/6001/pylib/gyp/generator/eclips... pylib/gyp/generator/eclipse.py:56: default_variables.setdefault('OS', gyp.common.GetFlavor(params)) On 2013/09/25 17:05:24, Slava Chigrin wrote: > This method called fom gyp/__init__.py, so it signature common for many > generators. Should I change this in all generators? Nono, I just mean do: flavor = gyp.common.GetFlavor(params) default_variables.setdefault('OS', flavor) instead of calling GetFlavor() twice. > > On 2013/09/24 17:03:48, Nico wrote: > > pass 'flavor' as last arg here too >
https://codereview.chromium.org/23606009/diff/6001/pylib/gyp/generator/eclips... File pylib/gyp/generator/eclipse.py (right): https://codereview.chromium.org/23606009/diff/6001/pylib/gyp/generator/eclips... pylib/gyp/generator/eclipse.py:56: default_variables.setdefault('OS', gyp.common.GetFlavor(params)) On 2013/09/25 21:27:08, Nico wrote: > On 2013/09/25 17:05:24, Slava Chigrin wrote: > > This method called fom gyp/__init__.py, so it signature common for many > > generators. Should I change this in all generators? > > Nono, I just mean do: > > flavor = gyp.common.GetFlavor(params) > default_variables.setdefault('OS', flavor) > > instead of calling GetFlavor() twice. > > > > > On 2013/09/24 17:03:48, Nico wrote: > > > pass 'flavor' as last arg here too > > > Done. Sorry, that I did not realized this at once.
Landed in gyp r1739, thanks!
On 2013/09/26 17:43:53, Nico wrote: > Landed in gyp r1739, thanks! Great, thank you! What about my proposition in the first patch? Some lines in CalculateVariables are duplicated in eclipse.py, ninja.py, msvs.py, and dump_dependency_json.py. I can refactor this, for example, by placing them as separate function in msvs_emulation.py. Is it OK? If yes, I'll send patch in the nearest days.
On 2013/09/26 18:00:49, Slava Chigrin wrote: > On 2013/09/26 17:43:53, Nico wrote: > > Landed in gyp r1739, thanks! > > Great, thank you! > > What about my proposition in the first patch? Some lines in CalculateVariables > are duplicated in eclipse.py, ninja.py, msvs.py, and dump_dependency_json.py. I > can refactor this, for example, by placing them as separate function in > msvs_emulation.py. Is it OK? If yes, I'll send patch in the nearest days. I think that would be good. |
