Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2094693002:
[Mac/GN] Print a warning if the SDK version is 10.11 but Xcode is <7.3. (Closed)
|
Created:
4 years, 6 months ago by Robert Sesek Modified:
4 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac/GN] Assert if the SDK version is 10.11 but Xcode is <7.3.
There is a known incompatibility when using the 10.11 SDK and Xcode 7.2
BUG=620127
R=mark@chromium.org,dpranke@chromium.org
Committed: https://crrev.com/42e3c3e7125d71393abb461ff00242a8d2d8aa57
Cr-Commit-Position: refs/heads/master@{#402977}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Make fatal #Messages
Total messages: 18 (8 generated)
Description was changed from ========== [Mac/GN] Print a warning if the SDK version is 10.11 but Xcode is <7.3. There is a known incompatibility when using the 10.11 SDK and Xcode 7.2 BUG=620127 R=mark@chromium.org ========== to ========== [Mac/GN] Print a warning if the SDK version is 10.11 but Xcode is <7.3. There is a known incompatibility when using the 10.11 SDK and Xcode 7.2 BUG=620127 R=mark@chromium.org,dpranke@chromium.org ==========
rsesek@chromium.org changed reviewers: + dpranke@google.com
rsesek@chromium.org changed reviewers: + dpranke@chromium.org - dpranke@google.com
Should this be a fatal error (and hence we should be using assert() )?
On 2016/06/24 19:36:45, Dirk Pranke wrote: > Should this be a fatal error (and hence we should be using assert() )? Maybe if is_component_build is true, since that would actually result in an unusable build. In the non-component case, the issue doesn't come up.
Up to you, though I prefer fatal errors to warnings where possible. Warnings get ignored ;) lgtm.
LGTM https://codereview.chromium.org/2094693002/diff/1/build/config/mac/mac_sdk.gni File build/config/mac/mac_sdk.gni (right): https://codereview.chromium.org/2094693002/diff/1/build/config/mac/mac_sdk.gn... build/config/mac/mac_sdk.gni:69: " either upgrade Xcode to the latest (7.3.1) or install the Mac OS X") Unless you plan on keeping the number 7.3.1 in sync with whatever’s latest, get rid of it. https://codereview.chromium.org/2094693002/diff/1/build/config/mac/mac_sdk.gn... build/config/mac/mac_sdk.gni:75: print( Does this go to stderr? (If not, could it?) https://codereview.chromium.org/2094693002/diff/1/build/config/mac/mac_sdk.gn... build/config/mac/mac_sdk.gni:76: "********************************************************************************") I like making this fatal, at least in the configuration where we know it won’t work as you suggested.
I've made this unconditionally fatal per your recommendations. https://codereview.chromium.org/2094693002/diff/1/build/config/mac/mac_sdk.gni File build/config/mac/mac_sdk.gni (right): https://codereview.chromium.org/2094693002/diff/1/build/config/mac/mac_sdk.gn... build/config/mac/mac_sdk.gni:69: " either upgrade Xcode to the latest (7.3.1) or install the Mac OS X") On 2016/06/25 20:31:38, Mark Mentovai wrote: > Unless you plan on keeping the number 7.3.1 in sync with whatever’s latest, get > rid of it. Done. https://codereview.chromium.org/2094693002/diff/1/build/config/mac/mac_sdk.gn... build/config/mac/mac_sdk.gni:75: print( On 2016/06/25 20:31:38, Mark Mentovai wrote: > Does this go to stderr? (If not, could it?) It goes to stdout, as does the new assertion failure. I don't know if we can change this easily. https://codereview.chromium.org/2094693002/diff/1/build/config/mac/mac_sdk.gn... build/config/mac/mac_sdk.gni:76: "********************************************************************************") On 2016/06/25 20:31:38, Mark Mentovai wrote: > I like making this fatal, at least in the configuration where we know it won’t > work as you suggested. Done.
Description was changed from ========== [Mac/GN] Print a warning if the SDK version is 10.11 but Xcode is <7.3. There is a known incompatibility when using the 10.11 SDK and Xcode 7.2 BUG=620127 R=mark@chromium.org,dpranke@chromium.org ========== to ========== [Mac/GN] Assert if the SDK version is 10.11 but Xcode is <7.3. There is a known incompatibility when using the 10.11 SDK and Xcode 7.2 BUG=620127 R=mark@chromium.org,dpranke@chromium.org ==========
The CQ bit was checked by rsesek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2094693002/#ps20001 (title: "Make fatal")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Mac/GN] Assert if the SDK version is 10.11 but Xcode is <7.3. There is a known incompatibility when using the 10.11 SDK and Xcode 7.2 BUG=620127 R=mark@chromium.org,dpranke@chromium.org ========== to ========== [Mac/GN] Assert if the SDK version is 10.11 but Xcode is <7.3. There is a known incompatibility when using the 10.11 SDK and Xcode 7.2 BUG=620127 R=mark@chromium.org,dpranke@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [Mac/GN] Assert if the SDK version is 10.11 but Xcode is <7.3. There is a known incompatibility when using the 10.11 SDK and Xcode 7.2 BUG=620127 R=mark@chromium.org,dpranke@chromium.org ========== to ========== [Mac/GN] Assert if the SDK version is 10.11 but Xcode is <7.3. There is a known incompatibility when using the 10.11 SDK and Xcode 7.2 BUG=620127 R=mark@chromium.org,dpranke@chromium.org Committed: https://crrev.com/42e3c3e7125d71393abb461ff00242a8d2d8aa57 Cr-Commit-Position: refs/heads/master@{#402977} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/42e3c3e7125d71393abb461ff00242a8d2d8aa57 Cr-Commit-Position: refs/heads/master@{#402977} |
