Fixed Gyp Xcode generator for libraries with identical names.

Fixed Gyp Xcode generator for libraries with identical names.

The Xcode generator produces the same hash for library target products that have
identical names and are defined in .gyp files with identical names that have
different source tree roots. The fix consists of setting the (already defined)
'projectRoot' property on the PBXProject object and adding it to the hashables
of this class.

Patch by Johannes Sartisohn <jsartisohn@google.com>;

R=mark@chromium.org

Committed: https://code.google.com/p/gyp/source/detail?r=2011

[jsartisohn, 2014-11-21 13:03:55]

jsartisohn@google.com changed reviewers:
+ mark@chromium.org, scottmg@chromium.org

[jsartisohn, 2014-11-21 13:06:24]

+adaubert

[jsartisohn, 2014-11-25 12:28:05]

jsartisohn@google.com changed reviewers:
+ thakis@chromium.org

[jsartisohn, 2014-11-25 12:28:05]

+Nico
and friendly ping

[M-A Ruel, 2014-11-25 13:46:16]

Is that a good idea at all? Having two libraries with the same name is a real
long term maintenance problem so there *is* benefit in disallowing this.  My 2¢.

[jsartisohn, 2014-11-25 14:27:39]

On 2014/11/25 13:46:16, M-A Ruel wrote:
> Is that a good idea at all? Having two libraries with the same name is a real
> long term maintenance problem so there *is* benefit in disallowing this.  My
2¢.

I understand your concern, and that's why I tried not do change the default
behavior of the Xcode generator.
For us the issue is, that we have quite a large codebase covering multiple
projects and platforms, of which not necessarily every project pulls in all
dependencies. When adding support for a new platform or onboarding new projects,
it's virtually impossible to avoid name collisions for some base libraries, say
for example "common". Also all our other build systems support same name libs if
they are in different subdirectories. The MSVS gyp generator for example has no
problems building these .gyp files and MSVS itself has no problems
compiling/linking the produced projects.

Bottom line:
We would be able to cope with same name libs, by renaming them, but since this
is a change that adds a feature and doesn't change default behavior, we'd be
happy if it gets some consideration.

[Nico, 2014-11-25 15:12:13]

Mark owns the Xcode generator, it's up to him if he wants to allow this.

I (weakly) think that this makes things more confusing, not just for devs who
now have two libcommon.a's to deal with, but also for code: The ninja generator
for example puts all libraries into its root build folder (out/Release), so if
you have two libcommon.a's, you need to fix their names if you want to use the
ninja generator.

For Chromium, we also rely on dependencies with dependencies with dependencies.
We do run into the problem you describe every now and then, but so far we always
were able to work with upstreams to rename their target (or, if that didn't
work, write our own gyp file for their thing, with a different product_name set
for the problematic targets.)

[Mark Mentovai, 2014-11-25 15:59:31]

On one hand, I’m not very enthusiastic about this. It doesn’t help Chrome out at
all, and it gives the Xcode generator a capability that the other generators
don’t have. As Nico points out, Chrome puts all build products into a single
directory, so tolerating this at the Xcode project level could just lead to
collisions down the line when two targets wind up trying to write products at
the same path.

On the other hand, if this makes it easier for projects that are Xcode-only to
use GYP as their xcodeproj-generating front end, I don’t really want to stand in
the way.

https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py
File pylib/gyp/generator/xcode.py (right):

https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
pylib/gyp/generator/xcode.py:118: # Only avoid add projectRoot to avoid hash
collision, if both SYMROOT
I don’t really understand this comment.

https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
pylib/gyp/generator/xcode.py:120: if 'SYMROOT' in xcs and
'CONFIGURATION_BUILD_DIR' in xcs:
I also don’t know how or why you’ve chosen this condition to dictate the
behavior.

https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
pylib/gyp/generator/xcode.py:121: self.project.SetProperty('projectRoot',
os.path.dirname(path))
What is path here? Absolute or relative?

https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
pylib/gyp/generator/xcode.py:121: self.project.SetProperty('projectRoot',
os.path.dirname(path))
Is projectRoot a real thing that Xcode ever sets? If not, I don’t think we
should leave it someplace that it’ll show up in the output.

https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
pylib/gyp/generator/xcode.py:126: 'CONFIGURATION_BUILD_DIR' defined in %s.
Either define both or none" %
Where does this requirement come from?

