Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9972015:
Create a gyp generator for Eclipse CDT settings (Closed)
|
Created:
8 years, 8 months ago by Jesse Greenwald Modified:
8 years, 7 months ago Reviewers:
Nico CC:
gyp-developer_googlegroups.com Base URL:
http://git.chromium.org/external/gyp.git@master Visibility:
Public. |
DescriptionCreate a gyp generator for Eclipse CDT settings
This generate will generate an XML file that can be import into an
Eclipse CDT project. It will set up all the necesary include paths and
defines.
r1364
Patch Set 1 #
Total comments: 16
Patch Set 2 : Fixed typs, added trailing periods. #Patch Set 3 : Remove hardcoded paths; add support for multiple configurations #Patch Set 4 : Cleanup #
Total comments: 12
Patch Set 5 : Resolving thakis's comments #Patch Set 6 : Added comment about lack of tests #Patch Set 7 : Fixed an abbreviation in a comment; optimized GetAllIncludeDirectories a little more #Messages
Total messages: 9 (0 generated)
thakis - not sure who the best person to review this change is, but it looks like you've doing a few gyp reviews lately. Please feel free to redirect me to someone else for review. Thanks,
Thanks for sending in this change! My main question for now is "What do you want to use this generator for?" https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... File pylib/gyp/generator/eclipse.py (right): https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:63: by any incude dirs that were added as cflag compiler options. typo incude https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:76: cflags = config['cflags'] 'cflags' is linux-specific. It shouldn't be used to add include directories, that's what the cross-OS cross-generator 'include_dirs' is for. If there are concerns about the ordering of directories in include_dir, that should be addressed in another way. https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:84: # Find standard gyp include dirs add trailing . https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:223: WriteMacros(out, eclipse_langs, GetAllDefines(target_list, target_dicts, This looks like it sets all defines from all targets for each target. That's not what other generators do: usually, the 'defines' in one target are only set for that target, unless they are also in 'export_dependent_settings'
Thanks for taking a look at this. The main purpose of this generator is to make development easier when use the Eclipse IDE. Eclipse has a C/C++ plugin called CDT - C/C++ Development Tooling. The artifacts generated by this generator can be imported into a CDT project to tell it where to look for includes and to tell it what symbols are defined. This makes CDT's indexer work much, much better. From testing, it seems that features like auto-complete, click to follow, etc nearly always work. Previously, this was almost never the case (for me). What this generator does not do, is to generate a perfect Eclipse CDT project/workspace that can be used to build a gyp project directly from Eclipse. That would be significantly more complicated. For reference, here is the current Chromium documentation on using Eclipse: http://code.google.com/p/chromium/wiki/LinuxEclipseDev https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... File pylib/gyp/generator/eclipse.py (right): https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:63: by any incude dirs that were added as cflag compiler options. On 2012/04/16 16:12:12, Nico wrote: > typo incude Done. https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:76: cflags = config['cflags'] On 2012/04/16 16:12:12, Nico wrote: > 'cflags' is linux-specific. It shouldn't be used to add include directories, > that's what the cross-OS cross-generator 'include_dirs' is for. If there are > concerns about the ordering of directories in include_dir, that should be > addressed in another way. That makes sense.. but unfortunately this is being done in build/common.gypi: 'cflags': [ '-I<(android_ndk_root)/sources/cxx-stl/stlport/stlport', ], Without this hack in the eclipse generator, the stlport includes won't be picked up. https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:84: # Find standard gyp include dirs On 2012/04/16 16:12:12, Nico wrote: > add trailing . Done. https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:223: WriteMacros(out, eclipse_langs, GetAllDefines(target_list, target_dicts, On 2012/04/16 16:12:12, Nico wrote: > This looks like it sets all defines from all targets for each target. That's not > what other generators do: usually, the 'defines' in one target are only set for > that target, unless they are also in 'export_dependent_settings' The settings.xml file that gets generated isn't perfect, it's more of a close approximation. This is because you can put target specific properties into a CDT settings file. If an entire Eclipse CDT project was being generated, we could do a better job because it allows include paths and symbols to be defined on a per-path basis. The Eclipse CDT project format is fairly complicated, though. The generated settings.xml file that gets generated seems to be a good compromise between complexity and correctness. I've found that nearly everything is resolving correctly in CDT when using one of these generated settings.xml files.
Is it possible to test this generator somehow? For the other generators, we let them generate build files and then call the build systems they generate files for (run `python gyptest.py -a` in gyp's source root to see that in action). https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... File pylib/gyp/generator/eclipse.py (right): https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:10: """ Since you don't intend this to be generally useful, the docstring should probably list that intention, and the specific caveats this generator has. https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:39: os.getcwd() + '/out/Release/obj/gen' If this supports just one configuration, maybe you want to take the configuration as a generator variable instead of hardcoding 'Release' (see the ninja generator as an example) https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:76: cflags = config['cflags'] On 2012/04/16 17:00:55, jgreenwald1 wrote: > On 2012/04/16 16:12:12, Nico wrote: > > 'cflags' is linux-specific. It shouldn't be used to add include directories, > > that's what the cross-OS cross-generator 'include_dirs' is for. If there are > > concerns about the ordering of directories in include_dir, that should be > > addressed in another way. > > That makes sense.. but unfortunately this is being done in build/common.gypi: > > 'cflags': [ > '-I<(android_ndk_root)/sources/cxx-stl/stlport/stlport', > ], > > Without this hack in the eclipse generator, the stlport includes won't be picked > up. build/common.gypi could be changed though, right? https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:213: You might want to print a warning or error if any of --generator-output (controls where generator output such as generated makefiles go), -Goutput_dir (controls where the build output goes), --target_dir (controls the source root location) are set, or are set to unexpected values.
The generator has been updated such that it now generates configs for different gyp configurations (i.e. Release, Debug) For testing, it would be non-trivial (if at all possible) to test that Eclipse would be happy with what's generated. I think the best way to test would be to run the generator and compare the output to known 'golden' settings files. Does that sound reasonable? https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... File pylib/gyp/generator/eclipse.py (right): https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:10: """ On 2012/04/16 17:21:52, Nico wrote: > Since you don't intend this to be generally useful, the docstring should > probably list that intention, and the specific caveats this generator has. Done. https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:39: os.getcwd() + '/out/Release/obj/gen' On 2012/04/16 17:21:52, Nico wrote: > If this supports just one configuration, maybe you want to take the > configuration as a generator variable instead of hardcoding 'Release' (see the > ninja generator as an example) I've updated it to match the ninja generator behavior. https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:76: cflags = config['cflags'] On 2012/04/16 17:21:52, Nico wrote: > On 2012/04/16 17:00:55, jgreenwald1 wrote: > > On 2012/04/16 16:12:12, Nico wrote: > > > 'cflags' is linux-specific. It shouldn't be used to add include directories, > > > that's what the cross-OS cross-generator 'include_dirs' is for. If there are > > > concerns about the ordering of directories in include_dir, that should be > > > addressed in another way. > > > > That makes sense.. but unfortunately this is being done in build/common.gypi: > > > > 'cflags': [ > > '-I<(android_ndk_root)/sources/cxx-stl/stlport/stlport', > > ], > > > > Without this hack in the eclipse generator, the stlport includes won't be > picked > > up. > > build/common.gypi could be changed though, right? It could probably be fixed at some point, although I'm not sure how this should be done. https://chromiumcodereview.appspot.com/9972015/diff/1/pylib/gyp/generator/ecl... pylib/gyp/generator/eclipse.py:213: On 2012/04/16 17:21:52, Nico wrote: > You might want to print a warning or error if any of --generator-output > (controls where generator output such as generated makefiles go), -Goutput_dir > (controls where the build output goes), --target_dir (controls the source root > location) are set, or are set to unexpected values. Done.
This looks pretty good, a few more questions below.
I think golden files don't make for good tests. When something is changed, the
golden files have to be updated often because the results changed in a valid way
(which is a pain), and since updating golden files after a normal change is
somewhat common, people often do that and miss bugs they introduce that way.
(Examples: webkit pixel expectation files, or the few golden files we use in
gyp.) I think I prefer not having tests over golden files. (I'd prefer actually
running the gyp tests even more, but if that's not possible it's not possible.)
If this ends up not having tests, please call that out in the docstring at the
top ("This generator has no automated tests, so expect it to be broken.") and
make sure people other than you don't start depending on it.
https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/...
File pylib/gyp/generator/eclipse.py (right):
https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/...
pylib/gyp/generator/eclipse.py:63: def GetAllIncludeDirs(target_list,
target_dicts, shared_intermediates_dir,
nit: style guide says no abbreviations. Use GetAllIncludeDirectories, also
s/dir/directory/ in the docstring. (I think it's fine for the variable names.)
https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/...
pylib/gyp/generator/eclipse.py:81: # may be done in gyp files to force certain
includes to come at the end.
Can you at least put a "# TODO: Change the gyp files to not abuse cflags for
this, and remove this." here?
https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/...
pylib/gyp/generator/eclipse.py:108: all_includes = list(gyp_includes)
s/list/set/, else the loop below is O(n^2). (Also, isn't gyp_includes a list
already?)
Maybe you can write this as:
all_includes = gyp_includes
all_includes.extend(list(set(compiler_includes) - set(all_includes)))
? (That might reorder compiler_includes, but from what I understand that list is
usually 1 element long anyway?)
https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/...
pylib/gyp/generator/eclipse.py:133: # Check to see if the compiler was specified
as an environment variable.
I think ninja allows 'CC' and 'CXX' as env keys too. (just fyi, if you don't
want to support that, that's fine)
https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/...
pylib/gyp/generator/eclipse.py:138: # Default.
Nit: Comment doesn't add much, remove it.
https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/...
pylib/gyp/generator/eclipse.py:167: # Get default compiler defines (if
possible).
I'm surprised this part is needed. Shouldn't Eclipse know about
compiler-specific defines itself?
Thanks for the changes!
I would mind writing some kind of test. Perhaps the test could spot check the generated XML file to make sure expected values are there? Otherwise, I'm not really sure how to verify the output. Other than that, I think I've addressed your comments. Thanks for taking the time to look at this. https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/... File pylib/gyp/generator/eclipse.py (right): https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/... pylib/gyp/generator/eclipse.py:63: def GetAllIncludeDirs(target_list, target_dicts, shared_intermediates_dir, On 2012/04/27 17:33:41, Nico wrote: > nit: style guide says no abbreviations. Use GetAllIncludeDirectories, also > s/dir/directory/ in the docstring. (I think it's fine for the variable names.) Done. https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/... pylib/gyp/generator/eclipse.py:81: # may be done in gyp files to force certain includes to come at the end. On 2012/04/27 17:33:41, Nico wrote: > Can you at least put a "# TODO: Change the gyp files to not abuse cflags for > this, and remove this." here? Done. https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/... pylib/gyp/generator/eclipse.py:108: all_includes = list(gyp_includes) On 2012/04/27 17:33:41, Nico wrote: > s/list/set/, else the loop below is O(n^2). (Also, isn't gyp_includes a list > already?) > > Maybe you can write this as: > > all_includes = gyp_includes > all_includes.extend(list(set(compiler_includes) - set(all_includes))) > > ? (That might reorder compiler_includes, but from what I understand that list is > usually 1 element long anyway?) To keep the ordering, I copied gyp_includes into a set and just check to make sure each compiler include isn't in there before appending it to the 'all' list. https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/... pylib/gyp/generator/eclipse.py:133: # Check to see if the compiler was specified as an environment variable. On 2012/04/27 17:33:41, Nico wrote: > I think ninja allows 'CC' and 'CXX' as env keys too. (just fyi, if you don't > want to support that, that's fine) Done. https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/... pylib/gyp/generator/eclipse.py:138: # Default. On 2012/04/27 17:33:41, Nico wrote: > Nit: Comment doesn't add much, remove it. Done. https://chromiumcodereview.appspot.com/9972015/diff/9001/pylib/gyp/generator/... pylib/gyp/generator/eclipse.py:167: # Get default compiler defines (if possible). On 2012/04/27 17:33:41, Nico wrote: > I'm surprised this part is needed. Shouldn't Eclipse know about > compiler-specific defines itself? Eclipse doesn't know what compiler is being used. In the case of Android, the NDK cross compiler is getting used with a different set of default includes/defines. This is to handle that case.
LGTM |
