[GN] Export include directories, defines and dialects to Xcode projects

[GN] Export include directories, defines and dialects to Xcode projects

Without properly specified include directories the Xcode indexer will choke
and give incomplete result.

Include directories and defines that apply to entire project are specified
on indexer project level. Directories and defines specific to individual
targets are set on file level. This together with properly specified dialect
should significantly improve chances of indexer getting through all files.

This patch also disables HEADERMAP which is not necessary with properly
specified directories and actually confuses the indexer when there are
files with same name in the project.

BUG=618863
R=sdefresne@chromium.org

[matt.k, 2016-06-10 07:35:11]

https://codereview.chromium.org/2057873002/diff/1/tools/gn/xcode_object.cc
File tools/gn/xcode_object.cc (right):

https://codereview.chromium.org/2057873002/diff/1/tools/gn/xcode_object.cc#ne...
tools/gn/xcode_object.cc:239: // split and print as vector
Splitting the string to get multiple values is a hack, I'll fix this properly.

[sdefresne, 2016-06-10 08:52:34]

Thank you for the proof-of-concept.

As I said, I think that using a std::multimap<> would allow removing the hack of
splitting the string on newlines.

I also see that your code is not following the style guide. Can you use "git cl
format" before uploading your next patch?

https://codereview.chromium.org/2057873002/diff/1/tools/gn/xcode_object.cc
File tools/gn/xcode_object.cc (right):

https://codereview.chromium.org/2057873002/diff/1/tools/gn/xcode_object.cc#ne...
tools/gn/xcode_object.cc:863: std::map<std::string, BuildSettingsValue>
build_settings;
I would use a multimap instead of splitting using the BuildSettingsValue hack.

[matt.k, 2016-06-10 11:43:53]

Description was changed from

==========
[GN] Export include directories, defines and dialects to Xcode projects

Without properly specified include directories the Xcode indexer will choke
and give incomplete result.

Include directories and defines that apply to entire project are specified
on indexer project level. Directories and defines specific to individual
targets are set on file level. This together with propery specified dialect
should significantly improve chances of indexer getting through all files.

This patch also disables HEADERMAP which is not necessary with properly
specified directories and actually confuses the indexer when there are
files with same name in the project.

BUG=618863
R=sdefresne@chromium.org
==========

to

==========
[GN] Export include directories, defines and dialects to Xcode projects

Without properly specified include directories the Xcode indexer will choke
and give incomplete result.

Include directories and defines that apply to entire project are specified
on indexer project level. Directories and defines specific to individual
targets are set on file level. This together with properly specified dialect
should significantly improve chances of indexer getting through all files.

This patch also disables HEADERMAP which is not necessary with properly
specified directories and actually confuses the indexer when there are
files with same name in the project.

BUG=618863
R=sdefresne@chromium.org
==========

[matt.k, 2016-06-10 11:44:17]

The CQ bit was checked by matej.knopp@gmail.com

[matt.k, 2016-06-10 11:44:18]

The CQ bit was unchecked by matej.knopp@gmail.com

[matt.k, 2016-06-11 02:38:32]

Okay, so here are few performance related observations:

1) Specifying defines on file level is not a good idea, because indexer can't
use precompiled headers (include dirs on file level are fine though,
fortunately); So I think having global defines (same for all targets, i.e.
_DEBUG should be good enough, defines specific to target (which are set on file
level) are just not worth it as they slow indexing down too much

2) Xcode project needs precompiled prefix header set. This makes indexing
significantly faster. The question is - what do we set here? The prefix header
can't contain any includes from project source tree (it breaks indexer) but it
should contain as many outside/system includes as possible. I see two options
here - either somehow let user specify file that will be set as Prefix in xcode
project, or try to merge prefix headers from all targets removing (or commenting
out) lines that contain project-relative includes; Not sure how reliable this
would be, but it's something to consider.

Either way, removing -Ds from individual files and specifying a prefix header
has cut indexing time of my test project from 1:20 to 35s, which is quite
significant.

[matt.k, 2016-06-11 19:35:31]

I was wondering if something like this would fly

declare_args() {
  xcode_precompiled_header = "//path/to/prefix.h"
}

This would make it fairly simple in the generator to fill in project prefix
header property and the value would be specified in project files instead of
generator command line arguments, where I don't think it belongs.

[sdefresne, 2016-06-13 07:36:25]

