https://pantsbuild.org/ logo
Powered by
a

ancient-vegetable-10556

03/09/2022, 5:21 PM
JVM codegen thread
@witty-crayon-22786 @hundreds-father-404 I’ve got a small test project pulling in a simple protobuf as a dependency, which is generating and compiling the java source (but failing due to lack of runtime library, whcih I am presently fixing)
❤️ 1
👍 2
I’ll be looking at dependencies shortly, but the single file case seems solid
(compilation succeeded!)
🚀 1
h

hundreds-father-404

03/09/2022, 5:24 PM
Sweet! Re runtime dependencies, my advice to Tom for Go Protobuf is rely on manually adding the dep to 
protobuf_source
for now. Doing it via dependency injection is a good followup, and Python has precedent (including w/ resolves)
a

ancient-vegetable-10556

03/09/2022, 5:24 PM
Yeah, that’s what I’m doing right now, which you’ll see once I put the sanity check PR online
f

fast-nail-55400

03/09/2022, 5:30 PM
Sweet! Re runtime dependencies, my advice to Tom for Go Protobuf is rely on manually adding the dep to 
protobuf_source
for now. Doing it via dependency injection is a good followup, and Python has precedent (including w/ resolves)
For Go, it was not necessary to manually add the dep. The Go Protobuf rules run the Go package analyzer on the generated sources to obtain imports. Those imports are then matched against third-party packages and the applicable 
BuildGoPackageRequest
obtained and added.
a

ancient-vegetable-10556

03/09/2022, 5:30 PM
fancy!
f

fast-nail-55400

03/09/2022, 5:30 PM
So it does infer dependencies but not by using the existing dependency inference mechanisms.
a

ancient-vegetable-10556

03/09/2022, 5:33 PM
FYI @witty-crayon-22786 @hundreds-father-404 https://github.com/pantsbuild/pants/pull/14747/files
w

witty-crayon-22786

03/09/2022, 5:47 PM
thanks, sounds good. next step is figuring out how to adjust 
ClasspathEntryRequest.for_targets
to account for codegen probably.
a

ancient-vegetable-10556

03/09/2022, 6:22 PM
Yeah, that’s definitely the next step!
It works!
❤️ 2
I’m going to tidy up my current branch and get one of you to sanity check it, so that when I get some time to pick it up, I should be able to plough through the rest of it.
Current things that are missing: • Classifying every member of the 
CoarsenedTarget
rather than just the representative one • Allowing for multiple generated language backends to be enabled at once
the good news is that all you need to implement is a 
GeneratedSourcesRequest
and you get compilation for free.
🙌 1
Here’s that sanity check PR: https://github.com/pantsbuild/pants/pull/14751
w

witty-crayon-22786

03/09/2022, 11:24 PM
looks reasonable! thanks.
a

ancient-vegetable-10556

03/10/2022, 9:12 PM
@witty-crayon-22786 For restricting generated languages, is the want for: • A 
generators
field, which allows multiple languages to be specified for a single target • A 
generator
field, which allows a single language to be specified, but can be parameterized (this seems extremely fraught in the JVM cases) • A 
jvm_language
field, which is specifically for determining the single JVM language that should be used when compiling?
h

hundreds-father-404

03/10/2022, 9:16 PM
A generators field, which allows multiple languages to be specified for a single target
This. See this comment for why we need it that way for Python to work with multiple resolves: https://github.com/pantsbuild/pants/issues/14484#issuecomment-1057601055
No thought has gone into what the values should be for 
generators
, like how to determine we call it 
"python"
. This may give some inspiration https://github.com/pantsbuild/pants/pull/14356
a

ancient-vegetable-10556

03/10/2022, 9:18 PM
ok
f

fast-nail-55400

03/10/2022, 9:22 PM
Also note that there can be more than one generator for same language and IDL. Namely Apache Thrift for Java and Scrooge for Java.
👍 1
w

witty-crayon-22786

03/10/2022, 9:23 PM
i wonder if it should be two fields then… languages and generators?
a

ancient-vegetable-10556

03/10/2022, 9:24 PM
My instinct here is to aim for languages first, then add generators subsequently
h

hundreds-father-404

03/10/2022, 9:24 PM
Does it make sense to ever generate both Java-Thrift and Java-Scrooge? That seems like the ambiguity that graph.py eagerly complains about
w

witty-crayon-22786

03/10/2022, 9:24 PM
making the field(s) plural makes sense for now, but it’s possible that with 
configuration
we’ll want them singular. it’s just not coming anytime soon.
a

ancient-vegetable-10556

03/10/2022, 9:24 PM
because of what @hundreds-father-404 just said
w

witty-crayon-22786

03/10/2022, 9:25 PM
Does it make sense to ever generate both Java-Thrift and Java-Scrooge?
it would if they had different addresses via either being separate targets or via parametrization (with config or no)
👍 1
…but, short answer: let’s just move pragmatically on this. people are not going to have lots of code generators in play for a little while, and so any deprecations we have to do should be cheap.
👍 2
f

fast-nail-55400

03/10/2022, 9:42 PM
It doesn't make sense to generate both as the Scrooge gen is intended to be a drop-in replacement for Apache Thrift gen.
a

ancient-vegetable-10556

03/10/2022, 9:43 PM
in the specific scrooge vs thrift case, right?
f

fast-nail-55400

03/10/2022, 9:44 PM
Yes for this specific case for Java.
Powered by