Skip to content

Add scalac_jvm_flags attribute to scalapb_proto_library #680

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

Closed
akakitani opened this issue Jan 28, 2019 · 5 comments
Closed

Add scalac_jvm_flags attribute to scalapb_proto_library #680

akakitani opened this issue Jan 28, 2019 · 5 comments

Comments

@akakitani
Copy link
Contributor

I'm working with some pretty large proto definitions, and scalac ends up choking on the generated classes with a StackOverflowError. I've been able to get this to compile by bumping up -Xss with scalac_jvm_flags on my modified version of scalapb_proto_library. Would you accept a PR with this change?

@johnynek
Copy link
Contributor

I think this could be related to #548

Currently the rules have the flaw that every proto target fully transitively rebuilds all dependencies. In a big repo, I fear that could get big, and it is wasteful.

This is just a flaw that should be fixed (to follow the aspect based approach thrift did) but it has not been done yet.

@akakitani
Copy link
Contributor Author

@johnynek ah I see, do you think this attribute would still be useful regardless? e.g. if I need to run bazel clean, it seems like we could run into this again even if #548 is fixed

@johnynek
Copy link
Contributor

I'm happy to accept the PR if it fixes an issue for you. It may be unrelated to what I'm suggesting. I'm not sure why we should need to have deep recursion depth. Are these really pathological protos?

@akakitani
Copy link
Contributor Author

A little bit yeah - these have a lot of nested message definitions and oneof fields with 200+ entries, and their dependencies are kind of complicated too. Here's a scalac stack trace in case it rings any bells:

Exception in thread "main" java.lang.StackOverflowError
        at scala.tools.nsc.typechecker.SuperAccessors$SuperAccTransformer.transform(SuperAccessors.scala:178)
        at scala.tools.nsc.typechecker.SuperAccessors$SuperAccTransformer.transform(SuperAccessors.scala:71)
        at scala.reflect.internal.Trees$class.itransform(Trees.scala:1345)
        at scala.reflect.internal.SymbolTable.itransform(SymbolTable.scala:16)
        at scala.reflect.internal.SymbolTable.itransform(SymbolTable.scala:16)
        at scala.reflect.api.Trees$Transformer.transform(Trees.scala:2555)
        at scala.tools.nsc.transform.TypingTransformers$TypingTransformer.transform(TypingTransformers.scala:44)
        at scala.tools.nsc.typechecker.SuperAccessors$SuperAccTransformer.mayNeedProtectedAccessor$1(SuperAccessors.scala:186)
        at scala.tools.nsc.typechecker.SuperAccessors$SuperAccTransformer.transformSelect$1(SuperAccessors.scala:332)
        at scala.tools.nsc.typechecker.SuperAccessors$SuperAccTransformer.transform(SuperAccessors.scala:335)
        at scala.tools.nsc.typechecker.SuperAccessors$SuperAccTransformer.transform(SuperAccessors.scala:71)
        at scala.reflect.internal.Trees$class.itransform(Trees.scala:1372)
        at scala.reflect.internal.SymbolTable.itransform(SymbolTable.scala:16)
        at scala.reflect.internal.SymbolTable.itransform(SymbolTable.scala:16)
        at scala.reflect.api.Trees$Transformer.transform(Trees.scala:2555)
        at scala.tools.nsc.transform.TypingTransformers$TypingTransformer.transform(TypingTransformers.scala:44)
        at scala.tools.nsc.typechecker.SuperAccessors$SuperAccTransformer.mayNeedProtectedAccessor$1(SuperAccessors.scala:186)
        at scala.tools.nsc.typechecker.SuperAccessors$SuperAccTransformer.transform(SuperAccessors.scala:341)
        at scala.tools.nsc.typechecker.SuperAccessors$SuperAccTransformer.transform(SuperAccessors.scala:359)
        at scala.tools.nsc.typechecker.SuperAccessors$SuperAccTransformer.transform(SuperAccessors.scala:71)
        at scala.reflect.internal.Trees$class.itransform(Trees.scala:1345)
        ... more of the same ...

But thank you for the quick response, I'll open a PR soon when I get some time

@ittaiz
Copy link
Contributor

ittaiz commented Jan 29, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants