Make compile scala callable #540
Conversation
| + [ctx.outputs.manifest, ctx.executable._ijar, argfile]) | ||
| compiler_classpath_jars.to_list() + all_srcjars.to_list() + list(sources) | ||
| + plugins_list + dependency_analyzer_plugin_jars + classpath_resources + | ||
| resources + resource_jars + [manifest, ctx.executable._ijar, argfile]) |
There was a problem hiding this comment.
What happened to ctx.files._java_runtime?
There was a problem hiding this comment.
That should have been removed a long time ago. It was for when we were calling the java compiler ourselves. It is not needed to call ijar which is a c++ program (which we shouldn’t be calling this way anyway).
| in_scalacopts, | ||
| print_compile_time, | ||
| expect_java_output, | ||
| scalac_jvm_flags = []): |
There was a problem hiding this comment.
Why does the scalac_jvn_flags have a default value?
There was a problem hiding this comment.
We have virtually never used this in practice and I just put it in. Would you prefer I remove it?
There was a problem hiding this comment.
"we have virtually never used this in practice" you mean stripe, right? Because the code uses it.
There was a problem hiding this comment.
We can't just remove it since it's part of the signature. Maybe your default makes sense. It's mainly surprising that it's the only one with a default value so it can confuse a future reader about whether this means something in particular
There was a problem hiding this comment.
I meant removing the default. Of course we can (and I assume will) iterate.
There was a problem hiding this comment.
I'd suggest to keep it as is (if it's alright with you) and remove the default
|
lgtm, remove the default in a follow up again if no one objects seems reasonable toof |
this moves all the rest of the items I don't expect would be required out of the function accessing by ctx and instead by a parameter.
After this is merged, I will pick back up #510 which I think can be made to work using this PR.
Note, there are still some toolchain-ish stuff accessed on ctx, but I think that we will just have to add those to the thrift/proto generation rule until we have toolchains working correctly (see #530)