Python: Build external extractor #15845
Conversation
tausbn
force-pushed
the
tausbn/python-extractor-fix-build
branch
3 times, most recently
from
a867679 to
f3d953d
Compare
tausbn
force-pushed
the
tausbn/python-extractor-fix-build
branch
from
f3d953d to
278efb0
Compare
tausbn
force-pushed
the
tausbn/python-extractor-fix-build
branch
4 times, most recently
from
795dcd4 to
4dbc742
Compare
This is essentially the contents of `language-packs/python/tools` with some minor modifications to account for the changed location. Of note: we explicitly exclude the `recorded-call-graph-metrics` director that was already present in `python/tools`. When we revisit this directory for some cleanup (e.g. to get rid of the `lgtm` references), we'll probably want to switch to an explicit list of sources to include.
Two issues: - Tests relying on existing query machinery (i.e. `import python`) were not resolving correctly due to a bad `qlpack.yml` file. - The diagnostics output tests needed an updated import to account for their new location.
The referenced file lives in the internal repo, so this is perhaps a bit of a hack, but I think it should be fine in the short run.
This aligns us a bit more with Ruby.
The current name results in a path that is more than 260 characters long, and this causes issues for the build on Windows.
tausbn
force-pushed
the
tausbn/python-extractor-fix-build
branch
from
4dbc742 to
d12ac1e
Compare
| srcs = glob(["**/*"]), | ||
| excludes = [ | ||
| "BUILD.bazel", | ||
| ] + glob(["recorded-call-graph-metrics/**"]), |
There was a problem hiding this comment.
I believe this is a functional change compared to the previous build. Is this on purpose?
There was a problem hiding this comment.
If you mean the exclusion of the recorded-call-graph-metrics directory, then yes. The reason for this is that this directory already existed as a subdirectory of the tools directory, and hence without the exclusion would get included in the extractor distribution (which was silly).
There was a problem hiding this comment.
Ah, I didn't ready properly - I thought it was an active inclusion of the directory, which is a functional change. Having an active exclusion for it is sensible, to keep the dist as it was previously. Thanks for the clarification!
| @@ -12,5 +12,5 @@ codeql_rust_binary( | |||
| visibility = ["//visibility:public"], | |||
| deps = all_crate_deps( | |||
| normal = True, | |||
| ) + ["//python/extractor/tsg-python/tree-sitter-python"], | |||
There was a problem hiding this comment.
nit: tspy would probably have been better to avoid confusion with the Tool Status Page (TSP), but not a blocker.
There was a problem hiding this comment.
Ah, good point. I'm hoping we can eventually get a better solution to this path problem anyway, and fixing this in a follow-up PR should be very doable.
At present, none of the code in this PR has any effect, as the internal copy is still used for the build and CI. However, the changes in this PR should enable us to build the external version instead.
I suggest reviewing commit-by-commit, as one of the commits consists of renaming the
tree-sitter-pythondirectory to something a bit shorter (due to path length issues on Windows).Apart from that, all this PR does is fix up some references in the various build files. Because tests are still being run using the internal extractor, this PR is therefore completely safe to merge.
A later PR will take care of hooking up the internal machinery so that we build this external copy of the extractor instead.