Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cxx_indexer): use default corpus for meta, builtin nodes #5008

Merged
merged 13 commits into from
Aug 2, 2021

Conversation

justbuchanan
Copy link
Contributor

This should be a no-op unless --use_compilation_corpus_as_default is
enabled, in which case these nodes will be assigned the compilation
unit's corpus.

KytheGraphObserver's constructor emits nodes for builtins and meta
nodes, so setting the default corpus later means it won't apply to these
nodes. The constructor has been modified to accept a default_corpus
parameter, which is set to the compilation unit's corpus when
--use_compilation_corpus_as_default is enabled.

A new test was added that reads the indexer output for a test example
and checks that each vname has a non-empty corpus.

this should be a no-op unless --use_compilation_corpus_as_default is
enabled, in which case these nodes will be assigned the compilation
unit's corpus.

KytheGraphObserver's constructor emits nodes for builtins and meta
nodes, so setting the default corpus later means it won't apply to these
nodes. The constructor has been modified to accept a default_corpus
parameter, which is set to the compilation unit's corpus when
--use_compilation_corpus_as_default is enabled.

A new test was added that reads the indexer output for a test example
and checks that each vname has a non-empty corpus.
@google-cla google-cla bot added the cla: yes label Aug 2, 2021
@justbuchanan
Copy link
Contributor Author

justbuchanan commented Aug 2, 2021

I'm having trouble getting //kythe/cxx/indexer/cxx/testdata:vname_corpus_empty_test to work because the expansion of "$(location :vname_corpus_entries)" gives kythe/cxx/indexer/cxx/testdata/vname_corpus_entries.entries.gz, but the actual path should be something like bazel-out/k8-fastbuild/bin/kythe/cxx/indexer/cxx/testdata/vname_corpus_entries.entries.gz.

Any tips on getting that working?

@justbuchanan justbuchanan requested a review from a team August 2, 2021 16:37
@shahms
Copy link
Contributor

shahms commented Aug 2, 2021

I'm having trouble getting //kythe/cxx/indexer/cxx/testdata:vname_corpus_empty_test to work because the expansion of "$(location :vname_corpus_entries)" gives kythe/cxx/indexer/cxx/testdata/vname_corpus_entries.entries.gz, but the actual path should be something like bazel-out/k8-fastbuild/bin/kythe/cxx/indexer/cxx/testdata/vname_corpus_entries.entries.gz.

Any tips on getting that working?

I'm having trouble getting //kythe/cxx/indexer/cxx/testdata:vname_corpus_empty_test to work because the expansion of "$(location :vname_corpus_entries)" gives kythe/cxx/indexer/cxx/testdata/vname_corpus_entries.entries.gz, but the actual path should be something like bazel-out/k8-fastbuild/bin/kythe/cxx/indexer/cxx/testdata/vname_corpus_entries.entries.gz.

Any tips on getting that working?

Use $(execpath ...) https://docs.bazel.build/versions/main/be/make-variables.html#predefined_label_variables

(Or match the VName path and use vnames.json to map it appropriately when extracted)

@justbuchanan
Copy link
Contributor Author

Thanks! I tried that and realized the issue was that I needed to use :vname_corpus_entries.entries.gz instead of :vname_corpus_entries. After that change, the file shows up at the path provided by $(location ...)

@justbuchanan justbuchanan merged commit fd30e09 into kythe:master Aug 2, 2021
@justbuchanan justbuchanan deleted the empty_corpus_cxx branch August 2, 2021 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants