Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1213123002:
Fix XCode emulation when SDK is not installed.
|
Created:
5 years, 5 months ago by bnoordhuis Modified:
4 years, 11 months ago Reviewers:
Nico CC:
gyp-developer_googlegroups.com Base URL:
https://chromium.googlesource.com/external/gyp@master Target Ref:
refs/heads/master Project:
gyp Visibility:
Public. |
DescriptionFix XCode emulation when SDK is not installed.
Fixes a regression from https://codereview.chromium.org/82763006
BUG=gyp:477
Patch Set 1 #
Total comments: 2
Messages
Total messages: 10 (3 generated)
ben@strongloop.com changed reviewers: + thakis@chromium.org
thakis@chromium.org changed reviewers: + sdefresne@chromium.org
+sdefresne who wrote this https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.py File pylib/gyp/xcode_emulation.py (right): https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.p... pylib/gyp/xcode_emulation.py:1484: env['SDKROOT'] = sdk_root Is it useful to set SDKROOT to an empty string? Should this just be if sdk_root is not None: env['SDKROOT'] = sdk_root
https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.py File pylib/gyp/xcode_emulation.py (right): https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.p... pylib/gyp/xcode_emulation.py:1484: env['SDKROOT'] = sdk_root On 2015/06/29 at 16:25:19, Nico wrote: > Is it useful to set SDKROOT to an empty string? Should this just be > > if sdk_root is not None: > env['SDKROOT'] = sdk_root IIUC this CL is to get Xcode emulation working when SDK is not installed. I'm not sure if it is possible to install Xcode without installing a SDK, and since this variable is only used by Xcode and its command line tools, I don't think exporting or not the environment variable has any impact. I'd tend to favor the code that does not export the variable then.
On 2015/06/29 16:48:59, sdefresne wrote: > https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.py > File pylib/gyp/xcode_emulation.py (right): > > https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.p... > pylib/gyp/xcode_emulation.py:1484: env['SDKROOT'] = sdk_root > On 2015/06/29 at 16:25:19, Nico wrote: > > Is it useful to set SDKROOT to an empty string? Should this just be > > > > if sdk_root is not None: > > env['SDKROOT'] = sdk_root > > IIUC this CL is to get Xcode emulation working when SDK is not installed. I'm > not sure if it is possible to install Xcode without installing a SDK, and since > this variable is only used by Xcode and its command line tools, I don't think > exporting or not the environment variable has any impact. > > I'd tend to favor the code that does not export the variable then. Correct me if I'm wrong, but wouldn't conditionally setting SDKROOT break a GYP file that links to '$(SDKROOT)/System/Library/Frameworks/Foo.framework' in its 'libraries' section?
On 2015/06/30 11:12:36, bnoordhuis wrote: > On 2015/06/29 16:48:59, sdefresne wrote: > > https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.py > > File pylib/gyp/xcode_emulation.py (right): > > > > > https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.p... > > pylib/gyp/xcode_emulation.py:1484: env['SDKROOT'] = sdk_root > > On 2015/06/29 at 16:25:19, Nico wrote: > > > Is it useful to set SDKROOT to an empty string? Should this just be > > > > > > if sdk_root is not None: > > > env['SDKROOT'] = sdk_root > > > > IIUC this CL is to get Xcode emulation working when SDK is not installed. I'm > > not sure if it is possible to install Xcode without installing a SDK, and > since > > this variable is only used by Xcode and its command line tools, I don't think > > exporting or not the environment variable has any impact. > > > > I'd tend to favor the code that does not export the variable then. > > Correct me if I'm wrong, but wouldn't conditionally setting SDKROOT break a GYP > file that links to '$(SDKROOT)/System/Library/Frameworks/Foo.framework' in its > 'libraries' section? Relying on $(SDKROOT) expanding to '' so that that becomes an absolute path is pretty subtle :-/ If you need this I guess it's fine, but add a comment like `# For $(SDKROOT)/System/Library/Frameworks/Foo.framework` above the line that sets that path to '' please.
On 2015/06/30 at 18:33:30, thakis wrote: > On 2015/06/30 11:12:36, bnoordhuis wrote: > > On 2015/06/29 16:48:59, sdefresne wrote: > > > https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.py > > > File pylib/gyp/xcode_emulation.py (right): > > > > > > > > https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.p... > > > pylib/gyp/xcode_emulation.py:1484: env['SDKROOT'] = sdk_root > > > On 2015/06/29 at 16:25:19, Nico wrote: > > > > Is it useful to set SDKROOT to an empty string? Should this just be > > > > > > > > if sdk_root is not None: > > > > env['SDKROOT'] = sdk_root > > > > > > IIUC this CL is to get Xcode emulation working when SDK is not installed. I'm > > > not sure if it is possible to install Xcode without installing a SDK, and > > since > > > this variable is only used by Xcode and its command line tools, I don't think > > > exporting or not the environment variable has any impact. > > > > > > I'd tend to favor the code that does not export the variable then. > > > > Correct me if I'm wrong, but wouldn't conditionally setting SDKROOT break a GYP > > file that links to '$(SDKROOT)/System/Library/Frameworks/Foo.framework' in its > > 'libraries' section? > > Relying on $(SDKROOT) expanding to '' so that that becomes an absolute path is pretty subtle :-/ If you need this I guess it's fine, but add a comment like `# For $(SDKROOT)/System/Library/Frameworks/Foo.framework` above the line that sets that path to '' please. How can not setting the variable break in such a situation. If a variable is unset, it gets expanded to '' by the shell. The only case were it would be different would be if you already had a SDKROOT variable in the current environment before invoking the build tool.
On 2015/07/01 10:49:58, sdefresne wrote: > On 2015/06/30 at 18:33:30, thakis wrote: > > On 2015/06/30 11:12:36, bnoordhuis wrote: > > > On 2015/06/29 16:48:59, sdefresne wrote: > > > > > https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.py > > > > File pylib/gyp/xcode_emulation.py (right): > > > > > > > > > > > > https://codereview.chromium.org/1213123002/diff/1/pylib/gyp/xcode_emulation.p... > > > > pylib/gyp/xcode_emulation.py:1484: env['SDKROOT'] = sdk_root > > > > On 2015/06/29 at 16:25:19, Nico wrote: > > > > > Is it useful to set SDKROOT to an empty string? Should this just be > > > > > > > > > > if sdk_root is not None: > > > > > env['SDKROOT'] = sdk_root > > > > > > > > IIUC this CL is to get Xcode emulation working when SDK is not installed. > I'm > > > > not sure if it is possible to install Xcode without installing a SDK, and > > > since > > > > this variable is only used by Xcode and its command line tools, I don't > think > > > > exporting or not the environment variable has any impact. > > > > > > > > I'd tend to favor the code that does not export the variable then. > > > > > > Correct me if I'm wrong, but wouldn't conditionally setting SDKROOT break a > GYP > > > file that links to '$(SDKROOT)/System/Library/Frameworks/Foo.framework' in > its > > > 'libraries' section? > > > > Relying on $(SDKROOT) expanding to '' so that that becomes an absolute path is > pretty subtle :-/ If you need this I guess it's fine, but add a comment like `# > For $(SDKROOT)/System/Library/Frameworks/Foo.framework` above the line that sets > that path to '' please. > > How can not setting the variable break in such a situation. If a variable is > unset, it gets expanded to '' by the shell. This is $(FOO), not ${FOO}. That's expanded by xcode / xcode_emulation. If the shell sees it $(asdf), it means the same as `asdf`: "run asdf, get the result from that": $ echo $(asdf) -bash: asdf: command not found > The only case were it would be > different would be if you already had a SDKROOT variable in the current > environment before invoking the build tool.
sdefresne@chromium.org changed reviewers: - sdefresne@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
