Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2332283002:
Use [CACHE]/cipd/goma for goma_dir in recipes/chromium_codesearch.py (Closed)
|
Created:
4 years, 3 months ago by tikuta Modified:
4 years, 3 months ago Reviewers:
iannucci, shinyak, ukai, emso, dsansome-google, dsansome, Yoshisato Yanagisawa, *Paweł Hajdan Jr. CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionUse [CACHE]/cipd/goma for goma_dir in recipes/chromium_codesearch.py
* use chromium.ensure_goma()
* modify gn args to use correct goma_dir
This is reland of https://codereview.chromium.org/2328263002/
BUG=621828
Committed: https://chromium.googlesource.com/chromium/tools/build/+/b204faa4ed43b92a19079a9f675c7c1bcbe65128
Patch Set 1 #
Total comments: 5
Patch Set 2 : use gomadir only in ninja #Patch Set 3 : check goma_dir #
Total comments: 2
Patch Set 4 : set goma_dir #
Total comments: 4
Patch Set 5 : use build_with_goma #
Total comments: 2
Patch Set 6 : use goma_dir #
Total comments: 1
Messages
Total messages: 40 (14 generated)
tikuta@chromium.org changed reviewers: + iannucci@chromium.org, shinyak@chromium.org, ukai@chromium.org, yyanagisawa@chromium.org
tikuta@chromium.org changed required reviewers: + iannucci@chromium.org
please fix subject/description
Description was changed from ========== Use [CACHE]/cipd/goma for goma_dir in recipes/client.nacl.sdk.recipe_autogen.py * use chromium.ensure_goma() * modify gn args to use correct goma_dir This is reland of https://codereview.chromium.org/2328263002/ BUG=621828 ========== to ========== Use [CACHE]/cipd/goma for goma_dir in recipes/chromium_codesearch.py * use chromium.ensure_goma() * modify gn args to use correct goma_dir This is reland of https://codereview.chromium.org/2328263002/ BUG=621828 ==========
On 2016/09/13 05:16:13, ukai wrote: > please fix subject/description ah, done.
lgtm https://codereview.chromium.org/2332283002/diff/1/scripts/slave/recipes/chrom... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/1/scripts/slave/recipes/chrom... scripts/slave/recipes/chromium_codesearch.py:104: args.extend(['use_goma=true', ok to add this to CHROMEOS_GN_ARGS?
https://codereview.chromium.org/2332283002/diff/1/scripts/slave/recipes/chrom... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/1/scripts/slave/recipes/chrom... scripts/slave/recipes/chromium_codesearch.py:104: args.extend(['use_goma=true', On 2016/09/13 05:20:25, ukai wrote: > ok to add this to CHROMEOS_GN_ARGS? I'm not sure, but CHROMEOS_GN_ARGS is made by adding some option to LINUX_GN_ARGS, so it is OK?
lgtm https://codereview.chromium.org/2332283002/diff/1/scripts/slave/recipes/chrom... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/1/scripts/slave/recipes/chrom... scripts/slave/recipes/chromium_codesearch.py:104: args.extend(['use_goma=true', On 2016/09/13 05:29:44, tikuta wrote: > On 2016/09/13 05:20:25, ukai wrote: > > ok to add this to CHROMEOS_GN_ARGS? > > I'm not sure, but CHROMEOS_GN_ARGS is made by adding some option to > LINUX_GN_ARGS, so it is OK? ah, ok.
lgtm w/ comment. https://codereview.chromium.org/2332283002/diff/1/scripts/slave/recipes/chrom... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/1/scripts/slave/recipes/chrom... scripts/slave/recipes/chromium_codesearch.py:105: 'goma_dir=%s' % api.chromium.c.compile_py.goma_dir]) It could be redundant but... if we only judge the behavior with local code, it might be better to set use_goma only if api.chromium.c.compile_py.goma_dir is not empty.
https://codereview.chromium.org/2332283002/diff/1/scripts/slave/recipes/chrom... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/1/scripts/slave/recipes/chrom... scripts/slave/recipes/chromium_codesearch.py:105: 'goma_dir=%s' % api.chromium.c.compile_py.goma_dir]) On 2016/09/14 01:21:22, Yoshisato Yanagisawa wrote: > It could be redundant but... > if we only judge the behavior with local code, it might be better to set > use_goma only if api.chromium.c.compile_py.goma_dir is not empty. Done.
https://codereview.chromium.org/2332283002/diff/40001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/40001/scripts/slave/recipes/c... scripts/slave/recipes/chromium_codesearch.py:106: args.append('use_goma=true') also add goma_dir?
https://codereview.chromium.org/2332283002/diff/40001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/40001/scripts/slave/recipes/c... scripts/slave/recipes/chromium_codesearch.py:106: args.append('use_goma=true') On 2016/09/14 04:10:06, ukai wrote: > also add goma_dir? Done.
lgtm
PTAL, iannucci? all of goma are not in OWNERS of these file.
tikuta@chromium.org changed reviewers: + phajdan.jr@chromium.org
tikuta@chromium.org changed required reviewers: + phajdan.jr@chromium.org - iannucci@chromium.org
phajdan.jr@chromium.org changed reviewers: + dsansome@chromium.org, emso@chromium.org
+emso,dsansome for parts of CS infrastructure https://codereview.chromium.org/2332283002/diff/60001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/60001/scripts/slave/recipes/c... scripts/slave/recipes/chromium_codesearch.py:104: if (hasattr(api.chromium.c.compile_py, 'goma_dir') and Why do we have this weird auto-detection? Don't we always want to run goma?
https://codereview.chromium.org/2332283002/diff/60001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/60001/scripts/slave/recipes/c... scripts/slave/recipes/chromium_codesearch.py:104: if (hasattr(api.chromium.c.compile_py, 'goma_dir') and On 2016/09/19 23:19:33, Paweł Hajdan Jr. wrote: > Why do we have this weird auto-detection? Don't we always want to run goma? From the modularity view point, I suggested to do so. GenerateCompilationDatabase cannot know ensure_goma has been called in caller or not. It cannot expect it can always set use_goma if we only see this function. To make recipe unittest detect not running goma, assertion could be the choice instead of this if statement.
dsansome@google.com changed reviewers: + dsansome@google.com
https://codereview.chromium.org/2332283002/diff/60001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/60001/scripts/slave/recipes/c... scripts/slave/recipes/chromium_codesearch.py:104: if (hasattr(api.chromium.c.compile_py, 'goma_dir') and On 2016/09/20 01:35:37, Yoshisato Yanagisawa wrote: > On 2016/09/19 23:19:33, Paweł Hajdan Jr. wrote: > > Why do we have this weird auto-detection? Don't we always want to run goma? > > From the modularity view point, I suggested to do so. > GenerateCompilationDatabase cannot know ensure_goma has been called in caller or > not. It cannot expect it can always set use_goma if we only see this function. > > To make recipe unittest detect not running goma, assertion could be the choice > instead of this if statement. Checking for the presence of an attribute seems like a weird API. Can you expose this as a "should_use_goma" boolean attribute somewhere? Where are the docs for using goma in recipes?
https://codereview.chromium.org/2332283002/diff/60001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/60001/scripts/slave/recipes/c... scripts/slave/recipes/chromium_codesearch.py:104: if (hasattr(api.chromium.c.compile_py, 'goma_dir') and On 2016/09/20 04:21:43, dsansome-google wrote: > On 2016/09/20 01:35:37, Yoshisato Yanagisawa wrote: > > On 2016/09/19 23:19:33, Paweł Hajdan Jr. wrote: > > > Why do we have this weird auto-detection? Don't we always want to run goma? > > > > From the modularity view point, I suggested to do so. > > GenerateCompilationDatabase cannot know ensure_goma has been called in caller > or > > not. It cannot expect it can always set use_goma if we only see this > function. > > > > To make recipe unittest detect not running goma, assertion could be the choice > > instead of this if statement. > > Checking for the presence of an attribute seems like a weird API. > Can you expose this as a "should_use_goma" boolean attribute somewhere? > > Where are the docs for using goma in recipes? OK. let me remove this if statement. assertion occurs in unittest if ensure_goma is not called. Currently there is no docs for using goma in recipes explaining usage more than example.py. https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/goma/exam...
LGTM w/comment https://codereview.chromium.org/2332283002/diff/80001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/80001/scripts/slave/recipes/c... scripts/slave/recipes/chromium_codesearch.py:106: 'goma_dir=%s' % api.path.dirname(api.goma.goma_ctl)]) Why don't we just expose goma_dir in api.goma? I'd be fine with a TODO.
Thank you for review. If there is no comment, I'll submit this in 2 days. https://codereview.chromium.org/2332283002/diff/80001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/80001/scripts/slave/recipes/c... scripts/slave/recipes/chromium_codesearch.py:106: 'goma_dir=%s' % api.path.dirname(api.goma.goma_ctl)]) On 2016/09/20 17:05:14, Paweł Hajdan Jr. wrote: > Why don't we just expose goma_dir in api.goma? > > I'd be fine with a TODO. Done.
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: This issue passed the CQ dry run.
lgtm better to submit soon, if it fixes codesearch bots?
On 2016/09/21 06:33:00, ukai wrote: > lgtm > > better to submit soon, if it fixes codesearch bots? OK, I commit this.
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 Link to the patchset: https://codereview.chromium.org/2332283002/#ps100001 (title: "use goma_dir")
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 ========== Use [CACHE]/cipd/goma for goma_dir in recipes/chromium_codesearch.py * use chromium.ensure_goma() * modify gn args to use correct goma_dir This is reland of https://codereview.chromium.org/2328263002/ BUG=621828 ========== to ========== Use [CACHE]/cipd/goma for goma_dir in recipes/chromium_codesearch.py * use chromium.ensure_goma() * modify gn args to use correct goma_dir This is reland of https://codereview.chromium.org/2328263002/ BUG=621828 Committed: https://chromium.googlesource.com/chromium/tools/build/+/b204faa4ed43b92a1907... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/tools/build/+/b204faa4ed43b92a1907...
Message was sent while issue was closed.
https://codereview.chromium.org/2332283002/diff/100001/scripts/slave/recipes/... File scripts/slave/recipes/chromium_codesearch.py (right): https://codereview.chromium.org/2332283002/diff/100001/scripts/slave/recipes/... scripts/slave/recipes/chromium_codesearch.py:106: 'goma_dir=%s' % api.goma.goma_dir]) hmm. https://build.chromium.org/p/chromium.infra.cron/builders/ChromiumOS%20Codese... ERROR at the command-line "--args":1:144: Invalid token. is_clang=true is_component_build=true is_debug=true symbol_level=1 target_cpu="x64" target_os="chromeos" use_ozone=true use_goma=true goma_dir=/b/build/slave/cache/cipd/goma ^ I have no idea what this is.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2356123002/ by tikuta@chromium.org. The reason for reverting is: https://build.chromium.org/p/chromium.infra.cron/builders/ChromiumOS%20Codese... ERROR at the command-line "--args":1:144: Invalid token. is_clang=true is_component_build=true is_debug=true symbol_level=1 target_cpu="x64" target_os="chromeos" use_ozone=true use_goma=true goma_dir=/b/build/slave/cache/cipd/goma Need quote for goma_dir?.
Message was sent while issue was closed.
Missing quotes seem like a likely cause. Any chance we could try a re-land of this change soon with quotes? On Wed, Sep 21, 2016 at 9:18 AM, <tikuta@chromium.org> wrote: > A revert of this CL (patchset #6 id:100001) has been created in > https://codereview.chromium.org/2356123002/ by tikuta@chromium.org. > > The reason for reverting is: > https://build.chromium.org/p/chromium.infra.cron/builders/ > ChromiumOS%20Codesearch/builds/3022 > > ERROR at the command-line "--args":1:144: Invalid token. > is_clang=true is_component_build=true is_debug=true symbol_level=1 > target_cpu="x64" target_os="chromeos" use_ozone=true use_goma=true > goma_dir=/b/build/slave/cache/cipd/goma > > Need quote for goma_dir?. > > https://codereview.chromium.org/2332283002/ > -- Emma Söderberg | Software Engineer | emso@google.com | Google Denmark Skt. Petri Passage 5, 1165 København +45 33 70 26 00 -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
