Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2178193003:
Move goma_teardown, goma_setup and determine_goma_jobs to goma_utils.py (Closed)
DescriptionMove goma_teardown, goma_setup and determine_goma_jobs to goma_utils.py
BUG=627693
Patch Set 1 #
Total comments: 10
Patch Set 2 : Move some code to goma_utils #
Total comments: 5
Patch Set 3 : separate option #
Messages
Total messages: 16 (6 generated)
The CQ bit was checked by tikuta@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 commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
tikuta@chromium.org changed reviewers: + shinyak@chromium.org, ukai@chromium.org, yyanagisawa@chromium.org
lgtm
Can you eliminate tight coupling of compile.py and goma_utils.py?
On 2016/07/26 04:57:55, Yoshisato Yanagisawa wrote: > Can you eliminate tight coupling of compile.py and goma_utils.py? I will do. To eliminate tight coupling, I will make compile.py only run ninja eventually. For that, I will move goma specific option to goma_utils.py too.
lgtm https://codereview.chromium.org/2178193003/diff/1/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2178193003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:28: DEFAULT_GOMA_CACHE_DIR = os.path.join(SLAVE_DIR, 'goma_cache') can't move it to goma_utils? https://codereview.chromium.org/2178193003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:94: def NeedEnvFileUpdateOnWin(env): move it to goma_utils? https://codereview.chromium.org/2178193003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:186: # HACK(yyanagisawa): update environment files on |env| update. might be goma specific (part of goma_setup)? https://codereview.chromium.org/2178193003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:339: if not goma_ready: if goma_ready: if options.goma_jobs is None: options.goma_jobs = .. else: assert .. options.goma_jobs = .. ? https://codereview.chromium.org/2178193003/diff/1/scripts/slave/goma_utils.py File scripts/slave/goma_utils.py (right): https://codereview.chromium.org/2178193003/diff/1/scripts/slave/goma_utils.py... scripts/slave/goma_utils.py:368: options.goma_dir and returns (False, None) Args: Returns: ?
https://codereview.chromium.org/2178193003/diff/1/scripts/slave/compile.py File scripts/slave/compile.py (right): https://codereview.chromium.org/2178193003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:28: DEFAULT_GOMA_CACHE_DIR = os.path.join(SLAVE_DIR, 'goma_cache') On 2016/07/27 01:13:29, ukai wrote: > can't move it to goma_utils? Done. https://codereview.chromium.org/2178193003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:94: def NeedEnvFileUpdateOnWin(env): On 2016/07/27 01:13:29, ukai wrote: > move it to goma_utils? Done. https://codereview.chromium.org/2178193003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:186: # HACK(yyanagisawa): update environment files on |env| update. On 2016/07/27 01:13:29, ukai wrote: > might be goma specific (part of goma_setup)? Done. https://codereview.chromium.org/2178193003/diff/1/scripts/slave/compile.py#ne... scripts/slave/compile.py:339: if not goma_ready: On 2016/07/27 01:13:29, ukai wrote: > if goma_ready: > if options.goma_jobs is None: > options.goma_jobs = .. > else: > assert .. > options.goma_jobs = .. > ? Done. https://codereview.chromium.org/2178193003/diff/1/scripts/slave/goma_utils.py File scripts/slave/goma_utils.py (right): https://codereview.chromium.org/2178193003/diff/1/scripts/slave/goma_utils.py... scripts/slave/goma_utils.py:368: options.goma_dir and returns (False, None) On 2016/07/27 01:13:29, ukai wrote: > Args: > Returns: > > ? Done.
What is goal of this refactoring? To modify goma_setup and goma_teardown, developers need to read both compile.py and goma_utils.py. That should make people difficult to understand the behavior of compile.py. By the way, current goma_utils only have code to upload statistics, and it does not need to be called from compile.py. I thought goma_setup and goma_teardown would be used from goma recipe_modules but I feel it difficult to do with current implementation. Why it need to depend on compile.py? https://codereview.chromium.org/2178193003/diff/20001/scripts/slave/goma_util... File scripts/slave/goma_utils.py (right): https://codereview.chromium.org/2178193003/diff/20001/scripts/slave/goma_util... scripts/slave/goma_utils.py:370: def goma_setup(options, env): options is data structure made in compile.py, why this need to be passed here? Can't we split it to something like dictionary or so to remove dependency to compile.py? https://codereview.chromium.org/2178193003/diff/20001/scripts/slave/goma_util... scripts/slave/goma_utils.py:394: # HACK(shinyak, yyanagisawa, goma): Windows NO_NACL_GOMA (crbug.com/390764) Can you remove this HACK, and injected from compile.py? This is too specific to chromium. https://codereview.chromium.org/2178193003/diff/20001/scripts/slave/goma_util... scripts/slave/goma_utils.py:541: def goma_teardown(options, env, exit_status, cloudtail_proc): The same, is it impossible for you to eliminate dependency to options?
https://codereview.chromium.org/2178193003/diff/20001/scripts/slave/goma_util... File scripts/slave/goma_utils.py (right): https://codereview.chromium.org/2178193003/diff/20001/scripts/slave/goma_util... scripts/slave/goma_utils.py:394: # HACK(shinyak, yyanagisawa, goma): Windows NO_NACL_GOMA (crbug.com/390764) On 2016/07/27 02:54:46, Yoshisato Yanagisawa wrote: > Can you remove this HACK, and injected from compile.py? This is too specific to > chromium. do we still need this hack?
https://codereview.chromium.org/2178193003/diff/20001/scripts/slave/goma_util... File scripts/slave/goma_utils.py (right): https://codereview.chromium.org/2178193003/diff/20001/scripts/slave/goma_util... scripts/slave/goma_utils.py:394: # HACK(shinyak, yyanagisawa, goma): Windows NO_NACL_GOMA (crbug.com/390764) On 2016/07/27 06:52:54, ukai wrote: > On 2016/07/27 02:54:46, Yoshisato Yanagisawa wrote: > > Can you remove this HACK, and injected from compile.py? This is too specific > to > > chromium. > > do we still need this hack? I do not think so. I feel a bit guilty but I feel we need to do somewhat dead code elimination before this refactoring. Since I know which code is dead / meaningless, you can assign me that task.
I'll fix the issue in other way. I close this issue.
Description was changed from ========== Move goma_teardown, goma_setup and determine_goma_jobs to goma_utils.py BUG=627693 ========== to ========== Move goma_teardown, goma_setup and determine_goma_jobs to goma_utils.py BUG=627693 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
