Add headers and move compiler interface #438
Conversation
build.sbt
Outdated
| @@ -288,7 +288,7 @@ lazy val zincCompileCore = (project in internalPath / "zinc-compile-core") | |||
| // defines Java structures used across Scala versions, such as the API structures and relationships extracted by | |||
| // the analysis compiler phases and passed back to sbt. The API structures are defined in a simple | |||
| // format from which Java sources are generated by the sbt-contraband plugin. | |||
| lazy val compilerInterface = (project in internalPath / "compiler-interface") | |||
| lazy val compilerInterface = (project in file("compiler-interface")) | |||
There was a problem hiding this comment.
Porting my comment from #428:
My opinion is that some datatype might be exposed, but in general the notion of compiler interface (abstraction over Scala compiler version) should be considered internal to Zinc. In other words, unless otherwise stated, we reserve the right to change compiler interface signature from Zinc 1.0 to 1.1, as long as it's not exposed to Zinc API.
There was a problem hiding this comment.
But the compiler interface does not only expose abstractions over Scala versions, it has all the facilities that are afterwards exposed to downstream users (loggers, reporters, scala instance, etc). Lots of these things also leak to sbt's API.
This goes to show that we should clearly define the boundary of what "Zinc API" is.
Logger, as you point out probably leaks out so it's likely part of the public API, so that should be included.
This could gradually increase later, but I think we should keep the promise of Zinc 1.0 to be that it's able to accept an array of Scala and Java source files repeatedly, and produce correct *.class files as the result. Everything else should be internal detail.
In other words, unless otherwise stated, we reserve the right to change compiler interface signature from Zinc 1.0 to 1.1, as long as it's not exposed to Zinc API.
This rule you propose is unexpected to me and doesn't satisfy the bincompat guarantees that other Scala tools do, like scalac. It's not even the same than the one for sbt, right?
sbt's binary compatibility responsibilities are mainly for compiled plugins. For that we guarantee binary compatibilities for sbt.* except sbt.internal.*.
Since the intended consumer of the compiler interface is the compiler bridge (xsbt.* classes), we should be able to add/remove things in the interface for various reasons.
There was a problem hiding this comment.
In any case, binary compatibility aside, I think this change is good. We currently have two top-level modules (one zinc and another one zinc-compile). The compiler interface is in my opinion the most important module in the project, and I think we should highlight it by placing it in the root of the project. It doesn't make sense to me that zinc-compile looks, at first glance, more important than the compiler interface.
zinc contains Zinc API, and zinc-compile should contain other related features such as ScalaDoc wrapper and Scala REPL wrapper, both intended as library someone can use.
There was a problem hiding this comment.
classes), we should be able to add/remove things in the interface for various reasons.
I think this is where we're not on the same page. The compiler interface contains interfaces that are used by all Zinc modules, not only the compiler bridge. You're probably right that the xsbt.api is not exposed and we can probably break it if we want to, but the majority of classes we cannot change in an binary incompatible way: IncOptions, CompileOptions, Companions, NameHash, etc.
This could gradually increase later, but I think we should keep the promise of Zinc 1.0 to be that it's able to accept an array of Scala and Java source files repeatedly, and produce correct *.class files as the result. Everything else should be internal detail.
sbt's binary compatibility responsibilities are mainly for compiled plugins. For that we guarantee binary compatibilities for sbt.* except sbt.internal.*.
My main fear is that breaking changes in the compiler interface will break downstream's user code (in Pants, Gradle, and other build tools that depend on us). We put a lot of effort into creating Java-friendly APIs that other tools like Maven can use, and not having strong bincompat guarantees on that code now is counterintuitive.
These days, I'm becoming more and more conservative about breaking APIs, and in this case I believe the consequences of breaking an API are worst than the advantages of improving it. I'm not sure we can expect our users to get compiler errors every time we make a new release of Zinc, for example.
It seems you want to cherry-pick concrete classes of the compiler interface and only ensure bincompat on those. I wonder two things:
- Is this possible at all? Our binary interfaces are so tightly coupled, that I believe it's challenging to cherry-pick interfaces in a reasonable and general way.
- What happens if you modify an interface that affects sbt plugins? It seems that you're saying that the compiler interface should be considered
sbt.internal, but isn't this going to create frustration for people interfacing with this code? Examples that come to mind aresbt-checkand Triplequote's sbt plugin (/cc @dragos @dotta). In my opinion, a build tool is all about compiling, and if we have bincompat guarantees at all they should focus on the compilation APIs.
There was a problem hiding this comment.
Thanks a lot for cc-ing us @jvican!
I'm currently not entirely clear on what are the source/binary incompatible changes that you may want to do now or in the future, but I can only agree with the general sentiment of this statement:
These days, I'm becoming more and more conservative about breaking APIs, and in this case I believe the consequences of breaking an API are worst than the advantages of improving it.
In our sbt plugin we replace the sbt compiler-bridge with our own hydra-bridge. The main change is the implementation of CompilerInterface, as it needs to call the Hydra Scala compiler instead of the vanilla Scala one. In the HydraPlugin we do depend on things like the ScalaInstance, Compiler.Inputs, CompileOptions and we also had to replace some sbt internals in order to swap the AnalysisCallback and filter compiler options that shouldn't trigger recompilation (these hacks won't be needed once #405 and #406 are resolved).
Let me know if there is anything I should check and report that can help the discussion.
This also proves that our sbt-header configuration works.
jvican
force-pushed
the
add-headers-restructure-dir
branch
from
059e9df to
0816162
Compare
|
@eed3si9n Can you review this please? |
|
LGTM |
This has been split from #428 to ease review.
It only does the following: