Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1699273002:
Add a script to build and copy the command buffer shared library. This script will be used by the b… (Closed)
DescriptionAdd a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1699273002
NOTREECHECKS=true
NOTRY=true
BUG=skia:4957
Committed: https://skia.googlesource.com/skia/+/c43f2af60e11c3b09aa70725e758ae0e455f065d
Patch Set 1 #Patch Set 2 : update #Patch Set 3 : whitespace #
Total comments: 10
Patch Set 4 : cleanup #Patch Set 5 : more #
Total comments: 8
Patch Set 6 : update #Messages
Total messages: 15 (7 generated)
Description was changed from ========== Add a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer. ========== to ========== Add a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
bsalomon@google.com changed reviewers: + borenet@google.com
I believe we'd want the bot to run: ./tools/build_command_buffer.py -c <some_dir> -o out/Debug/lib --make-output-dir -f
https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... File tools/build_command_buffer.py (right): https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... tools/build_command_buffer.py:16: Style nits: 2 lines between top-level blocks, 2-space indent except for line breaks which are 4. https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... tools/build_command_buffer.py:21: 'path to Chromium checkout (directory containing .gclient)') The "-src" is a little misleading since that's also the name of the directory within --chrome-src. https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... tools/build_command_buffer.py:45: os.mkdir(args.chrome_src) Probably want os.makedirs here, which is equivalent to "mkdir -p" vs just "mkdir" https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... tools/build_command_buffer.py:49: if os.system('fetch chromium'): This would be safer and avoid the chdir: subprocess.check_call(['fetch', 'chromium'], cwd=args.chrome_src) The above throws an exception if the command fails, so you can either catch it and print an error message or just let the exception propagate. https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... tools/build_command_buffer.py:55: sys.exit(args.chrome_src + '/src' + ' does not exist') Use os.path.join(args.chrome_src, 'src'), here and below.
PTAL https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... File tools/build_command_buffer.py (right): https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... tools/build_command_buffer.py:16: On 2016/02/17 12:26:14, borenet wrote: > Style nits: 2 lines between top-level blocks, 2-space indent except for line > breaks which are 4. Done. https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... tools/build_command_buffer.py:21: 'path to Chromium checkout (directory containing .gclient)') On 2016/02/17 12:26:15, borenet wrote: > The "-src" is a little misleading since that's also the name of the directory > within --chrome-src. Changed to chrome-dir https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... tools/build_command_buffer.py:45: os.mkdir(args.chrome_src) On 2016/02/17 12:26:14, borenet wrote: > Probably want os.makedirs here, which is equivalent to "mkdir -p" vs just > "mkdir" Done. https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... tools/build_command_buffer.py:49: if os.system('fetch chromium'): On 2016/02/17 12:26:14, borenet wrote: > This would be safer and avoid the chdir: > > subprocess.check_call(['fetch', 'chromium'], cwd=args.chrome_src) > > The above throws an exception if the command fails, so you can either catch it > and print an error message or just let the exception propagate. Done. https://codereview.chromium.org/1699273002/diff/40001/tools/build_command_buf... tools/build_command_buffer.py:55: sys.exit(args.chrome_src + '/src' + ' does not exist') On 2016/02/17 12:26:14, borenet wrote: > Use os.path.join(args.chrome_src, 'src'), here and below. Done.
Looks mostly good, a couple of comments. https://codereview.chromium.org/1699273002/diff/80001/tools/build_command_buf... File tools/build_command_buffer.py (right): https://codereview.chromium.org/1699273002/diff/80001/tools/build_command_buf... tools/build_command_buffer.py:50: if not os.path.isdir(args.output_dir): Not sure it's really necessary to check, but this should probably be isfile, right? https://codereview.chromium.org/1699273002/diff/80001/tools/build_command_buf... tools/build_command_buffer.py:73: error.cmd + '" in ' + args.chrome_dir) Format strings would be better here: sys.exit('Error (ret code: %d) calling "%s" in %s' % (error.returncode, error.cmd, args.chrome_dir)) https://codereview.chromium.org/1699273002/diff/80001/tools/build_command_buf... tools/build_command_buffer.py:110: subprocess.check_call(['ninja', args.extra_ninja_args, '-C', You might need to do (['ninja'] + shlex.split(args.extra_ninja_args) + ['-C, ...]) in case extra_ninja_args contains more than one argument. https://codereview.chromium.org/1699273002/diff/80001/tools/build_command_buf... tools/build_command_buffer.py:133: main() Nit: indent
https://codereview.chromium.org/1699273002/diff/80001/tools/build_command_buf... File tools/build_command_buffer.py (right): https://codereview.chromium.org/1699273002/diff/80001/tools/build_command_buf... tools/build_command_buffer.py:50: if not os.path.isdir(args.output_dir): On 2016/02/17 16:31:17, borenet wrote: > Not sure it's really necessary to check, but this should probably be isfile, > right? yep https://codereview.chromium.org/1699273002/diff/80001/tools/build_command_buf... tools/build_command_buffer.py:73: error.cmd + '" in ' + args.chrome_dir) On 2016/02/17 16:31:16, borenet wrote: > Format strings would be better here: > > sys.exit('Error (ret code: %d) calling "%s" in %s' % (error.returncode, > error.cmd, args.chrome_dir)) Done. https://codereview.chromium.org/1699273002/diff/80001/tools/build_command_buf... tools/build_command_buffer.py:110: subprocess.check_call(['ninja', args.extra_ninja_args, '-C', On 2016/02/17 16:31:17, borenet wrote: > You might need to do (['ninja'] + shlex.split(args.extra_ninja_args) + ['-C, > ...]) in case extra_ninja_args contains more than one argument. Done. https://codereview.chromium.org/1699273002/diff/80001/tools/build_command_buf... tools/build_command_buffer.py:133: main() On 2016/02/17 16:31:17, borenet wrote: > Nit: indent Done.
LGTM
Description was changed from ========== Add a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... NOTREECHECKS=true NOTRY=true ==========
Description was changed from ========== Add a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... NOTREECHECKS=true NOTRY=true ========== to ========== Add a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... NOTREECHECKS=true NOTRY=true BUG=skia:4957 ==========
The CQ bit was checked by bsalomon@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699273002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699273002/100001
Message was sent while issue was closed.
Description was changed from ========== Add a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... NOTREECHECKS=true NOTRY=true BUG=skia:4957 ========== to ========== Add a script to build and copy the command buffer shared library. This script will be used by the bots to test skia on the command buffer. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... NOTREECHECKS=true NOTRY=true BUG=skia:4957 Committed: https://skia.googlesource.com/skia/+/c43f2af60e11c3b09aa70725e758ae0e455f065d ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/c43f2af60e11c3b09aa70725e758ae0e455f065d |