https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
pylib/gyp/generator/xcode.py:128: pass
Not needed since you already have the (questionable) sys.stderr.write.

[jsartisohn, 2014-11-27 15:04:42]

On 2014/11/25 15:59:31, Mark Mentovai wrote:
> On one hand, I’m not very enthusiastic about this. It doesn’t help Chrome out
at
> all, and it gives the Xcode generator a capability that the other generators
> don’t have. As Nico points out, Chrome puts all build products into a single
> directory, so tolerating this at the Xcode project level could just lead to
> collisions down the line when two targets wind up trying to write products at
> the same path.
> 
> On the other hand, if this makes it easier for projects that are Xcode-only to
> use GYP as their xcodeproj-generating front end, I don’t really want to stand
in
> the way.
> 
> https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py
> File pylib/gyp/generator/xcode.py (right):
> 
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> pylib/gyp/generator/xcode.py:118: # Only avoid add projectRoot to avoid hash
> collision, if both SYMROOT
> I don’t really understand this comment.
> 
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> pylib/gyp/generator/xcode.py:120: if 'SYMROOT' in xcs and
> 'CONFIGURATION_BUILD_DIR' in xcs:
> I also don’t know how or why you’ve chosen this condition to dictate the
> behavior.
> 
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> pylib/gyp/generator/xcode.py:121: self.project.SetProperty('projectRoot',
> os.path.dirname(path))
> What is path here? Absolute or relative?
> 
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> pylib/gyp/generator/xcode.py:121: self.project.SetProperty('projectRoot',
> os.path.dirname(path))
> Is projectRoot a real thing that Xcode ever sets? If not, I don’t think we
> should leave it someplace that it’ll show up in the output.
> 
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> pylib/gyp/generator/xcode.py:126: 'CONFIGURATION_BUILD_DIR' defined in %s.
> Either define both or none" %
> Where does this requirement come from?
> 
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> pylib/gyp/generator/xcode.py:128: pass
> Not needed since you already have the (questionable) sys.stderr.write.

After considering all your comments and realizing, that I did way too many
assumptions, I reworked the CL quite a bit:

First of all I removed every occurrence of BUILT_PRODUCTS_DIR, because it is
affected by SYMROOT anyways, see
https://developer.apple.com/library/mac/documentation/DeveloperTools/Referenc...
It was a mere reamnant of our project setup, that I didn't remove because I
didn't thing about it before submitting this CL.
Secondly, I tried to avoid using SYMROOT (that is, overriding its default value)
for setting the .a file output directory. I tried using 'product_dir' which
didn't yield any workable solution. So I still have to override SYMROOT. One
change I made though, is to pull the SYMROOT attribute up from the project level
into the target level, by putting it into the 'target_defaults' dict (in
test/mac/identical-name/test.gypi).

The main change though, is how and which _hashables are updated. Instead of
updating the hashables of PBXProject I now extend the hashables of a given
PBXReferenceProxy if any of its targets has the 'SYMROOT' attribute set AND if
this 'SYMROOT' attribute contains a form of the '$SRCROOT' Xcode macro. Putting
this macro in the SYMROOT attribute ensures, that the target products will be
put into a different folder than products for any other target and thus
allowing/forcing us to give those reference a different hash.
That way the hashes only differ, if the build products actually land in
different directories.

I hope this is somehow clear.

Now to address the question if this change is actually reasonable and to argue
that in my opinion it is:
For me this change doesn't really add a feature to the generator, it rather
fixes a bug that the other generators I use, don't have. Again, the MSVS
generator supports this feature when overriding the
'msvs_configuration_attributes': { 'OutputDirectory': '...' } value. And the
internal BUILD file generator also supports same name libs that are in different
directories.
Also I think now, that the hashable is directly tied to the uniqueness of the
output directory, there should be no drawback for Chromium or the average GYP
based project, as overriding 'SYMROOT' is something that you should only do if
you know what you're doing.

Thanks for the reviews and the patience.
Have nice holidays.

[jsartisohn, 2014-11-27 17:06:09]

