Skip to content
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

Add the first iteration of the Zinc API 1.0 #304

Merged
merged 51 commits into from May 26, 2017

Conversation

Projects
None yet
3 participants
@jvican
Copy link
Member

commented May 22, 2017

This is just the first iteration of the Zinc API.

This iteration's goal is to make safe functionality of Zinc public to allow Scala and Java parties (Gradle, Pants, Maven, et cetera) use Zinc with binary compatibility guarantees. This task needs another iteration given:

  1. The big surface of the API; and,
  2. The fact that the whole Zinc API was made private for 1.0 (!).

The last iteration will happen before RC1 (end of June - beginning of July).

The new public API makes it easier to:

  • Get a compiled bridge for a given scala instance (via ivy).
  • Get a scala instance (via ivy).
  • Instantiate a Zinc compiler.
  • Provide utilities for both Java and Scala users to get default implementations that do not expose the internals, placed in sbt.internal.inc.

Aside from these, this API also addresses some issues that have arisen in the past:

  • It makes it easier to customize the classfile manager, as required by Scalajs and Scala Native. #125
  • It replaces any instance of Maybe with Optional. #302

I find that in general, these commits make Zinc easier to use and improve the consistency of the whole API. The most remarkable change is the CompileAnalysis API, which now provides useful information for build tools like file stamps, compilations and source infos.

Note that all the utilities made public only expose functionality that is a product of the incremental compiler. No internal details of the incremental analysis are exposed to the users.

There are some Java files that have been moved out of the compiler interface because they required static method implementations in the projects on which the implementations of certain classes were provided.

In the future, I feel that these are the things that should be worked on to finish off the Zinc API:

  1. Reorder project structure. Simplify it and make it more granular and easier to test. #286
  2. Decide what and how to use to persist analysis files with appropriate benchmarking. #268 (protobuf is not possible, so we need to find a way to either use sbinary or a similar solution that does not harm (or even improve!) speed of compiler iterations).
  3. Create utilities to instantiate:
    3.1. Empty Analysis.
    3.2. DefinesClass.
    3.3. AnalysisStore (up to discussion for next iteration).
    3.4. JavaTool et al, necessary to create Compilers.
    3.5. PerClasspathLookupEntry.
  4. Make analysis content machine-independent: #218

I have more ideas, but those will be discussed when time's up.

The breaking changes are documented in every commit. I suggest that reviewers go through the commits one by one, though I don't expect them to do so (given the big diff size). As this is just the first iteration of the redesign and targets a milestone, I am not writing any migration notes for now. I'll wait until everything is finished in our first RC to write the migration notes so that other build tools (other than sbt) can start using it.

As a reminder, the public API is xsbti. Scala implementations live in sbt.internal.inc.

This is a good time to gather feedback. So that's very much welcome.

jvican added some commits May 15, 2017

Remove deprecated compatibility stubs
Remove unused `GlobalCompat` for compatibility with 2.8.1 and extract
the run subclassing out of the main logic of the `run` method.
Make Zinc `Global` implementations independent
The callback implementation should be independent of the compiler
interface and the source code should reflect so.

Breaking change in comparison with the previous API: `Compiler` is now
called `ZincCompiler` to make the naming more clear and better
distinguish between `CachedCompiler` and its underlying `ZincCompiler`.

Docs have been added and simplifications to methods have been provided
and double checked with previous Scala versions. For instance, `dropRun`
has been confirmed to only exist in 2.10.x series and removed in 2.11.x.
Therefore, its implementation has been moved to the compatibility traits
for 2.10.x series.  We then remove a reflective call for versions above
2.10.6.
Port `CompileAnalysis` from contraband to Java
We need `CompileAnalysis` to start creating the Java API with good docs.
Contraband is not necessary for the analysis since we only
serialize/deserialize `MAnalysis`, its subclass.
Change `Stamps` interface and functionality
* Make `ReadStamps` a Java interface exposed in the public API.
* Create a `Stamp` interface that Java users can use without any
  knowledge of the internal requirement.
