Expose a SemanticDB provider in the semanticdb rule phase #60
Conversation
jadenPete
force-pushed
the
expose-semanticdb-provider
branch
from
08361b8 to
24e4665
Compare
| ) | ||
|
|
||
| def _semanticdb_directory_from_file(file): | ||
| return file.path[:file.path.find("META-INF") - 1] |
There was a problem hiding this comment.
Does this work with path mapping? If so, can this be changed to use short_path instead, so we don't accidentally match on any config/architecture dependent parts of the path?
There was a problem hiding this comment.
I probably should've mentioned this in a comment, but I acknowledge this is pretty janky and could match on the wrong part of the path. Unfortunately, we're pretty limited in what we can do here. From the documentation on Args#add_all:
To avoid unintended retention of large analysis-phase data structures into the execution phase, the
map_eachfunction must be declared by a top-leveldefstatement; it may not be a nested function closure by default.
It does work with path mapping because this function will be called at execution time, not analysis time. In fact, this sort of approach is recommended for use with path mapping:
Generally speaking, all functions that return a path string that may contain a configuration prefix such as bazel-out/darwin-amd64-fastbuild/bin must only be called in map_each callbacks, where they are automatically path mapped by Bazel.
Using short_path doesn't work because the compiler expects the output base to be in the path as its working directory is not the output base. I can do this instead, though:
def _semanticdb_directory_from_file(file):
return "{}/{}".format(file.root.path, file.short_path[:file.short_path.find("META-INF") - 1])|
|
||
| options.flatMap { | ||
| case s"-P:semanticdb:targetroot:$path" => | ||
| case scala2SemanticDbTargetRootRegex(path) => |
There was a problem hiding this comment.
This is going to be slow and it's going to get slower the more cases we have here. If we have lots of regexes here you could be evaluating a lot of regexes for every compiler option that has a path in it.
Could we instead make --compiler_option_referencing_path with a format of --compiler_option_referencing_path=<template_compiler_option> <path> to avoid the regex?
We could do something like --compiler_option_referencing_path=-P:semanticdb:targetroot:${path} <path> and then replace ${path} with the path. We do something vaguely similar to this already for --group in the deps checker.
That transforms this from something where, worst case, we evaluate n regexes to something where we evaluate 0 regexes and instead just do a string replace.
There was a problem hiding this comment.
I realize reading more that this doesn't handle your -sourceroot case, but that can easily be handled with something like -sourceroot:${workdir} and then on every string we'd be doing two string replaces: one for the path and one for workdir.
If that's too slow then you could have a separate argument list for --compiler_option_referencing_workdir and just do the workdir there. That way you're not doing unnecessary string replace calls.
There was a problem hiding this comment.
Good idea; I didn't think about the case where we add more compiler options to this list. I'll do that.
| List(s"-P:semanticdb:sourceroot:${workDir.toString}", s"-P:semanticdb:targetroot:${adjustStringPath(path)}") | ||
|
|
||
| case s"-semanticdb-target:$path" => | ||
| case scala3SemanticDbTargetRootRegex(path) => | ||
| List(s"-semanticdb-target:${adjustStringPath(path)}", s"-sourceroot:${workDir.toString}") |
There was a problem hiding this comment.
I forgot to ask this when we added the semanticdb functionality, but does it matter if the sourceroot ceases to exist after the semanticdb call? Because there's no guarantee the sandbox directory will exist long term.
There was a problem hiding this comment.
I don't think so. SemanticDB information is separate from sources and from what I understand, the SemanticDB files don't include the source root path. The sandbox directory only needs to live as long as the compiler.
| workRequest.plugins.map(p => s"-Xplugin:$p") ++ | ||
| workRequest.compilerOptions ++ | ||
| workRequest.compilerOptionsReferencingPaths, | ||
| ).toArray, |
There was a problem hiding this comment.
Good catch on the single .toArray optimization here
|
|
||
| case s"-semanticdb-target:$path" => | ||
| List(s"-semanticdb-target:${adjustStringPath(path)}", s"-sourceroot:${workDir.toString}") | ||
| private val pathPlaceholderRegex = """\$\{path\}""".r |
Matching on string interpolation is terribly slow.
jadenPete
force-pushed
the
expose-semanticdb-provider
branch
from
67bee63 to
4c0cc32
Compare
|
Squashed. |
This also makes the phase work under path mapping.