On 2014/11/27 15:04:42, jsartisohn wrote:
> On 2014/11/25 15:59:31, Mark Mentovai wrote:
> > On one hand, I’m not very enthusiastic about this. It doesn’t help Chrome
out
> at
> > all, and it gives the Xcode generator a capability that the other generators
> > don’t have. As Nico points out, Chrome puts all build products into a single
> > directory, so tolerating this at the Xcode project level could just lead to
> > collisions down the line when two targets wind up trying to write products
at
> > the same path.
> > 
> > On the other hand, if this makes it easier for projects that are Xcode-only
to
> > use GYP as their xcodeproj-generating front end, I don’t really want to
stand
> in
> > the way.
> > 
> >
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py
> > File pylib/gyp/generator/xcode.py (right):
> > 
> >
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> > pylib/gyp/generator/xcode.py:118: # Only avoid add projectRoot to avoid hash
> > collision, if both SYMROOT
> > I don’t really understand this comment.
> > 
> >
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> > pylib/gyp/generator/xcode.py:120: if 'SYMROOT' in xcs and
> > 'CONFIGURATION_BUILD_DIR' in xcs:
> > I also don’t know how or why you’ve chosen this condition to dictate the
> > behavior.
> > 
> >
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> > pylib/gyp/generator/xcode.py:121: self.project.SetProperty('projectRoot',
> > os.path.dirname(path))
> > What is path here? Absolute or relative?
> > 
> >
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> > pylib/gyp/generator/xcode.py:121: self.project.SetProperty('projectRoot',
> > os.path.dirname(path))
> > Is projectRoot a real thing that Xcode ever sets? If not, I don’t think we
> > should leave it someplace that it’ll show up in the output.
> > 
> >
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> > pylib/gyp/generator/xcode.py:126: 'CONFIGURATION_BUILD_DIR' defined in %s.
> > Either define both or none" %
> > Where does this requirement come from?
> > 
> >
>
https://codereview.chromium.org/748793002/diff/1/pylib/gyp/generator/xcode.py...
> > pylib/gyp/generator/xcode.py:128: pass
> > Not needed since you already have the (questionable) sys.stderr.write.
> 
> After considering all your comments and realizing, that I did way too many
> assumptions, I reworked the CL quite a bit:
> 
> First of all I removed every occurrence of BUILT_PRODUCTS_DIR, because it is
> affected by SYMROOT anyways, see
>
https://developer.apple.com/library/mac/documentation/DeveloperTools/Referenc...
> It was a mere reamnant of our project setup, that I didn't remove because I
> didn't thing about it before submitting this CL.
> Secondly, I tried to avoid using SYMROOT (that is, overriding its default
value)
> for setting the .a file output directory. I tried using 'product_dir' which
> didn't yield any workable solution. So I still have to override SYMROOT. One
> change I made though, is to pull the SYMROOT attribute up from the project
level
> into the target level, by putting it into the 'target_defaults' dict (in
> test/mac/identical-name/test.gypi).
> 
> The main change though, is how and which _hashables are updated. Instead of
> updating the hashables of PBXProject I now extend the hashables of a given
> PBXReferenceProxy if any of its targets has the 'SYMROOT' attribute set AND if
> this 'SYMROOT' attribute contains a form of the '$SRCROOT' Xcode macro.
Putting
> this macro in the SYMROOT attribute ensures, that the target products will be
> put into a different folder than products for any other target and thus
> allowing/forcing us to give those reference a different hash.
> That way the hashes only differ, if the build products actually land in
> different directories.
> 
> I hope this is somehow clear.
> 
> Now to address the question if this change is actually reasonable and to argue
> that in my opinion it is:
> For me this change doesn't really add a feature to the generator, it rather
> fixes a bug that the other generators I use, don't have. Again, the MSVS
> generator supports this feature when overriding the
> 'msvs_configuration_attributes': { 'OutputDirectory': '...' } value. And the
> internal BUILD file generator also supports same name libs that are in
different
> directories.
> Also I think now, that the hashable is directly tied to the uniqueness of the
> output directory, there should be no drawback for Chromium or the average GYP
> based project, as overriding 'SYMROOT' is something that you should only do if
> you know what you're doing.
> 
> Thanks for the reviews and the patience.
> Have nice holidays.

Ran into same problems with PBXGroup. Changed the CL to update the hashables of
PBXGroup instead of PBXReferenceProxy. Works the same way in all other regards.

[jsartisohn, 2014-11-27 17:10:46]

Patchset #4 (id:60001) has been deleted

[jsartisohn, 2014-12-01 16:33:32]

Patchset #5 (id:100001) has been deleted

[jsartisohn, 2014-12-01 16:38:41]

Made another change with even stricter rules for changing the hash.
Now _all_ targets must have a unique 'SYMROOT' attribute (either directly set or
inherited by the project).

