-
Notifications
You must be signed in to change notification settings - Fork 63
Fix StackOverflowError during "Apply changes to the internal targets model" #346
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
base: 252
Are you sure you want to change the base?
Conversation
| // Don't create dependency to synthetic output_jars library if real target already exists | ||
| // This prevents cyclic dependencies like proto -> proto_output_jars -> proto -> proto_output_jars | ||
| if (outputJarsLabel in targetsToImportSet) { | ||
| null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line completely prevents creating the synthetic dependency proto -> proto_output_jars when a real proto_output_jars target exists. While this fixes the StackOverflowError, it causes IDE issues - proto imports now show as unresolved because they can't see the generated Java classes.
The dilemma is:
- With dependency:
proto -> proto_output_jars+proto_output_jars -> proto= cycle → StackOverflowError incalculateTransitivelyExecutableTargets - Without dependency: No cycle, but IDE loses the connection between proto files and generated Java classes
What would be the best approach here to fix it?
- Create the dependency but mark it as "non-transitive" for executable target calculation?
- Fix
calculateTransitivelyExecutableTargetsto handle cycles with recursion protection? - Something else?
|
I wonder if we could just rename the suffix so that a clash is less likely, e.g., _ij_output_jars? |
…model"
Problem: `calculateOutputJarsLibraries()` was creating synthetic dependencies from targets
to output_jars libraries that already exist as real targets. For example, with BUILD file:
```starlark
proto_library(
name = "proto",
srcs = glob(["proto/**/*.proto"]),
strip_import_prefix = "proto",
)
java_proto_library(
name = "proto_output_jars",
deps = [":proto"],
)
```
The plugin would create a synthetic dependency proto -> proto_output_jars,
combined with the real dependency proto_output_jars -> proto, creating cycles:
proto -> proto_output_jars -> proto -> proto_output_jars -> ...
This resulted in infinite recursion and `StackOverflowError` in `calculateTransitivelyExecutableTargets`
during the "Apply changes to the internal targets model" sync phase, causing sync to hang.
Stack trace excerpt:
```
at org.jetbrains.bazel.target.TargetUtils.calculateTransitivelyExecutableTargets(TargetUtils.kt:206)
at org.jetbrains.bazel.target.TargetUtils.access$calculateTransitivelyExecutableTargets(TargetUtils.kt:60)
at org.jetbrains.bazel.target.TargetUtils$calculateTransitivelyExecutableTargets$1$1.invoke(TargetUtils.kt:216)
at org.jetbrains.bazel.target.TargetUtils$calculateTransitivelyExecutableTargets$1$1.invoke(TargetUtils.kt:215)
[infinite recursion continues...]
```
Solution: rename the suffix so that a clash is less likely
085813c to
b05eb1c
Compare
Done. Thanks for suggestion! |
|
YT issue to track: BAZEL-2812 StackOverflowError during "Apply changes to the internal targets model" |
Problem:
calculateOutputJarsLibraries()was creating synthetic dependencies from targets to output_jars libraries that already exist as real targets. For example, with BUILD file:The plugin would create a synthetic dependency proto -> proto_output_jars, combined with the real dependency proto_output_jars -> proto, creating cycles: proto -> proto_output_jars -> proto -> proto_output_jars -> ...
This resulted in infinite recursion and
StackOverflowErrorincalculateTransitivelyExecutableTargetsduring the "Apply changes to the internal targets model" sync phase, causing sync to hang.Stack trace excerpt:
Solution: Don't create dependency from target to synthetic output_jars library if real target already exists.