Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1991873003:
[Mac/GN] Setup Keystone for official Chrome builds. (Closed)
|
Created:
4 years, 7 months ago by Robert Sesek Modified:
4 years, 7 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] Set up Keystone for official Chrome builds.
BUG=431177
R=mark@chromium.org
Committed: https://crrev.com/8c741818ec4296f8b8f889671e930be4dd926b59
Cr-Commit-Position: refs/heads/master@{#394510}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Address comments #Patch Set 3 : python #
Messages
Total messages: 18 (7 generated)
Description was changed from ========== [Mac/GN] Setup Keystone for official builds. BUG=431177 R=mark@chromium.org ========== to ========== [Mac/GN] Setup Keystone for official Chrome builds. BUG=431177 R=mark@chromium.org ==========
https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... File chrome/tools/build/mac/copy_keystone_framework.py (right): https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. #!/usr/bin/env python https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:27: shutil.copytree(os.path.join(args[1], 'Versions/A'), output_path) os.readlink(os.path.join(args[1], 'Versions/Current')) and use Versions/that_thing instead of hard-coding A. https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:29: # Strip headers from the shipped product. Lose PrivateHeaders too, unless it’s an error to rmtree something that’s not currently there. I’m also worried about SCM garbage and .DS_Store files. copytree doesn’t give you much control, can you run a rsync here? You can tell it what you want to exclude. https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:34: return subprocess.check_call( check_call won’t return nonzero, but hiding that zero in the return doesn’t make this very obvious. I’d just have a separate “return 0” after this.
https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... File chrome/tools/build/mac/copy_keystone_framework.py (right): https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/05/18 17:53:10, Mark Mentovai wrote: > #!/usr/bin/env python This isn't done for GN scripts because GN explicitly invokes `python ${script}` (an action can't invoke a shell script, only Python). https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:27: shutil.copytree(os.path.join(args[1], 'Versions/A'), output_path) On 2016/05/18 17:53:10, Mark Mentovai wrote: > os.readlink(os.path.join(args[1], 'Versions/Current')) and use > Versions/that_thing instead of hard-coding A. Switched to using rsync and Versions/Current/ so don't need to directly readlink. https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:29: # Strip headers from the shipped product. On 2016/05/18 17:53:10, Mark Mentovai wrote: > Lose PrivateHeaders too, unless it’s an error to rmtree something that’s not > currently there. > > I’m also worried about SCM garbage and .DS_Store files. copytree doesn’t give > you much control, can you run a rsync here? You can tell it what you want to > exclude. Switched to rsync using the same incantation as copy_framework_unversioned.sh. https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:34: return subprocess.check_call( On 2016/05/18 17:53:11, Mark Mentovai wrote: > check_call won’t return nonzero, but hiding that zero in the return doesn’t make > this very obvious. I’d just have a separate “return 0” after this. Done.
LGTM https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... File chrome/tools/build/mac/copy_keystone_framework.py (right): https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/05/18 18:36:25, Robert Sesek wrote: > On 2016/05/18 17:53:10, Mark Mentovai wrote: > > #!/usr/bin/env python > > This isn't done for GN scripts because GN explicitly invokes `python ${script}` > (an action can't invoke a shell script, only Python). Then put python in the usage on line 10, because it won’t work as written otherwise. https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:27: shutil.copytree(os.path.join(args[1], 'Versions/A'), output_path) Robert Sesek wrote: > On 2016/05/18 17:53:10, Mark Mentovai wrote: > > os.readlink(os.path.join(args[1], 'Versions/Current')) and use > > Versions/that_thing instead of hard-coding A. > > Switched to using rsync and Versions/Current/ so don't need to directly > readlink. Cool. The old script needed to readlink for reasons that don’t apply here. Reading directly through Current is fine.
https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... File chrome/tools/build/mac/copy_keystone_framework.py (right): https://codereview.chromium.org/1991873003/diff/1/chrome/tools/build/mac/copy... chrome/tools/build/mac/copy_keystone_framework.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/05/18 18:40:11, Mark Mentovai wrote: > On 2016/05/18 18:36:25, Robert Sesek wrote: > > On 2016/05/18 17:53:10, Mark Mentovai wrote: > > > #!/usr/bin/env python > > > > This isn't done for GN scripts because GN explicitly invokes `python > ${script}` > > (an action can't invoke a shell script, only Python). > > Then put python in the usage on line 10, because it won’t work as written > otherwise. Done.
rsesek@chromium.org changed reviewers: + thakis@chromium.org
+thakis for OWNERS
rs-lgtm based on mark's review As far as I know "setup" is the noun while "set up" is the verb, so maybe add a ' ' to the cl description.
Description was changed from ========== [Mac/GN] Setup Keystone for official Chrome builds. BUG=431177 R=mark@chromium.org ========== to ========== [Mac/GN] Set up Keystone for official Chrome builds. BUG=431177 R=mark@chromium.org ==========
On 2016/05/18 18:49:30, Nico (vacation May 19 to 22) wrote: > rs-lgtm based on mark's review > > As far as I know "setup" is the noun while "set up" is the verb, so maybe add a > ' ' to the cl description. Done.
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 Link to the patchset: https://codereview.chromium.org/1991873003/#ps40001 (title: "python")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1991873003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1991873003/40001
Message was sent while issue was closed.
Description was changed from ========== [Mac/GN] Set up Keystone for official Chrome builds. BUG=431177 R=mark@chromium.org ========== to ========== [Mac/GN] Set up Keystone for official Chrome builds. BUG=431177 R=mark@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Mac/GN] Set up Keystone for official Chrome builds. BUG=431177 R=mark@chromium.org ========== to ========== [Mac/GN] Set up Keystone for official Chrome builds. BUG=431177 R=mark@chromium.org Committed: https://crrev.com/8c741818ec4296f8b8f889671e930be4dd926b59 Cr-Commit-Position: refs/heads/master@{#394510} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8c741818ec4296f8b8f889671e930be4dd926b59 Cr-Commit-Position: refs/heads/master@{#394510} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
