Add -no-relative-path: flag to control indexing of generated files
#611
+117
−40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, the SemanticDB compiler plugin errored when indexing auto-generated files outside of the configured `-sourceroot` directory (which is automatically inferred for Bazel builds). This behavior was undesirable because: - There's no good workaround for the issue - The error message was cryptic making it difficult to understand what went wrong For some cases, we were able to detect this situation for Bazel and ignore the indexed file while printing an informative message, but this behavior was also undesirable because we skipping these files means that we can't render hover messages for symbols in those generated files. This PR fixes the issue by adding a configurable `-no-relative-path:` flag with the following valid options: - `index_anyways` (default): indexes the file but with no guarantee that it's possible to recover the location of the original generated file. This allows us to display accurate hover tooltips for symbols in these files even if "Go to definition" won't work. - `skip`: silently ignored these files. - `warning`: ignore these files and print a message explaining it was skipped. - `error`: fail the compilation process (old default).
olafurpg
commented
Jul 12, 2023
| @@ -1,18 +1,20 @@ | |||
| """Java rules that automatically register the SemanticDB compiler plugin based on the presence of a string flag.""" | |||
| load("@rules_java//java:defs.bzl", native_java_library="java_library", native_java_binary="java_binary") | |||
There was a problem hiding this comment.
Unrelated formatting changes because format-on-save 🤦🏻
olafurpg
commented
Jul 12, 2023
|
|
||
| if (options.uriScheme == UriScheme.BAZEL && options.generatedTargetRoot != null) { | ||
| try { | ||
| if (absolutePath.toRealPath().startsWith(options.generatedTargetRoot)) { |
There was a problem hiding this comment.
@keynmol FYI I kept options.generatedTargetRoot even if it's no longer used.
keynmol
reviewed
Jul 12, 2023
| // In the future, we may want to additionally embed the full text contents of these files | ||
| // so that it's possible to browse generated files with precise code navigation. | ||
| String uniqueFilename = | ||
| String.format("%d.%s.semanticdb", ++noRelativePathCounter, absolutePath.getFileName()); |
There was a problem hiding this comment.
Do we have any guarantees about javac being invoked only once per Bazel target?
Otherwise the noRelativePathCounter in second invocation will be reset and files can start overwriting each other
There was a problem hiding this comment.
The output of rebuilding the same Bazel target is guaranteed to produce identical results, unless the build is buggy. You have to use different targets if you're cross-building the same set of sources.
keynmol
reviewed
Jul 12, 2023
| return Result.error( | ||
| String.format( | ||
| "Unable to detect the relative path of '%s'. A common reason for this error is that the file is that this file is auto-generated. " | ||
| + "To fix this problem, either configure the -sourceroot:PATH flag to be the parent directory of all indexed files, or " |
There was a problem hiding this comment.
Do you think we can change the error message for Bazel specifically to remove the -sourceroot part, which we know is not sufficient?
keynmol
reviewed
Jul 12, 2023
| String.format( | ||
| "Unable to detect the relative path of '%s'. A common reason for this error is that the file is that this file is auto-generated. " | ||
| + "To fix this problem, either configure the -sourceroot:PATH flag to be the parent directory of all indexed files, or " | ||
| + "configure -no-relative-path:VALUE flag to have one of the following values: index_anyway, skip, warning.", |
There was a problem hiding this comment.
Can use NoRelativePathMode.values() again to avoid things drifting if you were to change values/add new ones
keynmol
approved these changes
Jul 12, 2023
|
I will address the unresolved comments separately |
olafurpg
added a commit
that referenced
this pull request
Jul 13, 2023
This commit addresses unresolved comments from the PR #611
olafurpg
added a commit
that referenced
this pull request
Jul 13, 2023
This commit addresses unresolved comments from the PR #611
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the SemanticDB compiler plugin errored when indexing auto-generated files outside of the configured
-sourcerootdirectory (which is automatically inferred for Bazel builds). This behavior was undesirable because:For some cases, we were able to detect this situation for Bazel and ignore the indexed file while printing an informative message, but this behavior was also undesirable because we skipping these files means that we can't render hover messages for symbols in those generated files.
This PR fixes the issue by adding a configurable
-no-relative-path:flag with the following valid options:index_anyways(default): indexes the file but with no guarantee that it's possible to recover the location of the original generated file. This allows us to display accurate hover tooltips for symbols in these files even if "Go to definition" won't work.skip: silently ignored these files.warning: ignore these files and print a message explaining it was skipped.error: fail the compilation process (old default).Test plan
Manually tested this in the example Bazel workspace with the following commands:
Observe that the generated file does not use the
no-relative-path/NUMBER.FILENAME.semanticdblogic in this PR. To be sure, I manually tested that code path by changing the implementation a bit and confirmed that it generates valid relative paths underMETA-INF/semanticdb/no-relative-path/....