New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 643026 link

Starred by 12 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Feature

Blocking:
issue 526154



Sign in to add a comment

Provide codesearch XRefs for Android

Project Member Reported by carlosk@chromium.org, Sep 1 2016

Issue description

Is the summary statement possible? I'm coming at this while looking at this case:

- I found this abstract class definition OfflinePageArchiver [1] and its X-refs show only one subclass used for testing.
- Then I did a text search and found the implementation I knew had to exist, OfflinePageMHTMLArchiver [2].
- The latter is not analyzed for Xrefs because it's included in the chrome_browser_offline_pages_sources build target that is Android-only as defined in chrome/chrome_browser.gypi [3].
- And it doesn't make sense to make Linux targets build that file as it's only used in Android anyway (even though that would solve this problem).

So I was wondering if it would be possible to somehow "merge" the Xrefs from Android C++ files into the data we already have from Linux.


[1] https://cs.chromium.org/chromium/src/components/offline_pages/offline_page_archiver.h?cl=GROK&l=48&gs=cpp%253Aoffline_pages%253A%253Aclass-OfflinePageArchiver%2540chromium%252F..%252F..%252Fcomponents%252Foffline_pages%252Foffline_page_archiver.h%257Cdef&gsn=OfflinePageArchiver&ct=xref_usages

[2] https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h?q=%22OfflinePageArchiver%22&dr=C

[3] https://cs.chromium.org/chromium/src/chrome/chrome_browser.gypi?q="offline_page_mhtml_archiver.h"+"chrome_browser_offline_pages_sources"&sq=package:chromium&dr=C
 
Status: Available (was: Untriaged)

Comment 2 by emso@chromium.org, Sep 15 2016

Labels: Xrefs
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build.git/+/e6aead043608dd9e421106eaf4d1ba7b1a295f70

commit e6aead043608dd9e421106eaf4d1ba7b1a295f70
Author: Adam Chyb <achyb@chromium.org>
Date: Mon Jan 16 03:46:48 2017

Added a builder to create index packs for the Android build of Chromium

BUG= 662136 ,  643026 , 526154

Change-Id: Icc2f8bc57c5fa7d49dacfd9fd22476d768311b04
Reviewed-on: https://chromium-review.googlesource.com/426284
Reviewed-by: Dave Sansome <dsansome@chromium.org>
Reviewed-by: Phil Wright <philwright@chromium.org>
Commit-Queue: Adam Chyb <achyb@chromium.org>

[modify] https://crrev.com/e6aead043608dd9e421106eaf4d1ba7b1a295f70/masters/master.chromium.infra.cron/master.cfg
[modify] https://crrev.com/e6aead043608dd9e421106eaf4d1ba7b1a295f70/masters/master.chromium.infra.cron/slaves.cfg

Comment 5 by achyb@chromium.org, Jan 24 2017

Status: Started (was: Available)
Thanks for giving a concrete example of where XRefs weren't being provided for Android C++ code. We are in the process of migrating Chromium CodeSearch XRefs to Kythe and we're close to being able to provide XRefs for multiple build configs of Chromium, including Android. Here's the Android code you gave complete with XRefs on a local example XRefs browser.
android xrefs kythe.png
216 KB View Download
Awesome! Thanks for working on this! \o/
Blocking: 526154
Summary: Provide codesearch XRefs for Android (was: Is it possible to also include Xrefs for C++ files compiled for Android?)
Cc: benhenry@chromium.org dsansome@chromium.org emso@chromium.org jam@chromium.org ben@chromium.org tandrii@chromium.org
 Issue 662136  has been merged into this issue.

Comment 9 by backer@chromium.org, Jan 29 2018

I recently transferred from google3 to Chrome. codesearch was an invaluable tool for me in google3. It is much less valuable for me in chrome because I can't trust the Xrefs. It has cost me at least 1.5 days in the 3 months since I transferred onto Chrome.

Here's an example. When I click on the InProcessCommandBuffer::Service I get 31 references (https://cs.chromium.org/chromium/src/gpu/ipc/in_process_command_buffer.h?type=cs&q=InProcessCommandBuffer&sq=package:chromium&l=221), but none of these include Android. If I do a plain text search, I can find the Android references (https://cs.chromium.org/search/?q=InProcessCommandBuffer::Service+file:android&sq=package:chromium&type=cs).