* Add docs to the pertinent Java interface.
* Change `Stamp` companion object by `Stamper` to make it Java-friendly.
  I also think the name is clearer about the contents of the object.
* Rename `internalSouce` to `source` for consistency with the Scala API.
  It's made clear that source should always be an internal source in the
  docs. This makes sense anyway, otherwise it wouldn't make sense to ask
  for the stamp of a source file that is not compiled in the current
  analysis.
Remove unused stamp `Exists`
`Exists` has never been used, neither internally in Zinc nor in sbt and
pants (I double checked on both).

The concept of `exists` does not make sense, since there is no value in
asserting that a stamp "exists" only in asserting that a stamp is
**not** available for a given file.

The latter is represented by the new `Stamp` API since last modified
time and hash are both optional. If both are empty, a stamp does not
exist.
Turn `CompileAnalysis` to an interface
`CompileAnalysis` should be an interface instead of an abstract class.
Add `ReadStamps` to `CompileAnalysis`
Users of `CompileAnalysis` will have access to the stamps of the files
that have been processed by Zinc.
Create `ReadSourceInfos` & add to `CompileAnalysis`
`ReadSourceInfos` is a read-only interface for getting source info
(namely, compiler errors) associated with a given file.

This is very useful in lots of cases. Especially to process and improve
error messages in a lightweight way, though ScalaClippy already exists
for that purpose.

In any case, having access to the compiler errors outside of is of
ultimate importance for tools that want to preprocess compilation
results independently of sbt.
Replace `OutputSetting` by `OutputGroup`
`OutputSetting` is like `OutputGroup` by just returns strings instead of
files. I see no reason why `OutputGroup` should not be used instead,
therefore simplifying the Zinc API and allowing users to have access to
files and not mere strings. If they want strings, they always can do
`getAbsolutePath`.

N.B. This is good for machine independence.
Fix `Output` and `Compilation` API
* Remove dangerous use of output groups as fields in compilation. In the
  case of `SingleOutput`, the source directory was invented and
  incorrect (the root). Users of the API processing this file could
  create chaos in people's computers. The new design forces consumers
  of the `Compilation` API to make up their minds and identify whether
  the compilation was used with a single output or several outputs.
  In order to do that, we return `Output` instead of an array of
  `OutputGroup`.
* Rename `outputDirectory` and `sourceDirectory` to be Java friendly.
* Augment `Output` interface with Java-friendly methods. Scala users
  can use pattern matching instead.
Make the Output API more Java friendly
* Make methods in `Output` subclasses public
* Changes to the API. Java idiomatic renames:
  * sourceDirectory -> getSourceDirectory
  * outputDirectory -> getOutputDirectory
Add `ReadCompilations` to `CompileAnalysis`
* `Compilation` has been ported to Java code and documented.
* `ReadCompilations` is a read-only API of `Compilations`.
* Add a TODO to check the ordering of the array of compilations.
* Expose `ReadCompilations` to the `CompileAnalysis`.
Add `CompilerBridgeProvider` interface and misc.
* Add interface for the provider.
* Rename `IvyComponentCompiler` to `ZincComponentCompiler`.
* Split `ZincComponentCompiler` and `ZincComponentManager`.
* Define `IfMissing` and `InvalidComponent` into independent files.
* Rename variables and internal API to be clearer.
Move `constant` to static in `CompilerBridgeProvider`
No need to have it in the internal Scala side.
Rename `getCompilerSources` to `getCompiledBridge`
Previous name was incorrect, we actually return the compiled bridge.
Test `getScalaInstance` & `getCompiledBridge`
* Remove unused code in `BridgeProviderSpecification` and test the real
  code in the default provider.
* Don't use temporary directory to download the Scala jars, retrive them
  in a directory specified by the user.
* Use `java.net.ClassLoader` instead of `ScalaClassLoader`.
* Use the `ScalaInstance` interface, not the implementation. Remove any
  reference to the implementation.