[Mark Mentovai, 2014-12-01 21:38:31]

I think we can go with this now. LGTM.

[jsartisohn, 2014-12-02 10:26:18]

On 2014/12/01 21:38:31, Mark Mentovai wrote:
> I think we can go with this now. LGTM.

Nice, Thanks for the patience.
Also, not sure if something has changed concerning the submit process. I used
"git try" as it's mentioned here: https://code.google.com/p/gyp/wiki/GypHacking,
but it asked me for svn credentials, so I guess it's still the old system in the
background. I'll wait for someone of you to submit or tell me what to to to
submit for myself.
I uploaded a rebased patchset, but as it's probably still SVN in the background
this will not matter.

[Mark Mentovai, 2014-12-02 16:59:47]

The CQ bit was checked by mark@chromium.org

[commit-bot: I haz the power, 2014-12-02 17:02:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-02 17:02:03]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[jsartisohn, 2014-12-02 17:09:10]

On 2014/12/02 17:02:03, I haz the power (commit-bot) wrote:
> Commit queue rejected this change because it did not recognize the base URL.
> Please commit your change manually.

Hm, are empty files handled correctly? I had an issue with git aplly when
patchin this cl where it would not create for example void.cc.
I'll upload a patch set with coment sin the .cc files.

[scottmg, 2014-12-02 17:49:13]

On 2014/12/02 17:09:10, jsartisohn wrote:
> On 2014/12/02 17:02:03, I haz the power (commit-bot) wrote:
> > Commit queue rejected this change because it did not recognize the base URL.
> > Please commit your change manually.
> 
> Hm, are empty files handled correctly? I had an issue with git aplly when
> patchin this cl where it would not create for example void.cc.
> I'll upload a patch set with coment sin the .cc files.

There's just no commit queue. I can land it for you.

[scottmg, 2014-12-02 17:51:57]

On 2014/12/02 17:49:13, scottmg wrote:
> On 2014/12/02 17:09:10, jsartisohn wrote:
> > On 2014/12/02 17:02:03, I haz the power (commit-bot) wrote:
> > > Commit queue rejected this change because it did not recognize the base
URL.
> > > Please commit your change manually.
> > 
> > Hm, are empty files handled correctly? I had an issue with git aplly when
> > patchin this cl where it would not create for example void.cc.
> > I'll upload a patch set with coment sin the .cc files.
> 
> There's just no commit queue. I can land it for you.

It looks like the new test is failing on the bots for the previous patchset? I
just sent jobs for ps7.

[Mark Mentovai, 2014-12-02 17:52:00]

I’ve been trying to land this but the svn server isn’t taking it.

Authentication realm: <https://gyp.googlecode.com:443> Google Code Subversion
Repository
Username: mark@chromium.org
Password for 'mark@chromium.org': 
svn: E170001: Commit failed (details follow):
svn: E170001: MKACTIVITY of
'/svn/!svn/act/7837f15d-e31a-4982-af57-e275aad2ea04': authorization failed:
Could not authenticate to server: rejected Basic challenge
(https://gyp.googlecode.com)
svn: E170001: Your commit message was left in a temporary file:
svn: E170001:    '/chrome/gyp/svn-commit.tmp'

[scottmg, 2014-12-02 18:38:16]

Committed patchset #7 (id:160001) manually as 2011 (presubmit successful).

[scottmg, 2014-12-02 18:38:37]

On 2014/12/02 17:52:00, Mark Mentovai wrote:
> I’ve been trying to land this but the svn server isn’t taking it.
> 
> Authentication realm: <https://gyp.googlecode.com:443> Google Code Subversion
> Repository
> Username: mailto:mark@chromium.org
> Password for mailto:: 
> svn: E170001: Commit failed (details follow):
> svn: E170001: MKACTIVITY of
> '/svn/!svn/act/7837f15d-e31a-4982-af57-e275aad2ea04': authorization failed:
> Could not authenticate to server: rejected Basic challenge
> (https://gyp.googlecode.com)
> svn: E170001: Your commit message was left in a temporary file:
> svn: E170001:    '/chrome/gyp/svn-commit.tmp'

I'm not sure what that is, maybe need to re-authenticate for some reason. It
landed ok for me.

[Nico, 2015-01-15 18:43:23]

I'm reverting this for now as it made the xcode generator very slow for larger
projects (see https://code.google.com/p/chromium/issues/detail?id=447801)