On 2016/06/11 19:35:31, matej.knopp wrote:
> I was wondering if something like this would fly
> 
> declare_args() {
>   xcode_precompiled_header = "//path/to/prefix.h"
> }
> 
> This would make it fairly simple in the generator to fill in project prefix
> header property and the value would be specified in project files instead of
> generator command line arguments, where I don't think it belongs.

I think this will not work as there can be more than one precompiled header per
project.

When building Chromium (the Xcode generator was developed primarily to work with
Chromium), many of the third party dependencies (and there are a lot) can (and
do) use a separate precompiled header (see "gn help precompiled_header" and
you'll notice that the precompiled header is configured per source_set, not
globally). However, since in Chromium, all file from the project are included
using path relative to repository root, so the currently generated Xcode project
enable IntelliSense for the majority of the source files (i.e. all files from
main Chromium repository).

One possible alternative that should work for Chromium and other projects (and
will improve how Chromium works) is to collect all include_dirs from all
targets, and use that to configure the HEADER_SEARCH_PATHS.

The current goal of the generator was to create project that allow to browse the
source, build and debug from Xcode. Having better indexing would be good but I'm
afraid we won't be able to achieve perfection without generating real Xcode
project (i.e. project that build with Xcode and not ninja) which is explicitly a
non-goal for gn.

[matt.k, 2016-06-13 08:31:52]

> I think this will not work as there can be more than one precompiled header
per
> project.

I'm well aware of that. But given that you combine source code from all targets
to
single xcode target, there needs to be one precompiled header. It doesn't have
to
be perfect, but it should be good enough for most projects and will still
significantly 
improve indexing time.
 
> One possible alternative that should work for Chromium and other projects (and
> will improve how Chromium works) is to collect all include_dirs from all
> targets, and use that to configure the HEADER_SEARCH_PATHS.

How would this be better that what I'm doing right now? With my patch 
HEADER_SEARCH_PATHS contains includes that are same for all projects, and then
on individual files I set -I<dir> flags for includes that are specific to gn
target where
that particular source belongs to. This results in Xcode indexer knowing exactly
where it should look for includes for each file.

But this is separate issue from Xcode project precompiled header, which in this
case 
is purely to improve indexing speed.

> 
> The current goal of the generator was to create project that allow to browse
the
> source, build and debug from Xcode. Having better indexing would be good but
I'm
> afraid we won't be able to achieve perfection without generating real Xcode
> project (i.e. project that build with Xcode and not ninja) which is explicitly
a
> non-goal for gn.

I think the accuracy will be greatly improved with setting HEADER_SEARCH_PATHS 
and -I<dir>s on individual files. The only unresolved issue here is what value
to set 
as GCC_PREFIX_HEADER on xcode source target. In fact for chromium the perfect 
value would probably be "build/precompile.h", since it's generic enough for most

projects, but the generator has no way of knowing that.

[sdefresne, 2016-06-13 12:16:25]

On 2016/06/13 08:31:52, matej.knopp wrote:
> > I think this will not work as there can be more than one precompiled header
> per
> > project.
> 
> I'm well aware of that. But given that you combine source code from all
targets
> to
> single xcode target, there needs to be one precompiled header. It doesn't have
> to
> be perfect, but it should be good enough for most projects and will still
> significantly 
> improve indexing time.

