Adding framework path mapping for 'copies' action in gyp files.

Adding framework path mapping for 'copies' action in gyp files.

This will set the destination to number 10 (Frameworks in Xcode UI).
Enables 'Embedded Binaries' to be linked using a framework.
May need to use LD_RUNPATH_SEARCH_PATHS to local framework

R=mark@chromium.org

Committed: https://chromium.googlesource.com/external/gyp/+/427a0406159c5fe7a9d057076dfb7acd2b92682a

[yukawa, 2015-07-30 18:21:16]

yukawa@chromium.org changed reviewers:
+ mark@chromium.org, thakis@chromium.org, yukawa@chromium.org

[yukawa, 2015-07-30 18:21:16]

Hi Mark and Nico.  Can you take a look at Tony's CL?  He wanted to make this
change for his projects and it would be great if we can do it in upstream first.

Thanks!

[Nico, 2015-07-30 18:28:22]

is it possible to write a test for this?

this is really more for mark. he's on leave, but he should be back in two days…

https://codereview.chromium.org/1254413004/diff/20001/pylib/gyp/xcodeproj_fil...
File pylib/gyp/xcodeproj_file.py (right):

https://codereview.chromium.org/1254413004/diff/20001/pylib/gyp/xcodeproj_fil...
pylib/gyp/xcodeproj_file.py:1962: # : 10,  # Frameworks
remove

[tonyworks, 2015-09-25 16:23:19]

On 2015/07/30 18:28:22, Nico (offline until Fri Oct 9) wrote:
> is it possible to write a test for this?
> 
> this is really more for mark. he's on leave, but he should be back in two
days…
> 
>
https://codereview.chromium.org/1254413004/diff/20001/pylib/gyp/xcodeproj_fil...
> File pylib/gyp/xcodeproj_file.py (right):
> 
>
https://codereview.chromium.org/1254413004/diff/20001/pylib/gyp/xcodeproj_fil...
> pylib/gyp/xcodeproj_file.py:1962: # : 10,  # Frameworks
> remove

Is Mark able to provide some input? I have been manually adding this in my repo
for now. I would be happy to look at writing a test case for this, however I
could use some assistance as to where since I am not familiar with how this
section is being tested.

[Mark Mentovai, 2015-09-25 16:28:40]

This looks good, but with a test, it’d look even better.

[Mark Mentovai, 2015-09-25 16:45:53]

There’s a whole test infrastructure in the test directory, and the entry point
to run them is the gyptest.py script. There are a few “copies” tests in
test/copies.

[tonyworks, 2015-09-29 19:19:30]

On 2015/09/25 16:45:53, Mark Mentovai - August is over wrote:
> There’s a whole test infrastructure in the test directory, and the entry point
> to run them is the gyptest.py script. There are a few “copies” tests in
> test/copies.

I added some tests and verified. I did have to comment out the sys.exit in the
gyptest-copies.py to verify, then I added it back in for sanity.

[Mark Mentovai, 2015-09-29 20:49:24]

Does the test also pass with the ninja generator? Things that work in Xcode
should also work in ninja.

[tonyworks, 2015-09-29 22:50:07]

On 2015/09/29 20:49:24, Mark Mentovai - August is over wrote:
> Does the test also pass with the ninja generator? Things that work in Xcode
> should also work in ninja.

No, it isnt. I was only validating with -f xcode. I do not believe ninja can
handle the xcode variables unless I pipe them into something else. Would it be
more appropriate to drop any tests outside of the test/mac directory or
incorporate a new <(FRAMEWORKS_DIR) variable similar to how Products directory
is handled? Obviously the latter is more work and more prone to error. My
concern is that it is a little overkill for such a small change and use-case
only relevant to Xcode.

[Mark Mentovai, 2015-09-29 22:55:44]

ninja does handle some Xcode variables like SDKROOT and BUILT_PRODUCTS_DIR.
Since path_tree_to_subfolder already named BUILT_PRODUCTS_DIR for Xcode-side
support, it may make sense to ensure ninja support for BUILT_FRAMEWORKS_DIR too
now that it’s being added for Xcode.

[Mark Mentovai, 2015-10-01 14:25:25]

I don’t think it’s worthwhile to have this <(FRAMEWORK_DIR) thing or to have
this non-Mac test. It’s enough for xcode_emulation to understand
BUILT_FRAMEWORKS_DIR and for the Mac-specific test to pass with xcode, ninja,
and make.

https://codereview.chromium.org/1254413004/diff/80001/test/copies/gyptest-def...
File test/copies/gyptest-default.py (right):

