Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(244)

Issue 23475025: ninja/mac: Don't link c-file-only targets to libstdc++

Created:
7 years, 3 months ago by Nico
Modified:
7 years, 3 months ago
CC:
gyp-developer_googlegroups.com, Paweł Hajdan Jr.
Visibility:
Public.

Description

ninja/posix: Don't link c-file-only targets to libstdc++. The most important part of this change is that LINK will now default to what is set for CXX (in make) or what is set for CC (in ninja). In ninja, there's now an additional LINKXX that's set to what CXX is set to, and in ninja LINKXX is only used to link targets that contain at least one C++ source file. Also change several make_global_settings behaviors: 1. The ninja generator was using LD and LD.host for make_global_settings, while the make generator uses LINK and LINK.host. Chromium's common.gypi only sets the latter, so it looks like this code isn't needed. Removed support for setting LD and LD.host via make_global_settings in ninja. (In practice, this was alway set to the same thing as CC / CXX anyhow.) 2. The same is done for the environment variables in ninja. Again, as the linker now just uses what's set via CC / CXX, this shouldn't be an issue in practice. 3. Change the make generator to look at the LINK envvar instead of LD, for consistency with its make_global_settings key and with what `make -p` prints. (If setting a linker that's different from CC / CXX is needed later on, this is easy to add back.) (Make doesn't have explicit support for setting LINK, make_global_settings allows changing arbitrary make variables there. But due to the defaults change, setting LINK is no longer required in make either.) BUG=chromium:280718

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Patch Set 12 : #

Patch Set 13 : #

Patch Set 14 : #

Total comments: 6

Patch Set 15 : #

Patch Set 16 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+132 lines, -39 lines) Patch
M pylib/gyp/generator/make.py View 1 2 3 4 5 6 7 8 9 3 chunks +10 lines, -10 lines 0 comments Download
M pylib/gyp/generator/ninja.py View 1 2 3 4 5 6 7 8 9 10 11 12 11 chunks +14 lines, -9 lines 1 comment Download
M test/compiler-override/gyptest-compiler-env.py View 1 2 3 4 5 6 7 8 9 10 5 chunks +17 lines, -13 lines 0 comments Download
M test/make_global_settings/basics/gyptest-make_global_settings.py View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -3 lines 0 comments Download
M test/make_global_settings/basics/make_global_settings.gyp View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M test/make_global_settings/env-wrapper/gyptest-wrapper.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +6 lines, -1 line 0 comments Download
M test/make_global_settings/env-wrapper/wrapper.gyp View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M test/make_global_settings/wrapper/gyptest-wrapper.py View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
A test/no-cpp/gyptest-no-cpp.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +49 lines, -0 lines 0 comments Download
A test/no-cpp/src/call-f-main.c View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
A test/no-cpp/src/empty-main.c View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A test/no-cpp/src/f.cc View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
A test/no-cpp/src/test.gyp View 1 2 3 4 5 6 7 8 9 10 1 chunk +25 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Nico
7 years, 3 months ago (2013-09-10 20:46:39 UTC) #1
scottmg
BUG link in description? lgtm https://codereview.chromium.org/23475025/diff/33002/test/no-cpp/gyptest-no-cpp.py File test/no-cpp/gyptest-no-cpp.py (right): https://codereview.chromium.org/23475025/diff/33002/test/no-cpp/gyptest-no-cpp.py#newcode27 test/no-cpp/gyptest-no-cpp.py:27: def LinksLibStdCpp(p): nit; p ...
7 years, 3 months ago (2013-09-10 21:09:15 UTC) #2
Nico
Bug link added, thanks! https://codereview.chromium.org/23475025/diff/33002/test/no-cpp/gyptest-no-cpp.py File test/no-cpp/gyptest-no-cpp.py (right): https://codereview.chromium.org/23475025/diff/33002/test/no-cpp/gyptest-no-cpp.py#newcode21 test/no-cpp/gyptest-no-cpp.py:21: # TODO: Does a test ...
7 years, 3 months ago (2013-09-10 21:11:45 UTC) #3
Paweł Hajdan Jr.
Drive-by with a nit, totally up to you. https://codereview.chromium.org/23475025/diff/38014/pylib/gyp/generator/ninja.py File pylib/gyp/generator/ninja.py (right): https://codereview.chromium.org/23475025/diff/38014/pylib/gyp/generator/ninja.py#newcode377 pylib/gyp/generator/ninja.py:377: self.uses_cpp ...
7 years, 3 months ago (2013-09-10 21:25:31 UTC) #4
scottmg
https://codereview.chromium.org/23475025/diff/33002/test/no-cpp/gyptest-no-cpp.py File test/no-cpp/gyptest-no-cpp.py (right): https://codereview.chromium.org/23475025/diff/33002/test/no-cpp/gyptest-no-cpp.py#newcode21 test/no-cpp/gyptest-no-cpp.py:21: # TODO: Does a test like this make sense ...
7 years, 3 months ago (2013-09-10 21:31:33 UTC) #5
scottmg
On 2013/09/10 21:31:33, scottmg wrote: > https://codereview.chromium.org/23475025/diff/33002/test/no-cpp/gyptest-no-cpp.py > File test/no-cpp/gyptest-no-cpp.py (right): > > https://codereview.chromium.org/23475025/diff/33002/test/no-cpp/gyptest-no-cpp.py#newcode21 > ...
7 years, 3 months ago (2013-09-10 21:41:39 UTC) #6
binji
7 years, 3 months ago (2013-09-12 20:46:12 UTC) #7
This change seems to be breaking the mac_sdk_multi bots:

http://build.chromium.org/p/client.nacl.sdk/builders/mac-sdk-multi/builds/589...

AFAICT, the sel_ldr executable only has .c sources, but has dependencies that
have .cc sources. Perhaps this CL is not checking for that case?

Powered by Google App Engine
This is Rietveld 408576698