Since the file will not be used as part of the build, it will quickly bit rot
(as none of the developer on other platform will keep it up-to-date, and
probably most of the developer on Mac/iOS won't either). This will make this
file mostly useless.

On the other hand, include_dirs need to be kept up-to-date as otherwise the
compilation will fail on the bots.

> > One possible alternative that should work for Chromium and other projects
(and
> > will improve how Chromium works) is to collect all include_dirs from all
> > targets, and use that to configure the HEADER_SEARCH_PATHS.
> 
> How would this be better that what I'm doing right now? With my patch 
> HEADER_SEARCH_PATHS contains includes that are same for all projects, and then
> on individual files I set -I<dir> flags for includes that are specific to gn
> target where
> that particular source belongs to. This results in Xcode indexer knowing
exactly
> where it should look for includes for each file.
> 
> But this is separate issue from Xcode project precompiled header, which in
this
> case 
> is purely to improve indexing speed.
> 
> > 
> > The current goal of the generator was to create project that allow to browse
> the
> > source, build and debug from Xcode. Having better indexing would be good but
> I'm
> > afraid we won't be able to achieve perfection without generating real Xcode
> > project (i.e. project that build with Xcode and not ninja) which is
explicitly
> a
> > non-goal for gn.
> 
> I think the accuracy will be greatly improved with setting HEADER_SEARCH_PATHS

> and -I<dir>s on individual files. The only unresolved issue here is what value
> to set 
> as GCC_PREFIX_HEADER on xcode source target. In fact for chromium the perfect 
> value would probably be "build/precompile.h", since it's generic enough for
most
> 
> projects, but the generator has no way of knowing that.

The current Xcode project generator in gn gives similar results to the
corresponding xcode-ninja generator in gyp. It has been acceptable in term of
performances and accuracy for Chromium developments since it was introduced, so
I'm unconvinced by that we need to set this variables. But if we need to set it,
then I think we'll have to set it per file too, as the precompiled_header is set
per source_set/static_library/executable/... (and usually a source file is in a
single source_set/static_library/executable/...).

[matt.k, 2016-06-13 13:42:58]

On 2016/06/13 12:16:25, sdefresne wrote:
> On 2016/06/13 08:31:52, matej.knopp wrote:
> > > I think this will not work as there can be more than one precompiled
header
> > per
> > > project.
> > 
> > I'm well aware of that. But given that you combine source code from all
> targets
> > to
> > single xcode target, there needs to be one precompiled header. It doesn't
have
> > to
> > be perfect, but it should be good enough for most projects and will still
> > significantly 
> > improve indexing time.
> 
> Since the file will not be used as part of the build, it will quickly bit rot
> (as none of the developer on other platform will keep it up-to-date, and
> probably most of the developer on Mac/iOS won't either). This will make this
> file mostly useless.
> 
> On the other hand, include_dirs need to be kept up-to-date as otherwise the
> compilation will fail on the bots.
> 
> > > One possible alternative that should work for Chromium and other projects
> (and
> > > will improve how Chromium works) is to collect all include_dirs from all
> > > targets, and use that to configure the HEADER_SEARCH_PATHS.
> > 
> > How would this be better that what I'm doing right now? With my patch 
> > HEADER_SEARCH_PATHS contains includes that are same for all projects, and
then
> > on individual files I set -I<dir> flags for includes that are specific to gn
> > target where
> > that particular source belongs to. This results in Xcode indexer knowing
> exactly
> > where it should look for includes for each file.
> > 
> > But this is separate issue from Xcode project precompiled header, which in
> this
> > case 
> > is purely to improve indexing speed.
> > 
> > > 
> > > The current goal of the generator was to create project that allow to
browse
> > the
> > > source, build and debug from Xcode. Having better indexing would be good
but
> > I'm
> > > afraid we won't be able to achieve perfection without generating real
Xcode
> > > project (i.e. project that build with Xcode and not ninja) which is
> explicitly
> > a
> > > non-goal for gn.
> > 
> > I think the accuracy will be greatly improved with setting
HEADER_SEARCH_PATHS
> 
> > and -I<dir>s on individual files. The only unresolved issue here is what
value
> > to set 
> > as GCC_PREFIX_HEADER on xcode source target. In fact for chromium the
perfect 
> > value would probably be "build/precompile.h", since it's generic enough for
> most
> > 
> > projects, but the generator has no way of knowing that.
> 
> The current Xcode project generator in gn gives similar results to the
> corresponding xcode-ninja generator in gyp. It has been acceptable in term of
> performances and accuracy for Chromium developments since it was introduced,
so
> I'm unconvinced by that we need to set this variables. But if we need to set
it,
> then I think we'll have to set it per file too, as the precompiled_header is
set
> per source_set/static_library/executable/... (and usually a source file is in
a
> single source_set/static_library/executable/...).

I tried it, it's not possible to set precompiled header per file on xcode
project. You can add the compile flag to file flags that will specify
precompiled header, but the indexer will just ignore it. It seems to only take
GCC_PREFIX_HEADER into account.

Honestly, I really don't feel like pushing this. If you are happy with the
indexing performance so be it (even though as I mentioned specifying (any
reasonable) prefix header did cut indexing time by 50% in my case. I understand
perfectly that if you're working on Chromium you have likely completely
different priorities for the Xcode project than I do so I'd much rather focus on
something like this getting in https://codereview.chromium.org/2064533002

[sdefresne, 2016-06-14 12:10:55]

On 2016/06/13 13:42:58, matej.knopp wrote:
> On 2016/06/13 12:16:25, sdefresne wrote:
> > On 2016/06/13 08:31:52, matej.knopp wrote:
> > > > I think this will not work as there can be more than one precompiled
> header
> > > per
> > > > project.
> > > 
> > > I'm well aware of that. But given that you combine source code from all
> > targets
> > > to
> > > single xcode target, there needs to be one precompiled header. It doesn't
> have
> > > to
> > > be perfect, but it should be good enough for most projects and will still
> > > significantly 
> > > improve indexing time.
> > 
> > Since the file will not be used as part of the build, it will quickly bit
rot
> > (as none of the developer on other platform will keep it up-to-date, and
> > probably most of the developer on Mac/iOS won't either). This will make this
> > file mostly useless.
> > 
> > On the other hand, include_dirs need to be kept up-to-date as otherwise the
> > compilation will fail on the bots.
> > 
> > > > One possible alternative that should work for Chromium and other
projects
> > (and
> > > > will improve how Chromium works) is to collect all include_dirs from all
> > > > targets, and use that to configure the HEADER_SEARCH_PATHS.
> > > 
> > > How would this be better that what I'm doing right now? With my patch 
> > > HEADER_SEARCH_PATHS contains includes that are same for all projects, and
> then
> > > on individual files I set -I<dir> flags for includes that are specific to
gn
> > > target where
> > > that particular source belongs to. This results in Xcode indexer knowing
> > exactly
> > > where it should look for includes for each file.
> > > 
> > > But this is separate issue from Xcode project precompiled header, which in
> > this
> > > case 
> > > is purely to improve indexing speed.
> > > 
> > > > 
> > > > The current goal of the generator was to create project that allow to
> browse
> > > the
> > > > source, build and debug from Xcode. Having better indexing would be good
> but
> > > I'm
> > > > afraid we won't be able to achieve perfection without generating real
> Xcode
> > > > project (i.e. project that build with Xcode and not ninja) which is
> > explicitly
> > > a
> > > > non-goal for gn.
> > > 
> > > I think the accuracy will be greatly improved with setting
> HEADER_SEARCH_PATHS
> > 
> > > and -I<dir>s on individual files. The only unresolved issue here is what
> value
> > > to set 
> > > as GCC_PREFIX_HEADER on xcode source target. In fact for chromium the
> perfect 
> > > value would probably be "build/precompile.h", since it's generic enough
for
> > most
> > > 
> > > projects, but the generator has no way of knowing that.
> > 
> > The current Xcode project generator in gn gives similar results to the
> > corresponding xcode-ninja generator in gyp. It has been acceptable in term
of
> > performances and accuracy for Chromium developments since it was introduced,
> so
> > I'm unconvinced by that we need to set this variables. But if we need to set
> it,
> > then I think we'll have to set it per file too, as the precompiled_header is
> set
> > per source_set/static_library/executable/... (and usually a source file is
in
> a
> > single source_set/static_library/executable/...).
> 
> I tried it, it's not possible to set precompiled header per file on xcode
> project. You can add the compile flag to file flags that will specify
> precompiled header, but the indexer will just ignore it. It seems to only take
> GCC_PREFIX_HEADER into account.
> 
> Honestly, I really don't feel like pushing this. If you are happy with the
> indexing performance so be it (even though as I mentioned specifying (any
> reasonable) prefix header did cut indexing time by 50% in my case. I
understand
> perfectly that if you're working on Chromium you have likely completely
> different priorities for the Xcode project than I do so I'd much rather focus
on
> something like this getting in https://codereview.chromium.org/2064533002

As you said, some of the changes are a net benefit, so can you split your CL in
multiple CL, each of them doing an independent changes.

I can see at least three or four different CLs here:
- disable HEADERMAP
- better HEADER_SEARCH_PATHS
- correctly settings cflags (or at least -D)
- setting GCC_PREFIX_HEADER

This is only the fourth one that I'm unsure we can do correctly, but maybe we
can add a --extra_xcode_settings that takes a list of key=value pairs for
developer that want to maintain a precompiled header for their local
developement.

WDYT?

[matt.k, 2016-06-14 12:30:48]

> 
> As you said, some of the changes are a net benefit, so can you split your CL
in
> multiple CL, each of them doing an independent changes.
> 
> I can see at least three or four different CLs here:
> - disable HEADERMAP
> - better HEADER_SEARCH_PATHS
> - correctly settings cflags (or at least -D)
> - setting GCC_PREFIX_HEADER
> 
> This is only the fourth one that I'm unsure we can do correctly, but maybe we
> can add a --extra_xcode_settings that takes a list of key=value pairs for
> developer that want to maintain a precompiled header for their local
> developement.
> 
> WDYT?

Actually, this changeset doesn't try to do anything about GCC_PREFIX_HEADER. I
wasn't sure how to approach this so I didn't upload anything yet. So it only
covers 1-3.

As for the cflags, what the changeset currenty does is that global defines (same
for every target) are set on Xcode project level. Target specific defines are
ignored. It is possible to set target specific defines in individual file
compiler settings, but that severely impacts indexing speed (apparently
precompiled headers are not used for such files during indexing even if enabled
in project settings).

Other than that, on file level this sets -I<dir> and -std, which don't seem to
affect indexing speed.

All these changes are somewhat related, i.e. changing build settings to multimap
is needed for both headers and defines, I'm not sure about splitting the CS.

[sdefresne, 2016-06-14 18:05:01]

On 2016/06/14 12:30:48, matej.knopp wrote:
> > 
> > As you said, some of the changes are a net benefit, so can you split your CL
> in
> > multiple CL, each of them doing an independent changes.
> > 
> > I can see at least three or four different CLs here:
> > - disable HEADERMAP
> > - better HEADER_SEARCH_PATHS
> > - correctly settings cflags (or at least -D)
> > - setting GCC_PREFIX_HEADER
> > 
> > This is only the fourth one that I'm unsure we can do correctly, but maybe
we
> > can add a --extra_xcode_settings that takes a list of key=value pairs for
> > developer that want to maintain a precompiled header for their local
> > developement.
> > 
> > WDYT?
> 
> Actually, this changeset doesn't try to do anything about GCC_PREFIX_HEADER. I
> wasn't sure how to approach this so I didn't upload anything yet. So it only
> covers 1-3.
> 
> As for the cflags, what the changeset currenty does is that global defines
(same
> for every target) are set on Xcode project level. Target specific defines are
> ignored. It is possible to set target specific defines in individual file
> compiler settings, but that severely impacts indexing speed (apparently
> precompiled headers are not used for such files during indexing even if
enabled
> in project settings).
> 
> Other than that, on file level this sets -I<dir> and -std, which don't seem to
> affect indexing speed.
> 
> All these changes are somewhat related, i.e. changing build settings to
multimap
> is needed for both headers and defines, I'm not sure about splitting the CS.

Regarding GCC_PREFIX_HEADER, brettw suggested in
https://codereview.chromium.org/2063243002/ a way to pass arbitrary arguments
from BUILD.gn files to the IDE generator. Would this work for you?

[matt.k, 2016-06-14 19:24:14]

> Regarding GCC_PREFIX_HEADER, brettw suggested in
> https://codereview.chromium.org/2063243002/ a way to pass arbitrary arguments
> from BUILD.gn files to the IDE generator. Would this work for you?

Yes, this would definitely help.

[matt.k, 2016-07-06 08:46:57]

Slightly off-topic, I made a proof of concept JSON -> XCode project generator,
which generates separate indexing target for each GN target, so all includes,
defines, precompiled headers etc can be set properly for every indexed file

https://github.com/knopp/gn-project-generators

i.e. this would create project file for GN (and its dependencies) 
gn gen --ide=json --exec-script=<path-to>/gen_xcode.py --filters="//tools/gn:gn"
\
build-xcode

It seems to work quite well, but several thousands of targets when not using any
filters in single project might be bit much for Xcode.

It relies on JSON project generator https://codereview.chromium.org/2064533002/

On 2016/06/14 19:24:14, matt.k wrote:
> > Regarding GCC_PREFIX_HEADER, brettw suggested in
> > https://codereview.chromium.org/2063243002/ a way to pass arbitrary
arguments
> > from BUILD.gn files to the IDE generator. Would this work for you?
> 
> Yes, this would definitely help.

[sdefresne, 2017-05-03 09:07:35]

sdefresne@chromium.org changed reviewers:
- sdefresne@chromium.org