Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2155413003:
Refactoring: separate build steps in real_main for compile.py (Closed)
DescriptionRefactoring: separate build steps in real_main for compile.py
* extract goma_setup and goma_teardown from main_ninja.
* make a function get_parsed_options for options
After this CL will be committed, I will make scripts calling
function of compile.py in recipe_modules/goma to make separated build
steps.
BUG=627693
Committed: https://chromium.googlesource.com/chromium/tools/build/+/4960aa40d85730935876ef62bcbb56b90ab273be
Patch Set 1 #
Total comments: 16
Patch Set 2 : Refactoring: separate build steps in real_main for compile.py #Patch Set 3 : add TODO #Patch Set 4 : remove unnecessary return #
Total comments: 10
Patch Set 5 : move UploadNinjaLog in main_ninja #
Total comments: 4
Patch Set 6 : add two blank lines #
Total comments: 8
Patch Set 7 : move determine_goma_jobs out of main_ninja #Patch Set 8 : fix docstring in main_ninja #Messages
Total messages: 23 (7 generated)
Description was changed from ========== Refactoring: separate build steps in real_main for compile.py * extract goma_setup and goma_teardown from main_ninja. * make a function get_parsed_options for options After this CL will be committed, I will make scripts calling function of compile.py in recipe_modules/goma to make separated build steps. BUG=627693 ========== to ========== Refactoring: separate build steps in real_main for compile.py * extract goma_setup and goma_teardown from main_ninja. * make a function get_parsed_options for options After this CL will be committed, I will make scripts calling function of compile.py in recipe_modules/goma to make separated build steps. BUG=627693 ==========
https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:348: def main_ninja(options, args, env, goma_ready): instead of passing goma_ready, use options.goma_dir for goma readiness. https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:349: """calls ninja.""" Args and Returns ? https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:453: return exit_status, command better to emit json in a file for additional text to ninja_log, so goma_util.UploadNinjaLog just reads the file, instead of taking command line argument (so no need to return command here) https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:548: goma_ready, goma_cloudtail = goma_setup(options, env) move goma_setup and goma_teardown in goma_utils? (maybe, todo) https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:551: exit_status, command = main_ninja(options, args, env, goma_ready) catch exception in main_ninja?
https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:453: return exit_status, command On 2016/07/20 01:33:55, ukai wrote: > better to emit json in a file for additional text to ninja_log, > so goma_util.UploadNinjaLog just reads the file, instead of taking command line > argument (so no need to return command here) I understand that saying different opinions between reviewers confuses owner of the change list but I prefer explicit way of passing data. i.e. current implementation. By the way, what is preferred behavior when the JSON file is missing? If returning so much data is not good, why can't we execute goma_utils.UploadNinjaLog here? Command execution has already done. .ninja_log should already be updated? https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:455: pass you do not need finally. https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:541: def real_man(): real_main? pylint did not detect this?
https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:353: if not goma_ready: Since goma_ready is used only for assertion purpose here. So, I feel it's good to do out of this function.
https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:348: def main_ninja(options, args, env, goma_ready): On 2016/07/20 01:33:55, ukai wrote: > instead of passing goma_ready, use options.goma_dir for goma readiness. Done. https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:349: """calls ninja.""" On 2016/07/20 01:33:55, ukai wrote: > Args and Returns > ? Done. https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:353: if not goma_ready: On 2016/07/20 02:54:43, shinyak wrote: > Since goma_ready is used only for assertion purpose here. > So, I feel it's good to do out of this function. Done. https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:455: pass On 2016/07/20 02:30:32, Yoshisato Yanagisawa wrote: > you do not need finally. Done. https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:541: def real_man(): On 2016/07/20 02:30:32, Yoshisato Yanagisawa wrote: > real_main? > > pylint did not detect this? Thanks. It looks that `git cl upload` does not run pylint. https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:548: goma_ready, goma_cloudtail = goma_setup(options, env) On 2016/07/20 01:33:55, ukai wrote: > move goma_setup and goma_teardown in goma_utils? > (maybe, todo) Let me do in another CL. I added TODO comment in goma_setup and goma_teardown. https://codereview.chromium.org/2155413003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:551: exit_status, command = main_ninja(options, args, env, goma_ready) On 2016/07/20 01:33:55, ukai wrote: > catch exception in main_ninja? Done.
phajdan.jr@chromium.org changed reviewers: + phajdan.jr@chromium.org
LGTM I don't think you need that many reviewers. As far as I'm concerned, yyanagisawa@ is an owner of compile.py and goma_utils.py, and we could also add you goma folks to some OWNERS file in build.
lgtm https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.p... scripts/slave/compile.py:349: """This function calls ninja. it does clobbers object files etc? https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.p... scripts/slave/compile.py:358: secont element is actual command to run ninja. secont? https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.p... scripts/slave/compile.py:398: if options.compiler in ('goma', 'goma-clang'): might be better to pass number of jobs via options or so? https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.p... scripts/slave/compile.py:570: goma_utils.UploadNinjaLog( upload ninja log in main_ninja?
lgtm w/ nits. https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.p... scripts/slave/compile.py:364: # less than the other comparable functions in this file. Do we need this explanation? We might be only have ninja, and nothing else?
(don't wait until I'm back with landing this, I agree with phajdan that you don't need that many reviewers :-) ) https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.p... scripts/slave/compile.py:349: """This function calls ninja. On 2016/07/20 07:07:31, ukai wrote: > it does clobbers object files etc? It currently does (but only on the very few bots that don't use recipes -- recipe bots have a separate clobber step already), but I'd like to remove that. See the bug # in the TODO for the clobber bits in this file.
tikuta@chromium.org changed reviewers: - dpranke@chromium.org, phajdan@google.com
Thank you for review. I removed some reviwers from this issue. https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.p... scripts/slave/compile.py:358: secont element is actual command to run ninja. On 2016/07/20 07:07:31, ukai wrote: > secont? Done. https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.p... scripts/slave/compile.py:364: # less than the other comparable functions in this file. On 2016/07/20 07:31:57, Yoshisato Yanagisawa wrote: > Do we need this explanation? We might be only have ninja, and nothing else? Done. https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.p... scripts/slave/compile.py:398: if options.compiler in ('goma', 'goma-clang'): On 2016/07/20 07:07:31, ukai wrote: > might be better to pass number of jobs via options or so? Done. https://codereview.chromium.org/2155413003/diff/60001/scripts/slave/compile.p... scripts/slave/compile.py:570: goma_utils.UploadNinjaLog( On 2016/07/20 07:07:31, ukai wrote: > upload ninja log in main_ninja? Done.
lgtm https://codereview.chromium.org/2155413003/diff/80001/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/80001/scripts/slave/compile.p... scripts/slave/compile.py:74: # TODO(tikuta): move to goma_utils.py nit: Two blank lines between top-level definitions, https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines https://codereview.chromium.org/2155413003/diff/80001/scripts/slave/compile.p... scripts/slave/compile.py:225: # TODO(tikuta): move to goma_utils.py ditto
https://codereview.chromium.org/2155413003/diff/80001/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/80001/scripts/slave/compile.p... scripts/slave/compile.py:74: # TODO(tikuta): move to goma_utils.py On 2016/07/21 02:53:07, shinyak wrote: > nit: Two blank lines between top-level definitions, > > https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines > Done. https://codereview.chromium.org/2155413003/diff/80001/scripts/slave/compile.p... scripts/slave/compile.py:225: # TODO(tikuta): move to goma_utils.py On 2016/07/21 02:53:07, shinyak wrote: > ditto Done.
lgtm https://codereview.chromium.org/2155413003/diff/100001/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/100001/scripts/slave/compile.... scripts/slave/compile.py:360: second element is actual command to run ninja. only returns exit status? https://codereview.chromium.org/2155413003/diff/100001/scripts/slave/compile.... scripts/slave/compile.py:432: ['build%d-m4' % x for x in range(45, 48)] + why changed? https://codereview.chromium.org/2155413003/diff/100001/scripts/slave/compile.... scripts/slave/compile.py:439: if not options.goma_jobs: TODO: move determine_goma_jobs out of main_ninja ? https://codereview.chromium.org/2155413003/diff/100001/scripts/slave/compile.... scripts/slave/compile.py:568: assert options.goma_dir is None better to fix options.goma_jobs based on goma_ready here?
https://codereview.chromium.org/2155413003/diff/100001/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2155413003/diff/100001/scripts/slave/compile.... scripts/slave/compile.py:360: second element is actual command to run ninja. On 2016/07/21 07:29:08, ukai wrote: > only returns exit status? Done. https://codereview.chromium.org/2155413003/diff/100001/scripts/slave/compile.... scripts/slave/compile.py:432: ['build%d-m4' % x for x in range(45, 48)] + On 2016/07/21 07:29:08, ukai wrote: > why changed? fixed to use xrange https://codereview.chromium.org/2155413003/diff/100001/scripts/slave/compile.... scripts/slave/compile.py:439: if not options.goma_jobs: On 2016/07/21 07:29:08, ukai wrote: > TODO: move determine_goma_jobs out of main_ninja > ? Done. https://codereview.chromium.org/2155413003/diff/100001/scripts/slave/compile.... scripts/slave/compile.py:568: assert options.goma_dir is None On 2016/07/21 07:29:08, ukai wrote: > better to fix options.goma_jobs based on goma_ready here? Done.
I got lgtm from all reviewers not in holiday and fixed some code pointed out by reviewer. I commit this CL.
The CQ bit was checked by tikuta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yyanagisawa@chromium.org, phajdan.jr@chromium.org, shinyak@chromium.org, ukai@chromium.org Link to the patchset: https://codereview.chromium.org/2155413003/#ps140001 (title: "fix docstring in main_ninja")
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 ========== Refactoring: separate build steps in real_main for compile.py * extract goma_setup and goma_teardown from main_ninja. * make a function get_parsed_options for options After this CL will be committed, I will make scripts calling function of compile.py in recipe_modules/goma to make separated build steps. BUG=627693 ========== to ========== Refactoring: separate build steps in real_main for compile.py * extract goma_setup and goma_teardown from main_ninja. * make a function get_parsed_options for options After this CL will be committed, I will make scripts calling function of compile.py in recipe_modules/goma to make separated build steps. BUG=627693 Committed: https://chromium.googlesource.com/chromium/tools/build/+/4960aa40d85730935876... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/tools/build/+/4960aa40d85730935876... |