Compile bridge for 2.12.x and other Scala versions
This commit makes sure that we always check that we compile against the
latest two versions in Scala 2.10, Scala 2.11 and Scala 2.12.

This commit also removes the null loggers and uses normal loggers to
check what the output of the bridge provider is. This output is
important and the rationale to do this is that it should be visible for
us in order to detect cases where there's a regression or the output is
modified in some way. Output is short anyway.
Add Java API for Zinc Ivy integration
* Adds a method to fetch the default implementation of the compiler
  provider.
* Adds a method to get a default `IvyConfiguration` from a small number
  of parameters.

The public API is implemented in Java to ensure binary compatibility no
matter what happens to the internal Scala implementations. It also makes
it easier for Java users to use the API and avoid importing MODULE$'s
and Scala generated class files.'
@jvican

This comment has been minimized.

Copy link
Member Author

commented May 26, 2017

Feedback has been addressed @eed3si9n.

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 26, 2017

Tests are green.

@eed3si9n

This comment has been minimized.

Copy link
Member

commented May 26, 2017

Thanks for all the work on this.

@eed3si9n eed3si9n merged commit 931b57c into sbt:1.0 May 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eed3si9n eed3si9n removed the in progress label May 26, 2017

@stuhood

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2017

With regard to additional private/internal APIs that are currently in use, can browse through https://github.com/pantsbuild/pants/tree/master/src/scala/org/pantsbuild/zinc (as of zinc 1.0.0-X7) ... I'm currently working on updating to include this patch.

@jvican

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2017

@stuhood I've been browsing through that for a while, but thanks for the heads up. Are you in a hurry to do this? I'll be tackling the second iteration of the API soon before the release candidate and the API freeze, so maybe you want to wait until that's out?

@eed3si9n

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

The change I made to sbt is sbt/sbt#3228

@stuhood

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2017

@jvican : It would probably make sense for me to continue to attempt this, just in order to find any additional issues that you might not have seen. As an example, yesterday I noticed that it's not currently possible to extend LoggerReporter in a useful way in X16, because various methods were made private. So https://github.com/pantsbuild/pants/blob/master/src/scala/org/pantsbuild/zinc/logging/Reporters.scala#L28 is no longer possible. A theoretical FilteredReporter wrapper might make more sense anyway though.

@jvican

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2017

So https://github.com/pantsbuild/pants/blob/master/src/scala/org/pantsbuild/zinc/logging/Reporters.scala#L28 is no longer possible. A theoretical FilteredReporter wrapper might make more sense anyway though.

Great, I'll make this (or something similar, I have to look into the details) happen to unblock that implementation.

@stuhood

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2017

