Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2445993004:
Turn on hermetic toolchain for all corp machines. (Closed)
DescriptionTurn on hermetic toolchain for all corp machines.
BUG=659726
Committed: https://crrev.com/7778e931dabc1fa85d0b77321e3fdef166e0b6a0
Committed: https://crrev.com/da01626aa3c2e1533c53c325de9bd579f0aa54d2
Cr-Original-Commit-Position: refs/heads/master@{#430376}
Cr-Commit-Position: refs/heads/master@{#430863}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Update comment. #
Total comments: 2
Patch Set 3 : Don't use hermetic toolchain on iOS. #
Messages
Total messages: 43 (16 generated)
erikchen@chromium.org changed reviewers: + thakis@chromium.org
thakis: Please review.
thanks! https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py#newc... build/mac_toolchain.py:211: return 1 could this return the "are we using system" bit to gn? Then we'd save one python invocation.
https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py#newc... build/mac_toolchain.py:211: return 1 On 2016/10/26 20:12:08, Nico wrote: > could this return the "are we using system" bit to gn? Then we'd save one python > invocation. This is invoked during "gclient sync", which is decoupled from gn args generation. Either can be called without invoking the other.
on windows, i think we call the same script from deps and gn (https://cs.chromium.org/chromium/src/build/config/win/visual_studio_version.g... , https://cs.chromium.org/chromium/src/DEPS?q=vs_toolchain+file:deps&sq=package...). Is there any way mac could work similar to that?
On 2016/10/26 20:24:51, Nico wrote: > on windows, i think we call the same script from deps and gn > (https://cs.chromium.org/chromium/src/build/config/win/visual_studio_version.g... > , > https://cs.chromium.org/chromium/src/DEPS?q=vs_toolchain+file:deps&sq=package...). > Is there any way mac could work similar to that? This CL does the exact same thing. See build/toolchain/toolchain.gni.
On 2016/10/26 20:29:45, erikchen wrote: > On 2016/10/26 20:24:51, Nico wrote: > > on windows, i think we call the same script from deps and gn > > > (https://cs.chromium.org/chromium/src/build/config/win/visual_studio_version.g... > > , > > > https://cs.chromium.org/chromium/src/DEPS?q=vs_toolchain+file:deps&sq=package...). > > Is there any way mac could work similar to that? > > This CL does the exact same thing. See build/toolchain/toolchain.gni. Ping
Either we're talking past each other, or I'm dense -- in the latter case, apologies, I made an effort to read the file you pointed to. On Windows, as far as I can tell: * build/config/win/visual_studio_version.gni calls build/vs_toolchain.py get_toolchain_dir which checks if the hermetic toolchain should be used, updates it if needed, and returns the toolchain dir. * DEPS also calls vs_toolchain.py On Mac, DEPS calls build/mac_toolchain.py, but build/toolchain/toolchain.gni doesn't: It calls build/mac/should_use_hermetic_xcode.py instead. Hence my question if we could call the same script from deps and gn on mac, like we do on Windows. https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py File build/mac_toolchain.py (left): https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py#oldc... build/mac_toolchain.py:146: proc = subprocess.Popen(['xcode-select', '-p'], stdout=subprocess.PIPE) (at least this is gone) https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py#newc... build/mac_toolchain.py:7: xcode-select is already set and points to an external folder This is no longer true now, right? https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py#newc... build/mac_toolchain.py:144: proc = subprocess.Popen([script_path], stdout=subprocess.PIPE) https://cs.chromium.org/chromium/tools/depot_tools/win_toolchain/get_toolchai... also checks if a bot has access to src-internal, maybe we want that too (not needed in this cl though)
On 2016/11/01 01:07:34, Nico (afk Tue Nov 1) wrote: > Either we're talking past each other, or I'm dense -- in the latter case, > apologies, I made an effort to read the file you pointed to. > > On Windows, as far as I can tell: > * build/config/win/visual_studio_version.gni calls build/vs_toolchain.py > get_toolchain_dir which checks if the hermetic toolchain should be used, updates > it if needed, and returns the toolchain dir. > * DEPS also calls vs_toolchain.py > > On Mac, DEPS calls build/mac_toolchain.py, but build/toolchain/toolchain.gni > doesn't: It calls build/mac/should_use_hermetic_xcode.py instead. Hence my > question if we could call the same script from deps and gn on mac, like we do on > Windows. should_use_hermetic_xcode.py: Determines whether we should use hermetic xcode. mac_toolchain.py: Downloads and installs hermetic xcode. Calls out to should_use_hermetic_xcode.py to determine whether we should use hermetic xcode. So I think you're making the statement that we should have one monolithic script that does both? I don't see the benefit of that - having two smaller scripts with scoped behaviors seems cleaner.
thakis: PTAL https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py#newc... build/mac_toolchain.py:7: xcode-select is already set and points to an external folder On 2016/11/01 01:07:34, Nico (afk Tue Nov 1) wrote: > This is no longer true now, right? Updated Comment. https://codereview.chromium.org/2445993004/diff/1/build/mac_toolchain.py#newc... build/mac_toolchain.py:144: proc = subprocess.Popen([script_path], stdout=subprocess.PIPE) On 2016/11/01 01:07:34, Nico (afk Tue Nov 1) wrote: > https://cs.chromium.org/chromium/tools/depot_tools/win_toolchain/get_toolchai... > also checks if a bot has access to src-internal, maybe we want that too (not > needed in this cl though) Hm. """ if (HaveSrcInternalAccess() or LooksLikeGoogler() or CanAccessToolchainBucket()): """ It's not clear to me why we would want SrcInternalAccess to cause us to use hermetic toolchain.
erikchen@chromium.org changed reviewers: + dpranke@chromium.org
dpranke: Please review thakis: I believe I've addressed all your concerns. Getting another reviewer just to get the CL rolling, since you're OOO for a bit.
lgtm
erikchen@chromium.org changed reviewers: + brettw@chromium.org
brettw: Please review.
justincohen@chromium.org changed reviewers: + justincohen@chromium.org
https://codereview.chromium.org/2445993004/diff/20001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/2445993004/diff/20001/build/mac_toolchain.py#... build/mac_toolchain.py:12: * If xcode-select and xcodebuild are not passwordless in sudoers, requires Will this be a big problem for most googlers workflow?
https://codereview.chromium.org/2445993004/diff/20001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/2445993004/diff/20001/build/mac_toolchain.py#... build/mac_toolchain.py:12: * If xcode-select and xcodebuild are not passwordless in sudoers, requires On 2016/11/05 12:06:36, justincohen wrote: > Will this be a big problem for most googlers workflow? No, as hermetic usually lags behind system xcode.
lgtm
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by erikchen@chromium.org
The CQ bit was checked by erikchen@chromium.org
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Turn on hermetic toolchain for all corp machines. BUG=659726 ========== to ========== Turn on hermetic toolchain for all corp machines. BUG=659726 Committed: https://crrev.com/7778e931dabc1fa85d0b77321e3fdef166e0b6a0 Cr-Commit-Position: refs/heads/master@{#430376} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7778e931dabc1fa85d0b77321e3fdef166e0b6a0 Cr-Commit-Position: refs/heads/master@{#430376}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2487643002/ by sdefresne@chromium.org. The reason for reverting is: This break developer workflow for Googlers (as build/mac/should_use_hermetic_xcode.py returns 1) as the hermetic build does not contains support for iOS SDK, thus "gn gen" fails with the following error: $ gn gen --args='target_os="ios"' out/default ERROR at //build/config/ios/ios_sdk.gni:98:21: Script returned non-zero exit code. _ios_sdk_result = exec_script(script_name, ios_sdk_info_args, "scope") ^---------- Current dir: /Users/sdefresne/Developer/chromium/src/out/default/ Command: python -- /Users/sdefresne/Developer/chromium/src/build/config/mac/sdk_info.py --developer_dir /Users/sdefresne/Developer/chromium/src/build/mac_files/Xcode.app iphonesimulator Returned 1. stderr: xcodebuild: error: SDK "iphonesimulator" cannot be located. xcodebuild: error: SDK "iphonesimulator" cannot be located. xcrun: error: unable to lookup item 'Path' in SDK 'iphonesimulator' The was not caught by the bots because they do not set FORCE_MAC_TOOLCHAIN and are not corporate machine..
Message was sent while issue was closed.
Description was changed from ========== Turn on hermetic toolchain for all corp machines. BUG=659726 Committed: https://crrev.com/7778e931dabc1fa85d0b77321e3fdef166e0b6a0 Cr-Commit-Position: refs/heads/master@{#430376} ========== to ========== Turn on hermetic toolchain for all corp machines. BUG=659726 Committed: https://crrev.com/7778e931dabc1fa85d0b77321e3fdef166e0b6a0 Cr-Commit-Position: refs/heads/master@{#430376} ==========
dpranke: Please review. Updated the CL so that hermetic toolchain isn't used by default if target_os == "ios".
lgtm
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2445993004/#ps40001 (title: "Don't use hermetic toolchain on iOS.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by erikchen@chromium.org
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 ========== Turn on hermetic toolchain for all corp machines. BUG=659726 Committed: https://crrev.com/7778e931dabc1fa85d0b77321e3fdef166e0b6a0 Cr-Commit-Position: refs/heads/master@{#430376} ========== to ========== Turn on hermetic toolchain for all corp machines. BUG=659726 Committed: https://crrev.com/7778e931dabc1fa85d0b77321e3fdef166e0b6a0 Cr-Commit-Position: refs/heads/master@{#430376} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Turn on hermetic toolchain for all corp machines. BUG=659726 Committed: https://crrev.com/7778e931dabc1fa85d0b77321e3fdef166e0b6a0 Cr-Commit-Position: refs/heads/master@{#430376} ========== to ========== Turn on hermetic toolchain for all corp machines. BUG=659726 Committed: https://crrev.com/7778e931dabc1fa85d0b77321e3fdef166e0b6a0 Committed: https://crrev.com/da01626aa3c2e1533c53c325de9bd579f0aa54d2 Cr-Original-Commit-Position: refs/heads/master@{#430376} Cr-Commit-Position: refs/heads/master@{#430863} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/da01626aa3c2e1533c53c325de9bd579f0aa54d2 Cr-Commit-Position: refs/heads/master@{#430863}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2500483002/ by kjellander@chromium.org. The reason for reverting is: Breaks compilation in WebRTC since we use the same toolchain (but needs 10.12 SDK which is not available in the hermetic toolchain). See https://bugs.chromium.org/p/webrtc/issues/detail?id=6700#c1. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