Given that most of our users are on Android, is this indexing this code properly prioritized as a P3?
Cc: -tandrii@chromium.org
Labels: -Pri-3 Pri-2
Status: Available (was: Started)
For the sake of posterity, I'll copy here what I said in the email thread about this same topic:

We *do* want to provide indexing and XRefs for non-Linux platforms, in particular for Windows and Android. However, we are a team of 1.25 SWEs (flyboy@ full time, and me, trying to balance it with the three other projects I lead), and codesearch is, frankly, not stable. We believe that our time is better spent making sure that codesearch works at all, rather than trying to get it to work for more people but instead dropping the ball for everyone.

Our long-term plan is to first provide stability (through a combination of improving monitoring+alerting, partially unforking so external codesearch is broken by internal changes less often, integrating release processes, and preparing to move to the Cloud Source Repositories codesearch solution), and then provide features (xrefs for other platforms, inline editing) as we can.

If we want to be able to provide both a stable service, and get to things like this faster, then we're going to need to talk about how to get more heads working on codesearch :)
As someone with zero experience in infra-work: are there opportunities for someone interested to work on this as a 20% project. Or is this too big of a project for that?
(or maybe even if someone gets it done as 20%, there will be an ongoing maintenance burden for the 1.25 strong team?)
Yeah, it's mostly the latter. Supporting additional Xref pipelines and layers is highly nontrivial, so even if someone else were to add them, it would increase both our ongoing and our alert/interrupt-driven maintenance load. We don't want to do that until we're in a good enough place that the increase will be manageable.

Comment 15 by j...@chromium.org, Mar 27 2018

Owner: j...@chromium.org
Status: Started (was: Available)
Hi folks, I've just joined the Code Search team and I'm going to be taking this work on. I'm aiming to get it done within the next quarter.
Project Member

Comment 16 by bugdroid1@chromium.org, Apr 5 2018

Project Member

Comment 18 by bugdroid1@chromium.org, Apr 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/8e739056e8f811062913dd236215044f32b21323

commit 8e739056e8f811062913dd236215044f32b21323
Author: Joey Scarr <jsca@google.com>
Date: Wed Apr 11 05:11:05 2018

Add support for Kythe VName roots in package_index.py.

This CL also adds support to the Chromium codesearch builder for passing in the
root to use.

Bug:  643026 
Change-Id: I077c0e56303ee5fd33dccd278472135b9d72af40
Reviewed-on: https://chromium-review.googlesource.com/1004156
Commit-Queue: Joey Scarr <jsca@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>

[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/recipe_modules/codesearch/api.py
[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/recipe_modules/codesearch/tests/create_and_upload_kythe_index_pack.expected/basic_without_got_revision_cp.json
[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/recipes/chromium_codesearch.py
[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/recipe_modules/codesearch/tests/create_and_upload_kythe_index_pack.expected/bucket_name_not_set_failed.json
[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/recipe_modules/codesearch/config.py
[add] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/recipe_modules/codesearch/tests/create_and_upload_kythe_index_pack.expected/basic_without_kythe_root.json
[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/recipe_modules/codesearch/tests/create_and_upload_kythe_index_pack.py
[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/unittests/package_index_test.py
[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/README.recipes.md
[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/chromium/package_index.py
[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/recipe_modules/codesearch/tests/create_and_upload_kythe_index_pack.expected/basic.json
[modify] https://crrev.com/8e739056e8f811062913dd236215044f32b21323/scripts/slave/recipe_modules/codesearch/tests/configs.py

Project Member

Comment 19 by bugdroid1@chromium.org, Apr 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/4bcfbbac6f3fbec4c9854b721105dfd9191c9ee6

commit 4bcfbbac6f3fbec4c9854b721105dfd9191c9ee6
Author: Joey Scarr <jsca@google.com>
Date: Thu Apr 19 01:23:04 2018

Switch the new CS builders to use VName roots.

Previously we were using the corpus name to differentiate between Kythe
index packs for different build configs. Using VName roots instead
allows us to have everything in a single corpus, which means less extra
configuration and maintenance for the Kythe team.

Bug:  643026 
Change-Id: I880287d5231b5c1789bb737cb63f587dab50872b
Reviewed-on: https://chromium-review.googlesource.com/1016183
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Joey Scarr <jsca@chromium.org>

[modify] https://crrev.com/4bcfbbac6f3fbec4c9854b721105dfd9191c9ee6/scripts/slave/recipes/chromium_codesearch.py
[modify] https://crrev.com/4bcfbbac6f3fbec4c9854b721105dfd9191c9ee6/scripts/slave/recipes/chromium_codesearch.expected/full_codesearch_gen_chromium_android.json
[modify] https://crrev.com/4bcfbbac6f3fbec4c9854b721105dfd9191c9ee6/scripts/slave/README.recipes.md
[modify] https://crrev.com/4bcfbbac6f3fbec4c9854b721105dfd9191c9ee6/scripts/slave/recipes/chromium_codesearch.expected/full_codesearch_gen_chromium_android_with_revision.json
[modify] https://crrev.com/4bcfbbac6f3fbec4c9854b721105dfd9191c9ee6/scripts/slave/recipes/chromium_codesearch.expected/full_codesearch_gen_chromium_win_with_revision.json
[modify] https://crrev.com/4bcfbbac6f3fbec4c9854b721105dfd9191c9ee6/scripts/slave/recipes/chromium_codesearch.expected/full_codesearch_gen_chromium_win.json

Project Member

Comment 20 by bugdroid1@chromium.org, May 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/46e7b791cd226626c320de8999974dafe01262a3

commit 46e7b791cd226626c320de8999974dafe01262a3
Author: Joey Scarr <jsca@google.com>
Date: Mon May 21 23:28:36 2018

Add support for custom out directories to package_index.py.

This missing feature should be the final piece of the puzzle to get
Android xrefs working in Code Search. Before this CL, there is a bug
where output is written to src/out/chromium-android/Debug, but the code
in package_index.py assumes that it's in src/out/Debug. This results
in incorrect relative paths in the Kythe index packs.

Bug:  643026 
Change-Id: I39d3da00cbec6769337e43fb12856c0ad2ae8104
Reviewed-on: https://chromium-review.googlesource.com/1065962
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Joey Scarr <jsca@chromium.org>

[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/recipe_modules/codesearch/api.py
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/recipe_modules/codesearch/examples/full.py
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/recipes/chromium_codesearch.expected/full_codesearch_gen_chromium_android.json
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/recipe_modules/codesearch/examples/full.expected/codesearch_gen_chromium_win_delete_generated_files_fail.json
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/recipes/chromium_codesearch.expected/full_codesearch_gen_chromium_win_delete_generated_files_fail.json
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/unittests/package_index_test.py
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/README.recipes.md
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/recipes/chromium_codesearch.expected/full_codesearch_gen_chromium_android_with_revision.json
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/recipes/chromium_codesearch.expected/full_codesearch_gen_chromium_win_with_revision.json
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/chromium/package_index.py
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/recipe_modules/codesearch/examples/full.expected/codesearch_gen_chromium_win_test_basic.json
[modify] https://crrev.com/46e7b791cd226626c320de8999974dafe01262a3/scripts/slave/recipes/chromium_codesearch.expected/full_codesearch_gen_chromium_win.json

Project Member

Comment 21 by bugdroid1@chromium.org, May 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/51e85f8042a04abd2122155d0e8828da5fbc5a9c

commit 51e85f8042a04abd2122155d0e8828da5fbc5a9c
Author: Joey Scarr <jsca@google.com>
Date: Tue May 22 03:11:16 2018

Fix a bug in package_index.py.

This was introduced in crrev.com/c/1065962.

Example failure:
https://ci.chromium.org/buildbot/chromium.infra.codesearch/codesearch-gen-chromium-chromiumos/2622

TBR=agable@chromium.org

Bug:  643026 
Change-Id: If6f00f90b99e452227c98129477ee208f649690e
Reviewed-on: https://chromium-review.googlesource.com/1068547
Reviewed-by: Joey Scarr <jsca@chromium.org>
Commit-Queue: Joey Scarr <jsca@chromium.org>

[modify] https://crrev.com/51e85f8042a04abd2122155d0e8828da5fbc5a9c/scripts/slave/chromium/package_index.py

Comment 22 by j...@chromium.org, Jun 20 2018

Status: Fixed (was: Started)
\o/

When will this land in production?

Comment 24 by j...@chromium.org, Jun 21 2018

It's already in production :) See the "Target OS" dropdown. (UX improvements are in the works, but it's already functional).

Sign in to add a comment