Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1759793003:
Scripts to upload and update the mac toolchain. (Closed)
|
Created:
4 years, 9 months ago by justincohen Modified:
4 years, 8 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. |
DescriptionScripts to upload and update the mac toolchain.
The following two scripts allow for the update and roll of the mac
toolchain (Xcode) via DEPS instead of thru an infrastructure install
script. The upload / download nature of the script follows somewhat
what Windows does with visual studio files and what Mac already does
for clang rolls.
build/package_mac_toolchain.py takes an Xcode.app directory
and packages up what the parts the Mac build uses into a tar file
and uploads it to gs://chrome-mac-sdk/
build/mac_toolchain.py runs as part of hooks, downloads
and decompresses the mac toolchain, and points system files to this
new directory with the DEVELOPER_DIR environment variable.
Setting DEVELOPER_DIR throughout gyp-ninja requires this GYP CL:
https://codereview.chromium.org/1806733002/
BUG=474373
Committed: https://crrev.com/6a03a3dc9196b4474e7f5c08803f076a95055b44
Cr-Commit-Position: refs/heads/master@{#383461}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Use full path #
Total comments: 28
Patch Set 4 : Tighten up scripts #Patch Set 5 : nits #Patch Set 6 : Change data file name #
Total comments: 1
Patch Set 7 : More comments and sudoers check #Patch Set 8 : Add dev version to comments #Patch Set 9 : Comment typo #
Total comments: 5
Patch Set 10 : Ensure DownloadAndUnpack cleans up itself #
Total comments: 9
Patch Set 11 : Added per-file OWNERS #Patch Set 12 : Set DEVELOPER_DIR #Patch Set 13 : Use DEVELOPER_DIR flag instead #Patch Set 14 : Accept Xcode license #
Total comments: 6
Patch Set 15 : Address Erik comments #Patch Set 16 : Only run on mac #
Total comments: 10
Patch Set 17 : Roll to Xcode 7.3, address scottmg comments #
Total comments: 1
Patch Set 18 : Roll back to Xcode to 5.1.1 #Patch Set 19 : Roll to Xcode 5.1.1 with 10.10 SDK #
Total comments: 10
Patch Set 20 : Address Nico comments #Patch Set 21 : Change call to gyp_chromium from environment variable to param #Patch Set 22 : nit #
Messages
Total messages: 41 (9 generated)
https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... File third_party/mac_sdk/update.py (right): https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:60: def main(): I removed the GYP_DEFINE check, that needs to be re-added. As does an IsGoogler check. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:82: print 'Setting xcode-select' This is wrong, as I still want to run xcode-select if the SDK is already up to date.
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/1759793003/diff/40001/DEPS File DEPS (right): https://codereview.chromium.org/1759793003/diff/40001/DEPS#newcode614 DEPS:614: 'name': 'mac_sdk', technically, we're pulling a partial toolchain, not just an sdk. https://codereview.chromium.org/1759793003/diff/40001/DEPS#newcode616 DEPS:616: 'action': ['sudo', 'python', 'src/third_party/mac_sdk/update.py'], Is root really required, or is this for convenience for testing? https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... File third_party/mac_sdk/package.py (right): https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... third_party/mac_sdk/package.py:23: EXCLUDE_FOLDERS = [ This will need to be manually updated with each roll? I guess it won't be necessary if we only need SDKs, right? How did you decide on these folders? Let's say Xcode8/10.12 SDK comes out. How am I going to figure out what to add/remove from this list? https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... third_party/mac_sdk/package.py:39: print 'Adding', tarinfo.name I'm not a python guru, but I think it's good practice to use the logging module? Also this is never used. Based on name of function, I wouldn't expect a return value. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... third_party/mac_sdk/package.py:59: p = subprocess.Popen(['gsutil.py', '--force-version', '4.15', '--', 'ls', why --force-version? https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... third_party/mac_sdk/package.py:77: sdk_name = tempfile.mktemp(suffix='sdk.tgz') caller is responsible for cleaning up directory afterwards? https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... third_party/mac_sdk/package.py:80: args = ['tar', '-cvpzf', sdk_name] preserve permissions? How does that work when you untar as a different user on a different machine? Should we just manually fix all permissions on the other end? https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... File third_party/mac_sdk/update.py (right): https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:6: """This script is used to download prebuilt clang binaries.""" out of date. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:24: SDK_URL = 'gs://chrome-mac-sdk/' this url could be shared with other file? https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:50: temp_name = tempfile.mktemp(prefix='mac_sdk') clean up temp file. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:55: if url.endswith('.zip'): shouldn't we control the extension? why not standardize on zip or gzip? https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:60: def main(): On 2016/03/03 01:24:54, justincohen wrote: > I removed the GYP_DEFINE check, that needs to be re-added. > > As does an IsGoogler check. as does checking if xcode already exists (or whatever we're doing to no-op on dev machines)
Going to add some more comments and then shop this around to infra and others. https://codereview.chromium.org/1759793003/diff/40001/DEPS File DEPS (right): https://codereview.chromium.org/1759793003/diff/40001/DEPS#newcode614 DEPS:614: 'name': 'mac_sdk', On 2016/03/03 01:45:46, erikchen wrote: > technically, we're pulling a partial toolchain, not just an sdk. Renamed to mac_toolchain. https://codereview.chromium.org/1759793003/diff/40001/DEPS#newcode616 DEPS:616: 'action': ['sudo', 'python', 'src/third_party/mac_sdk/update.py'], On 2016/03/03 01:45:46, erikchen wrote: > Is root really required, or is this for convenience for testing? Mistake from testing, removed. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... File third_party/mac_sdk/package.py (right): https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... third_party/mac_sdk/package.py:23: EXCLUDE_FOLDERS = [ On 2016/03/03 01:45:47, erikchen wrote: > This will need to be manually updated with each roll? I guess it won't be > necessary if we only need SDKs, right? > > How did you decide on these folders? Let's say Xcode8/10.12 SDK comes out. How > am I going to figure out what to add/remove from this list? Added a comment explaining. /Platforms!MacOsx gives the most bang for your buck. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... third_party/mac_sdk/package.py:39: print 'Adding', tarinfo.name On 2016/03/03 01:45:47, erikchen wrote: > I'm not a python guru, but I think it's good practice to use the logging module? > Also this is never used. Based on name of function, I wouldn't expect a return > value. Removed. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... third_party/mac_sdk/package.py:59: p = subprocess.Popen(['gsutil.py', '--force-version', '4.15', '--', 'ls', On 2016/03/03 01:45:46, erikchen wrote: > why --force-version? Removed. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... third_party/mac_sdk/package.py:77: sdk_name = tempfile.mktemp(suffix='sdk.tgz') On 2016/03/03 01:45:47, erikchen wrote: > caller is responsible for cleaning up directory afterwards? Done. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/pac... third_party/mac_sdk/package.py:80: args = ['tar', '-cvpzf', sdk_name] On 2016/03/03 01:45:47, erikchen wrote: > preserve permissions? How does that work when you untar as a different user on a > different machine? Should we just manually fix all permissions on the other end? Removed 'p' https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... File third_party/mac_sdk/update.py (right): https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:6: """This script is used to download prebuilt clang binaries.""" On 2016/03/03 01:45:47, erikchen wrote: > out of date. Done. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:24: SDK_URL = 'gs://chrome-mac-sdk/' On 2016/03/03 01:45:47, erikchen wrote: > this url could be shared with other file? Holding off on this change now to determine what else needs can be shared https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:50: temp_name = tempfile.mktemp(prefix='mac_sdk') On 2016/03/03 01:45:47, erikchen wrote: > clean up temp file. Done. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:55: if url.endswith('.zip'): On 2016/03/03 01:45:47, erikchen wrote: > shouldn't we control the extension? why not standardize on zip or gzip? Done. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:60: def main(): On 2016/03/03 01:24:54, justincohen wrote: > I removed the GYP_DEFINE check, that needs to be re-added. > > As does an IsGoogler check. re-added. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:60: def main(): On 2016/03/03 01:45:47, erikchen wrote: > On 2016/03/03 01:24:54, justincohen wrote: > > I removed the GYP_DEFINE check, that needs to be re-added. > > > > As does an IsGoogler check. > > as does checking if xcode already exists (or whatever we're doing to no-op on > dev machines) added. https://codereview.chromium.org/1759793003/diff/40001/third_party/mac_sdk/upd... third_party/mac_sdk/update.py:82: print 'Setting xcode-select' On 2016/03/03 01:24:54, justincohen wrote: > This is wrong, as I still want to run xcode-select if the SDK is already up to > date. Done.
Overall, this looks pretty reasonable. Let me know when you want a full review. https://codereview.chromium.org/1759793003/diff/100001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/100001/build/mac_toolchain.py... build/mac_toolchain.py:96: SwitchToolchain(TOOLCHAIN_BUILD_DIR) if the toolchain is already up to date, why do we need to switch toolchains?
Description was changed from ========== Proof Try Two. BUG= ========== to ========== Scripts to upload and update the mac toolchain. The following two scripts allow for the update and roll of the mac toolchain (Xcode) via DEPS instead of thru an infrastructure install script. The upload / download nature of the script follows somewhat what Windows does with visual studio files and what Mac already does for clang rolls. build/package_mac_toolchain.py takes an Xcode.app directory and packages up what the parts the Mac build uses into a tar file and uploads it to gs://chrome-mac-sdk/ build/mac_toolchain.py runs as part of hooks, downloads and decompresses the mac toolchain, and points system files to this new directory with xcode-select. BUG= ==========
Description was changed from ========== Scripts to upload and update the mac toolchain. The following two scripts allow for the update and roll of the mac toolchain (Xcode) via DEPS instead of thru an infrastructure install script. The upload / download nature of the script follows somewhat what Windows does with visual studio files and what Mac already does for clang rolls. build/package_mac_toolchain.py takes an Xcode.app directory and packages up what the parts the Mac build uses into a tar file and uploads it to gs://chrome-mac-sdk/ build/mac_toolchain.py runs as part of hooks, downloads and decompresses the mac toolchain, and points system files to this new directory with xcode-select. BUG= ========== to ========== Scripts to upload and update the mac toolchain. The following two scripts allow for the update and roll of the mac toolchain (Xcode) via DEPS instead of thru an infrastructure install script. The upload / download nature of the script follows somewhat what Windows does with visual studio files and what Mac already does for clang rolls. build/package_mac_toolchain.py takes an Xcode.app directory and packages up what the parts the Mac build uses into a tar file and uploads it to gs://chrome-mac-sdk/ build/mac_toolchain.py runs as part of hooks, downloads and decompresses the mac toolchain, and points system files to this new directory with xcode-select. BUG=474373 ==========
justincohen@chromium.org changed reviewers: + phajdan.jr@chromium.org, scottmg@chromium.org, smut@google.com, thakis@chromium.org
For your consideration This hasn't had a chance to run on trybots yet, but the script is self contained enough as it stands that it could use more eyes. PTAL!
In general looks fine to me. Leaving final review specifically to Mac folks. https://codereview.chromium.org/1759793003/diff/160001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/160001/build/mac_toolchain.py... build/mac_toolchain.py:2: # Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: 2016, no (c) https://codereview.chromium.org/1759793003/diff/160001/build/mac_toolchain.py... build/mac_toolchain.py:80: os.unlink(temp_name) Shouldn't this be in try-finally? https://codereview.chromium.org/1759793003/diff/160001/build/mac_toolchain.py... build/mac_toolchain.py:150: print 'Exiting.' Whoa, that seems to totally hide what was the exception. Please make sure to print it for debugging.
https://codereview.chromium.org/1759793003/diff/160001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/160001/build/mac_toolchain.py... build/mac_toolchain.py:2: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/03/04 07:57:09, Paweł Hajdan Jr. wrote: > nit: 2016, no (c) Done. https://codereview.chromium.org/1759793003/diff/160001/build/mac_toolchain.py... build/mac_toolchain.py:80: os.unlink(temp_name) On 2016/03/04 07:57:10, Paweł Hajdan Jr. wrote: > Shouldn't this be in try-finally? Good catch, Done.
Please add erikchen and you as per-file owners for this. Does this xcode-select the downloaded toolchain globally for the whole system? Is there a way to only select it for chromium builds? https://codereview.chromium.org/1759793003/diff/180001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/180001/build/mac_toolchain.py... build/mac_toolchain.py:98: subprocess.check_call(['sudo', '/usr/bin/xcode-select', '-s', directory]) won't this require an auth password usually? https://codereview.chromium.org/1759793003/diff/180001/build/mac_toolchain.py... build/mac_toolchain.py:103: gyp_defines = gyp.NameValueListToDict(gyp.ShlexEnv('GYP_DEFINES')) (https://bugs.chromium.org/p/chromium/issues/detail?id=570091 tracks on what to do here wrt gn) https://codereview.chromium.org/1759793003/diff/180001/build/mac_toolchain.py... build/mac_toolchain.py:113: if not force_pull and rc == 0 and TOOLCHAIN_BUILD_DIR not in xcode_select_dir: should this check that the local toolchain has the right version? as-is, from what i understand this won't do anything if xcode 3.2 is installed, which seems suboptimal https://codereview.chromium.org/1759793003/diff/180001/build/package_mac_tool... File build/package_mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/180001/build/package_mac_tool... build/package_mac_toolchain.py:24: # problematic it's possibel this list is incorrect, and can be reduced to just *possible
xcode-select is system wide. We could also set DEVELOPER_DIR, but that's also system wide as long as the environment variable is set. https://codereview.chromium.org/1759793003/diff/180001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/180001/build/mac_toolchain.py... build/mac_toolchain.py:98: subprocess.check_call(['sudo', '/usr/bin/xcode-select', '-s', directory]) On 2016/03/08 18:48:46, Nico wrote: > won't this require an auth password usually? Correct. We check below that (sudo -l) to make sure we can sudo for xcode-select and xcodebuild. The bots can be configured to allow this without entering a password. https://codereview.chromium.org/1759793003/diff/180001/build/mac_toolchain.py... build/mac_toolchain.py:103: gyp_defines = gyp.NameValueListToDict(gyp.ShlexEnv('GYP_DEFINES')) On 2016/03/08 18:48:46, Nico wrote: > (https://bugs.chromium.org/p/chromium/issues/detail?id=570091 tracks on what to > do here wrt gn) Added a TODO https://codereview.chromium.org/1759793003/diff/180001/build/mac_toolchain.py... build/mac_toolchain.py:113: if not force_pull and rc == 0 and TOOLCHAIN_BUILD_DIR not in xcode_select_dir: On 2016/03/08 18:48:46, Nico wrote: > should this check that the local toolchain has the right version? as-is, from > what i understand this won't do anything if xcode 3.2 is installed, which seems > suboptimal I wasn't sure what the right functionality should be here. Since xcode-select will be system wide, last beyond reboot, etc. I didn't want to break old system. Ideally bots that are ready for this will set the GYP_DEFINE force_mac_toolchain. https://codereview.chromium.org/1759793003/diff/180001/build/mac_toolchain.py... build/mac_toolchain.py:128: print 'xcode-select and xcodebuild must be present in sudoers.' Missed a return 0 here. https://codereview.chromium.org/1759793003/diff/180001/build/package_mac_tool... File build/package_mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/180001/build/package_mac_tool... build/package_mac_toolchain.py:24: # problematic it's possibel this list is incorrect, and can be reduced to just On 2016/03/08 18:48:46, Nico wrote: > *possible Done.
would it be enough to set DEVELOPER_DIR in the env of the gyp / gn subprocess and then have those generate paths relative to that?
Not as long as we call xcrun ibtool from ninja's mac-tool.py
but that could expand to DEVELOPER_DIR/ibtool if that's set, no?
Yeah, that could work. It would look more like: env DEVELOPER_DIR=foo xcrun ibtool... That means this would require GYP/GN changes. Want me to go down that path?
The script would still require calling xcodebuild -license accept with sudo, FWIW.
On 2016/03/08 20:09:28, justincohen wrote: > Yeah, that could work. It would look more like: > > env DEVELOPER_DIR=foo xcrun ibtool... > > That means this would require GYP/GN changes. Want me to go down that path? Yes, I think that'd be good.
Is there an existing pipe for taking a return value from a DEPS hook and passing that to src/build/gyp_chromium?
The way this works on windows is that the toolchain downloader writes toolchain info to some well-known directory and then gyp/gn load it from there. I think I sent you links to the relevant scripts some time last week.
Description was changed from ========== Scripts to upload and update the mac toolchain. The following two scripts allow for the update and roll of the mac toolchain (Xcode) via DEPS instead of thru an infrastructure install script. The upload / download nature of the script follows somewhat what Windows does with visual studio files and what Mac already does for clang rolls. build/package_mac_toolchain.py takes an Xcode.app directory and packages up what the parts the Mac build uses into a tar file and uploads it to gs://chrome-mac-sdk/ build/mac_toolchain.py runs as part of hooks, downloads and decompresses the mac toolchain, and points system files to this new directory with xcode-select. BUG=474373 ========== to ========== Scripts to upload and update the mac toolchain. The following two scripts allow for the update and roll of the mac toolchain (Xcode) via DEPS instead of thru an infrastructure install script. The upload / download nature of the script follows somewhat what Windows does with visual studio files and what Mac already does for clang rolls. build/package_mac_toolchain.py takes an Xcode.app directory and packages up what the parts the Mac build uses into a tar file and uploads it to gs://chrome-mac-sdk/ build/mac_toolchain.py runs as part of hooks, downloads and decompresses the mac toolchain, and points system files to this new directory with the DEVELOPER_DIR environment variable. Setting DEVELOPER_DIR throughout gyp-ninja requires this GYP CL: https://codereview.chromium.org/1806733002/ BUG=474373 ==========
Latest CL removes the need to change xcode-select, but now requires a gyp change, and still requires sudo for accepting the xcode license.
nits, but still looks good https://codereview.chromium.org/1759793003/diff/260001/build/gyp_chromium.py File build/gyp_chromium.py (right): https://codereview.chromium.org/1759793003/diff/260001/build/gyp_chromium.py#... build/gyp_chromium.py:20: import mac_toolchain sort lexically, and in other files. https://codereview.chromium.org/1759793003/diff/260001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/260001/build/mac_toolchain.py... build/mac_toolchain.py:15: /usr/bin/xcode-select do we still need sudo for xcode-select? https://codereview.chromium.org/1759793003/diff/260001/build/mac_toolchain.py... build/mac_toolchain.py:78: temp_name = tempfile.mktemp(prefix='mac_toolchain') move this line to 76. Otherwise if tempfile.mktemp raises an exception, the finally: clause will also raise an exception.
https://codereview.chromium.org/1759793003/diff/260001/build/gyp_chromium.py File build/gyp_chromium.py (right): https://codereview.chromium.org/1759793003/diff/260001/build/gyp_chromium.py#... build/gyp_chromium.py:20: import mac_toolchain On 2016/03/15 20:31:26, erikchen wrote: > sort lexically, and in other files. Done. https://codereview.chromium.org/1759793003/diff/260001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/260001/build/mac_toolchain.py... build/mac_toolchain.py:15: /usr/bin/xcode-select On 2016/03/15 20:31:26, erikchen wrote: > do we still need sudo for xcode-select? Nope, updated the comment. https://codereview.chromium.org/1759793003/diff/260001/build/mac_toolchain.py... build/mac_toolchain.py:78: temp_name = tempfile.mktemp(prefix='mac_toolchain') On 2016/03/15 20:31:26, erikchen wrote: > move this line to 76. Otherwise if tempfile.mktemp raises an exception, the > finally: clause will also raise an exception. Done.
Looks fine to me. We should probably make Windows simpler like this (stamp rather than the hash, as it's not really useful any more.) https://codereview.chromium.org/1759793003/diff/300001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/300001/build/mac_toolchain.py... build/mac_toolchain.py:96: +\n https://codereview.chromium.org/1759793003/diff/300001/build/mac_toolchain.py... build/mac_toolchain.py:119: if sys.platform != "darwin": Prefer ' over " (and a couple more below). https://codereview.chromium.org/1759793003/diff/300001/build/package_mac_tool... File build/package_mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/300001/build/package_mac_tool... build/package_mac_toolchain.py:2: # Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c) https://codereview.chromium.org/1759793003/diff/300001/build/package_mac_tool... build/package_mac_toolchain.py:49: parser.add_argument('args', nargs='*', help="run ibtool command") This looks unused (and maybe has the wrong help= if not?) https://codereview.chromium.org/1759793003/diff/300001/build/package_mac_tool... build/package_mac_toolchain.py:78: extra \n here
https://codereview.chromium.org/1759793003/diff/300001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/300001/build/mac_toolchain.py... build/mac_toolchain.py:96: On 2016/03/21 20:42:20, scottmg wrote: > +\n Done. https://codereview.chromium.org/1759793003/diff/300001/build/mac_toolchain.py... build/mac_toolchain.py:119: if sys.platform != "darwin": On 2016/03/21 20:42:20, scottmg wrote: > Prefer ' over " (and a couple more below). Done. https://codereview.chromium.org/1759793003/diff/300001/build/package_mac_tool... File build/package_mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/300001/build/package_mac_tool... build/package_mac_toolchain.py:2: # Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/03/21 20:42:20, scottmg wrote: > No (c) Done. https://codereview.chromium.org/1759793003/diff/300001/build/package_mac_tool... build/package_mac_toolchain.py:49: parser.add_argument('args', nargs='*', help="run ibtool command") On 2016/03/21 20:42:21, scottmg wrote: > This looks unused (and maybe has the wrong help= if not?) Done. https://codereview.chromium.org/1759793003/diff/300001/build/package_mac_tool... build/package_mac_toolchain.py:78: On 2016/03/21 20:42:21, scottmg wrote: > extra \n here Done.
https://codereview.chromium.org/1759793003/diff/320001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/320001/build/mac_toolchain.py... build/mac_toolchain.py:36: TOOLCHAIN_REVISION = '7D175' What version of Xcode is this? How did you get this number? The official build environment uses Xcode 5.1.1. Are there any problems with using that as the toolchain? [I'm worried about running newer versions of Xcode on OSX 10.9 machines]
On 2016/03/22 17:51:39, erikchen wrote: > https://codereview.chromium.org/1759793003/diff/320001/build/mac_toolchain.py > File build/mac_toolchain.py (right): > > https://codereview.chromium.org/1759793003/diff/320001/build/mac_toolchain.py... > build/mac_toolchain.py:36: TOOLCHAIN_REVISION = '7D175' > What version of Xcode is this? How did you get this number? > > The official build environment uses Xcode 5.1.1. Are there any problems with > using that as the toolchain? > > [I'm worried about running newer versions of Xcode on OSX 10.9 machines] I'm pretty sure this doesn't work on OS X 10.9. :( """ To proceed, enter your password, or type Ctrl-C to abort. Password: dyld: Symbol not found: _kSZArchiverOptionNoCentralDirectory Referenced from: /Users/chrometest/projects/chromium/src/build/mac_files/Xcode.app/Contents/SharedFrameworks/DVTFoundation.framework/Versions/A/../../..//DVTInstrumentsFoundation.framework/Versions/A/DVTInstrumentsFoundation Expected in: /System/Library/PrivateFrameworks/StreamingZip.framework/Versions/A/StreamingZip in /Users/chrometest/projects/chromium/src/build/mac_files/Xcode.app/Contents/SharedFrameworks/DVTFoundation.framework/Versions/A/../../..//DVTInstrumentsFoundation.framework/Versions/A/DVTInstrumentsFoundation Failed to download toolchain toolchain-7D175-1.tgz. Exiting. """
I haven't test it yet, but I updated the script to pull 5.1.1 instead. If you have a machine handy, want to try that?
Looks great, lgtm. Some suggestions below, mostly about using more dependency injection and fewer global variables, if you will. https://codereview.chromium.org/1759793003/diff/360001/build/gyp_chromium.py File build/gyp_chromium.py (right): https://codereview.chromium.org/1759793003/diff/360001/build/gyp_chromium.py#... build/gyp_chromium.py:326: mac_toolchain.SetEnvironment() Would it be possible to pass explicit flags to gyp for what you need instead of going through the env here? https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py... build/mac_toolchain.py:98: def SwitchToolchain(directory): Rename to AcceptLicense https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py... build/mac_toolchain.py:108: force_pull = 'force_mac_toolchain' in gyp_defines Hm, this won't work with gn. Is there an advantage for looking at GYP_DEFINES (instead of some other env var -- FORCE_MAC_TOOLCHAIN or something like that)? https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py... build/mac_toolchain.py:129: toolchain_revision = gyp_defines.get('mac_toolchain_revision', same question here https://codereview.chromium.org/1759793003/diff/360001/tools/clang/scripts/up... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/1759793003/diff/360001/tools/clang/scripts/up... tools/clang/scripts/update.py:340: mac_toolchain.SetEnvironment() I'm not sure cmake will pay attention to that env var (?) I think you can revert the change to this file for now, we can consider changing this later on. This code is only used on some clang trunk bots which currently have Xcode, and only to build clang, not chromium.
https://codereview.chromium.org/1759793003/diff/360001/build/gyp_chromium.py File build/gyp_chromium.py (right): https://codereview.chromium.org/1759793003/diff/360001/build/gyp_chromium.py#... build/gyp_chromium.py:326: mac_toolchain.SetEnvironment() On 2016/03/22 20:59:05, Nico (away until Mon Mar 27) wrote: > Would it be possible to pass explicit flags to gyp for what you need instead of > going through the env here? I could add something to args, but then will args be accessible in all the places necessary in https://codereview.chromium.org/1806733002/, the gyp side of this CL? https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py File build/mac_toolchain.py (right): https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py... build/mac_toolchain.py:98: def SwitchToolchain(directory): On 2016/03/22 20:59:05, Nico (away until Mon Mar 27) wrote: > Rename to AcceptLicense Done. https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py... build/mac_toolchain.py:108: force_pull = 'force_mac_toolchain' in gyp_defines On 2016/03/22 20:59:05, Nico (away until Mon Mar 27) wrote: > Hm, this won't work with gn. Is there an advantage for looking at GYP_DEFINES > (instead of some other env var -- FORCE_MAC_TOOLCHAIN or something like that)? Changed to environ FORCE_MAC_TOOLCHAIN https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py... build/mac_toolchain.py:129: toolchain_revision = gyp_defines.get('mac_toolchain_revision', On 2016/03/22 20:59:05, Nico (away until Mon Mar 27) wrote: > same question here Changed to environ MAC_TOOLCHAIN_REVISION https://codereview.chromium.org/1759793003/diff/360001/tools/clang/scripts/up... File tools/clang/scripts/update.py (right): https://codereview.chromium.org/1759793003/diff/360001/tools/clang/scripts/up... tools/clang/scripts/update.py:340: mac_toolchain.SetEnvironment() On 2016/03/22 20:59:05, Nico (away until Mon Mar 27) wrote: > I'm not sure cmake will pay attention to that env var (?) > > I think you can revert the change to this file for now, we can consider changing > this later on. This code is only used on some clang trunk bots which currently > have Xcode, and only to build clang, not chromium. Reverted.
LGTM
The CQ bit was checked by justincohen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/1759793003/#ps420001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1759793003/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1759793003/420001
Message was sent while issue was closed.
Description was changed from ========== Scripts to upload and update the mac toolchain. The following two scripts allow for the update and roll of the mac toolchain (Xcode) via DEPS instead of thru an infrastructure install script. The upload / download nature of the script follows somewhat what Windows does with visual studio files and what Mac already does for clang rolls. build/package_mac_toolchain.py takes an Xcode.app directory and packages up what the parts the Mac build uses into a tar file and uploads it to gs://chrome-mac-sdk/ build/mac_toolchain.py runs as part of hooks, downloads and decompresses the mac toolchain, and points system files to this new directory with the DEVELOPER_DIR environment variable. Setting DEVELOPER_DIR throughout gyp-ninja requires this GYP CL: https://codereview.chromium.org/1806733002/ BUG=474373 ========== to ========== Scripts to upload and update the mac toolchain. The following two scripts allow for the update and roll of the mac toolchain (Xcode) via DEPS instead of thru an infrastructure install script. The upload / download nature of the script follows somewhat what Windows does with visual studio files and what Mac already does for clang rolls. build/package_mac_toolchain.py takes an Xcode.app directory and packages up what the parts the Mac build uses into a tar file and uploads it to gs://chrome-mac-sdk/ build/mac_toolchain.py runs as part of hooks, downloads and decompresses the mac toolchain, and points system files to this new directory with the DEVELOPER_DIR environment variable. Setting DEVELOPER_DIR throughout gyp-ninja requires this GYP CL: https://codereview.chromium.org/1806733002/ BUG=474373 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Scripts to upload and update the mac toolchain. The following two scripts allow for the update and roll of the mac toolchain (Xcode) via DEPS instead of thru an infrastructure install script. The upload / download nature of the script follows somewhat what Windows does with visual studio files and what Mac already does for clang rolls. build/package_mac_toolchain.py takes an Xcode.app directory and packages up what the parts the Mac build uses into a tar file and uploads it to gs://chrome-mac-sdk/ build/mac_toolchain.py runs as part of hooks, downloads and decompresses the mac toolchain, and points system files to this new directory with the DEVELOPER_DIR environment variable. Setting DEVELOPER_DIR throughout gyp-ninja requires this GYP CL: https://codereview.chromium.org/1806733002/ BUG=474373 ========== to ========== Scripts to upload and update the mac toolchain. The following two scripts allow for the update and roll of the mac toolchain (Xcode) via DEPS instead of thru an infrastructure install script. The upload / download nature of the script follows somewhat what Windows does with visual studio files and what Mac already does for clang rolls. build/package_mac_toolchain.py takes an Xcode.app directory and packages up what the parts the Mac build uses into a tar file and uploads it to gs://chrome-mac-sdk/ build/mac_toolchain.py runs as part of hooks, downloads and decompresses the mac toolchain, and points system files to this new directory with the DEVELOPER_DIR environment variable. Setting DEVELOPER_DIR throughout gyp-ninja requires this GYP CL: https://codereview.chromium.org/1806733002/ BUG=474373 Committed: https://crrev.com/6a03a3dc9196b4474e7f5c08803f076a95055b44 Cr-Commit-Position: refs/heads/master@{#383461} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/6a03a3dc9196b4474e7f5c08803f076a95055b44 Cr-Commit-Position: refs/heads/master@{#383461}
Message was sent while issue was closed.
On 2016/03/23 17:12:44, justincohen wrote: > https://codereview.chromium.org/1759793003/diff/360001/build/gyp_chromium.py > File build/gyp_chromium.py (right): > > https://codereview.chromium.org/1759793003/diff/360001/build/gyp_chromium.py#... > build/gyp_chromium.py:326: mac_toolchain.SetEnvironment() > On 2016/03/22 20:59:05, Nico (away until Mon Mar 27) wrote: > > Would it be possible to pass explicit flags to gyp for what you need instead > of > > going through the env here? > > I could add something to args, but then will args be accessible in all the > places necessary in https://codereview.chromium.org/1806733002/, the gyp side of > this CL? Yes, but there aren't many. > > https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py > File build/mac_toolchain.py (right): > > https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py... > build/mac_toolchain.py:98: def SwitchToolchain(directory): > On 2016/03/22 20:59:05, Nico (away until Mon Mar 27) wrote: > > Rename to AcceptLicense > > Done. > > https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py... > build/mac_toolchain.py:108: force_pull = 'force_mac_toolchain' in gyp_defines > On 2016/03/22 20:59:05, Nico (away until Mon Mar 27) wrote: > > Hm, this won't work with gn. Is there an advantage for looking at GYP_DEFINES > > (instead of some other env var -- FORCE_MAC_TOOLCHAIN or something like that)? > > Changed to environ FORCE_MAC_TOOLCHAIN > > https://codereview.chromium.org/1759793003/diff/360001/build/mac_toolchain.py... > build/mac_toolchain.py:129: toolchain_revision = > gyp_defines.get('mac_toolchain_revision', > On 2016/03/22 20:59:05, Nico (away until Mon Mar 27) wrote: > > same question here > > Changed to environ MAC_TOOLCHAIN_REVISION > > https://codereview.chromium.org/1759793003/diff/360001/tools/clang/scripts/up... > File tools/clang/scripts/update.py (right): > > https://codereview.chromium.org/1759793003/diff/360001/tools/clang/scripts/up... > tools/clang/scripts/update.py:340: mac_toolchain.SetEnvironment() > On 2016/03/22 20:59:05, Nico (away until Mon Mar 27) wrote: > > I'm not sure cmake will pay attention to that env var (?) > > > > I think you can revert the change to this file for now, we can consider > changing > > this later on. This code is only used on some clang trunk bots which currently > > have Xcode, and only to build clang, not chromium. > > Reverted. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
