Do not remote duplicate entries from ldflags when generating ninja files as it changes behavior

Do not remote duplicate entries from ldflags when generating ninja files as it changes behavior

In some cases linker command line is required to contain duplicate entries. One example is to pass
-Wl,--whole-archive <libs> -Wl,--no-whole-archive. Removing repeating --no-whole-archive will result in incorrect behavior.

This happens e.g. when generating ninja files for io.js (./tools/gyp_node.py -f ninja). Without this  fix linkage would fail due to multiple definition of symbols from libopenssl and libv8_base. That's because node.gyp contains several places where ldflags are extended with manual flags, e.g. https://github.com/nodejs/io.js/commit/9f36c0d235f4eb7e6528face49c15045a5e41e14 . Without this CL iojs.ninja contains

ldflags = -pthread -rdynamic -m64 -Wl,--whole-archive libopenssl.a $
    -Wl,--no-whole-archive -Wl,-z,noexecstack -Wl,--whole-archive libv8_base.a

Notice unbalanced --whole-archive. With the patch it becomes

ldflags = -pthread -rdynamic -m64 -Wl,--whole-archive libopenssl.a $
    -Wl,--no-whole-archive -Wl,-z,noexecstack -Wl,--whole-archive $
    libv8_base.a -Wl,--no-whole-archive -pthread

BUG=None
R=thakis@chromium.org

Committed: https://chromium.googlesource.com/external/gyp/+/25ed9ac4ac2a4d2a08909225fbb6d56e89ad38a6

[yurys, 2015-06-24 12:22:39]

yurys@chromium.org changed reviewers:
+ thakis@chromium.org

[yurys, 2015-06-24 12:22:53]

The CQ bit was checked by yurys@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-24 12:26:03]

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

[commit-bot: I haz the power, 2015-06-24 12:26:04]

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

[Nico, 2015-06-24 17:47:50]

thakis@chromium.org changed reviewers:
+ evan@chromium.org

[Nico, 2015-06-24 17:47:51]

Hm, the make generator seems to only unique libraries, not ldflags. Looks like
this call has been around since the initial check-in of the ninja generator in
https://codereview.chromium.org/7696003 . Evan, do you remember why you added
it?

If this doesn't break any tests and you add a new test for your change, this
looks fine to me. Have you tried building chromium with this change applied?

[yurys, 2015-06-24 18:57:45]

On 2015/06/24 17:47:51, Nico (away Wed Jun 24) wrote:
> Hm, the make generator seems to only unique libraries, not ldflags. Looks like
> this call has been around since the initial check-in of the ninja generator in
> https://codereview.chromium.org/7696003 . Evan, do you remember why you added
> it?
> 
> If this doesn't break any tests and you add a new test for your change, this
> looks fine to me. Have you tried building chromium with this change applied?

Yes, I managed to built Chromium Release on my Linux box. I will add a test for
the described use case tomorrow.

[yurys, 2015-06-25 14:43:59]

Added a test. PTAL.

[Nico, 2015-06-25 16:02:21]

https://codereview.chromium.org/1209553002/diff/20001/test/ninja/duplicate-ld...
File test/ninja/duplicate-ldflags/gyptest-duplicate-ldflags.py (right):

https://codereview.chromium.org/1209553002/diff/20001/test/ninja/duplicate-ld...
test/ninja/duplicate-ldflags/gyptest-duplicate-ldflags.py:17:
'-Wl,--whole-archive lib1.a -Wl,--no-whole-archive')
Can you rewrite this to not inspect the ninja files? I think if you use a
make_global_settings block with a LINK_wrapper pointing at some python script,
the script can inspect the link flags. Then the test would work with make and
ninja and would also test that the two generators are consistent.

[yurys, 2015-06-26 09:35:50]

On 2015/06/25 16:02:21, Nico wrote:
>
https://codereview.chromium.org/1209553002/diff/20001/test/ninja/duplicate-ld...
> File test/ninja/duplicate-ldflags/gyptest-duplicate-ldflags.py (right):
> 
>
https://codereview.chromium.org/1209553002/diff/20001/test/ninja/duplicate-ld...
> test/ninja/duplicate-ldflags/gyptest-duplicate-ldflags.py:17:
> '-Wl,--whole-archive lib1.a -Wl,--no-whole-archive')
> Can you rewrite this to not inspect the ninja files? I think if you use a
> make_global_settings block with a LINK_wrapper pointing at some python script,
> the script can inspect the link flags. Then the test would work with make and
> ninja and would also test that the two generators are consistent.

Done. However, the paths where lib1.a is put are different between make
(out/Default/obj.target/lib1.a) and ninja (out/Default/obj/lib1.a). I gave up
trying to figure out how to reference the output in toolset-independent manner.
I expected a variable like PRODUCT_DIR that I could use, could you help me with
that?

[Nico, 2015-06-26 17:15:36]

lgtm, thanks!

[yurys, 2015-06-27 07:17:51]

The CQ bit was checked by yurys@chromium.org

[yurys, 2015-06-27 07:17:51]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1209553002/#ps80001
(title: "Make now puts lib1.a and lib2.a into PRODUCT_DIR")

[commit-bot: I haz the power, 2015-06-27 07:22:01]

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

[commit-bot: I haz the power, 2015-06-27 07:22:01]

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

[yurys, 2015-06-27 07:22:36]

The CQ bit was checked by yurys@chromium.org

[yurys, 2015-06-27 07:22:37]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1209553002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-27 07:27:01]

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

[commit-bot: I haz the power, 2015-06-27 07:27:02]

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

[yurys, 2015-06-27 08:23:45]

Nico, do I need to request write access for this particular repository or
Chromium commit access is enough?

I'm getting the following error while runnint git cl land:
...
error: failed to push some refs to
'https://chromium.googlesource.com/external/gyp.git'
Failed to push. If this persists, please file a bug.


On 2015/06/26 17:15:36, Nico (afk offsite Jun 26) wrote:
> lgtm, thanks!

[Nico, 2015-06-28 01:04:14]

Committed patchset #7 (id:120001) manually as
25ed9ac4ac2a4d2a08909225fbb6d56e89ad38a6 (presubmit successful).

[Nico, 2015-06-28 01:05:04]

I landed this for you. Your test failed for me on OS X (it doesnt' use
cflags/ldflags), so I tried to make it linux-only and moved it below test/linux.
Please check that the test still does the right thing.

[yurys, 2015-06-29 06:56:33]

Thanks for landing this. I confirm that the test still does what expected.

On 2015/06/28 01:05:04, Nico wrote:
> I landed this for you. Your test failed for me on OS X (it doesnt' use
> cflags/ldflags), so I tried to make it linux-only and moved it below
test/linux.
> Please check that the test still does the right thing.