https://codereview.chromium.org/1254413004/diff/80001/test/copies/gyptest-def...
test/copies/gyptest-default.py:25:
test.built_file_must_match('copies-out/framework/file1',
I wouldn’t expect this test to work on non-Mac. Maybe it incidentally will with
ninja and make, but definitely not msvs.

[tonyworks, 2015-10-01 14:36:59]

On 2015/10/01 14:25:25, Mark Mentovai wrote:
> I don’t think it’s worthwhile to have this <(FRAMEWORK_DIR) thing or to have
> this non-Mac test. It’s enough for xcode_emulation to understand
> BUILT_FRAMEWORKS_DIR and for the Mac-specific test to pass with xcode, ninja,
> and make.
> 
>
https://codereview.chromium.org/1254413004/diff/80001/test/copies/gyptest-def...
> File test/copies/gyptest-default.py (right):
> 
>
https://codereview.chromium.org/1254413004/diff/80001/test/copies/gyptest-def...
> test/copies/gyptest-default.py:25:
> test.built_file_must_match('copies-out/framework/file1',
> I wouldn’t expect this test to work on non-Mac. Maybe it incidentally will
with
> ninja and make, but definitely not msvs.

I agree. Where would the go? Or is putting in an if darwin statement
appropriate?

[tonyworks, 2015-10-01 14:37:26]

https://codereview.chromium.org/1254413004/diff/80001/test/copies/gyptest-def...
File test/copies/gyptest-default.py (right):

https://codereview.chromium.org/1254413004/diff/80001/test/copies/gyptest-def...
test/copies/gyptest-default.py:25:
test.built_file_must_match('copies-out/framework/file1',
On 2015/10/01 14:25:25, Mark Mentovai wrote:
> I wouldn’t expect this test to work on non-Mac. Maybe it incidentally will
with
> ninja and make, but definitely not msvs.

How would you recommend I proceed? Any recommendations?

[Mark Mentovai, 2015-10-01 14:54:23]

If you don’t need the <(FRAMEWORKS_DIR) stuff, get rid of it and the test that
uses it. The $(BUILT_FRAMEWORKS_DIR) test can stay, and will run on Mac-only,
with all generators on Mac, so you’ll have test coverage there. It looks like
the xcode_emulation change should make that work for both ninja and make.

[tonyworks, 2015-10-01 15:56:46]

On 2015/10/01 14:54:23, Mark Mentovai wrote:
> If you don’t need the <(FRAMEWORKS_DIR) stuff, get rid of it and the test that
> uses it. The $(BUILT_FRAMEWORKS_DIR) test can stay, and will run on Mac-only,
> with all generators on Mac, so you’ll have test coverage there. It looks like
> the xcode_emulation change should make that work for both ninja and make.

I'm not the expert, so I can only speak to what I have seen so far. Ninja will
fail even if using $(BUILT_PRODUCTS_DIR), so I need to set a variable in
ninja.py and make.py if I am going to test via copies.gyp. The other option is
to have the test/mac/framework.gyp be the only test, but it is disabled.

[Mark Mentovai, 2015-10-01 22:52:23]

tonyworks wrote:
> The other option is
> to have the test/mac/framework.gyp be the only test, but it is disabled.

Yes, that’s what I meant. I didn’t realize it was disabled. That’s unfortunate,
because I think it’s really the right way to test it.

The bug about it being disabled isn’t specific to that test, so I’m not really
sure why it’s off.

I’ll try to find time to take a look at that test, but I’m not sure how quickly
I’ll be able to get to it. If you’re able to get it passing, that’d be great
too.

[tonyworks, 2015-10-15 16:01:52]

On 2015/10/01 22:52:23, Mark Mentovai wrote:
> tonyworks wrote:
> > The other option is
> > to have the test/mac/framework.gyp be the only test, but it is disabled.
> 
> Yes, that’s what I meant. I didn’t realize it was disabled. That’s
unfortunate,
> because I think it’s really the right way to test it.
> 
> The bug about it being disabled isn’t specific to that test, so I’m not really
> sure why it’s off.
> 
> I’ll try to find time to take a look at that test, but I’m not sure how
quickly
> I’ll be able to get to it. If you’re able to get it passing, that’d be great
> too.

I decided to go to ninja and make which makes everything pass and I do not
believe it impacts or weakens any of the tests, while it does ensure that the
framework flag works correctly.

[Mark Mentovai, 2015-10-19 20:53:13]

https://codereview.chromium.org/1254413004/diff/120001/test/copies/src/copies...
File test/copies/src/copies.gyp (right):

https://codereview.chromium.org/1254413004/diff/120001/test/copies/src/copies...
test/copies/src/copies.gyp:37: 'destination':
'<(FRAMEWORK_DIR)/copies-out/framework',
I don’t think we should bother defining FRAMEWORK_DIR for any generator now.
(And this test will be broken for non-Mac generators like msvs.) We can rely on
the generators that work on Mac to understand the Xcode-style
$(BUILT_FRAMEWORKS_DIR) variable reference.

[tonyworks, 2015-10-19 20:55:57]

On 2015/10/19 20:53:13, Mark Mentovai wrote:
>
https://codereview.chromium.org/1254413004/diff/120001/test/copies/src/copies...
> File test/copies/src/copies.gyp (right):
> 
>
https://codereview.chromium.org/1254413004/diff/120001/test/copies/src/copies...
> test/copies/src/copies.gyp:37: 'destination':
> '<(FRAMEWORK_DIR)/copies-out/framework',
> I don’t think we should bother defining FRAMEWORK_DIR for any generator now.
> (And this test will be broken for non-Mac generators like msvs.) We can rely
on
> the generators that work on Mac to understand the Xcode-style
> $(BUILT_FRAMEWORKS_DIR) variable reference.

Can you expand more on how you want me to proceed? Clearly I need to add this to
xcoderproj_file.py and then just remove all tests?

[Mark Mentovai, 2015-10-19 21:00:39]

I mean:

Remove the changes to make.py, ninja.py, and xcode.py, and the test changes in
test/copies. Leave the changes in xcode_emulation.py and xcodeproj_file.py
alone, as well as the test in test/mac.

But that test is still disabled :(.

[tonyworks, 2015-10-19 21:23:09]

On 2015/10/19 21:00:39, Mark Mentovai wrote:
> I mean:
> 
> Remove the changes to make.py, ninja.py, and xcode.py, and the test changes in
> test/copies. Leave the changes in xcode_emulation.py and xcodeproj_file.py
> alone, as well as the test in test/mac.
> 
> But that test is still disabled :(.

K, I am ok with that.

[Mark Mentovai, 2015-10-20 13:59:14]

LGTM

[Mark Mentovai, 2015-10-20 14:00:34]

Committed patchset #8 (id:140001) manually as
427a0406159c5fe7a9d057076dfb7acd2b92682a (presubmit successful).

[Mark Mentovai, 2015-10-20 14:02:09]

Thanks for the patch and the patience!