@jvican: A few more details here:

  1. the Reporter API seems to require a ManagedLogger, but it's not clear how (or whether it would be a good idea) to create one of those, versus passing in an xsbti.Logger or sbt.util.Logger
  2. Because a Reporter is a required parameter for IncrementalCompilerImpl.compile (and there doesn't appear to be a way to create a LoggerReporter without a ManagerLogger), it does not currently seem to possible to compile using the 18 arg form.

I'm going to try porting to usage of xsbti.compile.Inputs instead.

@stuhood

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2017

Alright, I ported to xsbti.compile.Inputs, and it cleared things up considerably. But the above issue with LoggerReporter and ManagedLogger is still an issue.

Also, it doesn't look like it's possible to use a "default" Reporter (as it is with Progress, which is Optional)... it's conceivable that at some point in the future we'll move the log filtering out of the Reporter, at which point being able to use a default by specifying None would be good.

@jvican

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2017

@stuhood: Yes, it seems that the entanglement between LoggerReporter and ManagedLogger is due to sbt. It's an API where sbt's design decisions leak too much.

To overcome this issue, some of Zinc's test suites create a ConsoleReporter that is a direct subclass of Reporter and just printlns, but that's far from desirable. I think I am going to create a ManagedLogger for Zinc and expose it somewhere, or at least make it trivial to create a Zinc reporter. I am working on it as we speak.

With regards to making the reporter Optional, I have my doubts this is the best choice. It's such a key component of the compilation that I would prefer that users explicitly pass it. Also, depending on the use case, it's likely to be changed. What is a safe assumption for the user? Will Zinc output to my console? Since it's not obvious, I believe users may be better off being more explicit about what they want. I guess that you need for this would disappear if we add methods to instantiate zinc reporters that you can pass in. WDYT?

@jvican

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2017

For the record, if you want to move forward and need a ManagedLogger, you can do sbt.util.LoggerExchange.logger(...).

jvican added a commit to scalacenter/zinc that referenced this pull request Jun 20, 2017

Fix sbt#319: Add a filtered compiler reporter
As explained in the issue and discussions [here](sbt#304 (comment)).
@stuhood

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2017

What is a safe assumption for the user? Will Zinc output to my console? Since it's not obvious, I believe users may be better off being more explicit about what they want. I guess that you need for this would disappear if we add methods to instantiate zinc reporters that you can pass in. WDYT?

Yea, that makes sense. I'm still thinking of zinc as a CLI tool, but in the context of a library, being explicit about output location is important... ditto logging.

stuhood added a commit to twitter/pants that referenced this pull request Jun 21, 2017

@jvican

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2017

Hey @stuhood, I've been fixing all the issues you've reported regarding logging. I think I have come up with some elegant solutions you may want to have a look at.

This is the most important change: scalacenter@a9a9c2f.

All the changes are in this branch: 1.0...scalacenter:fix-loggers.

I can release these changes as part of a temporary scalacenter/zinc release to unblock you and get more feedback. These changes will need, of course, to be merged upstream in sbt/zinc.

In the new interface, I have added a filtered reporter in Zinc and the normal LoggerReporter (now renamed to LoggedReporter, see scalacenter@cac0149). LoggedReporter now accepts xsbti.Logger, meaning that you can pass in whatever logging mechanism you want so long as it extends that interface. These reporter implementations are internal, but you can get a reporter backed up by a logger via the public ReporterUtil: scalacenter@60820b1.

Let me know if this public API works for you.

@stuhood

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2017

Thanks @jvican . The API looks great (with one exception mentioned below): thanks for incorporating file/msg based filtering.

I should be able to sanity check incremental compile before the end of Monday.

There is a bit of inconsistency between java.util.function.Function and xsbti.F1 for positionMappers in ReporterUtil and CompileOptions. pants currently only requires JDK7, but I'm floating the idea of switching to JDK8 now... Function should be fine, but consistency would be good.

@jvican

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2017

@stuhood Happy that you mention that, I opened a PR in sbt/util to address that issue three days ago. The idea is to remember any trace of the old Java-friendly classes for bincompat (F0, F1 and Maybe) and replace them by the default Java ones (Supplier, Function and Optional).
sbt/util#84

@stuhood

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2017

The new API looks good from a logging/usability perspective (other than #317). I have things working well enough to have reproduced pantsbuild/pants#4477, so I will begin debugging that tomorrow.

@stuhood

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

@jvican : If you have a PR for the Reporters/Logger change, I'd shipit. I'm getting close to needing an X17 release to move forward.

@jvican

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2017

Shipping it tomorrow morning. It's ready but kept it local because I'm changing more things in the API.

jvican added a commit to scalacenter/zinc that referenced this pull request Jun 28, 2017

Fix sbt#319: Add a filtered compiler reporter
As explained in the issue and discussions [here](sbt#304 (comment)).

@jvican jvican referenced this pull request Jun 28, 2017

Merged

Improve Zinc logging API #325

jvican added a commit to scalacenter/zinc that referenced this pull request Jul 13, 2017

Fix sbt#319: Add a filtered compiler reporter
As explained in the issue and discussions [here](sbt#304 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.