Improve Zinc logging API #325

jvican · 2017-06-28T08:50:02Z

This is the follow-up work to #304 and all the
comments that @stuhood has provided. Thank you for that.

Every commit is rich in details in its message. If you have any doubt about a
change, I invite you to read it.

I'm attaching a screenshot showing all the commits passing in Scala Center's
CI. I had to remove the first of the commits, and therefore all those passing
commits are lost.

jvican · 2017-06-28T08:50:58Z

jvican · 2017-06-28T10:49:35Z

internal/zinc-compile-core/src/main/scala/sbt/internal/inc/FilteredReporter.scala

+      filters.exists(_.findFirstIn(str).isDefined)
+
+    severity != Severity.Error && (
+      (pos.sourceFile.isPresent && isFiltered(fileFilters, pos.sourceFile.get.getPath)) ||


Note that this uses getPath (and not getAbsolutePath) on purpose.

jvican · 2017-06-28T11:06:18Z

CI is passing, I have requested three reviews. I need two to merge, as usual. I would appreciate feedback.

@eed3si9n @dwijnand I have tried to remove the Travis hook with no luck. It still happens to kick in for my PRs. If you happen to know why or know how to remove it, I would appreciate if you do so.

dwijnand · 2017-06-28T12:02:27Z

Disabled Travis CI for this repo.

jvican · 2017-06-28T16:24:59Z

It seems Travis CI is still enabled.

stuhood · 2017-06-28T17:18:24Z

Looks good to me! Thanks @jvican .

eed3si9n

Overall it looks good to me.
Could you open a corresponding PR for sbt so either Dbuild or I can validate it.

jvican · 2017-06-28T21:32:07Z

I'm happy to do it but I don't have access to dbuild. Can you give me permissions to do so?

eed3si9n · 2017-06-28T21:35:23Z

Paging @cunei for Dbuild access.
For now, if you have a PR, I can run the validation.

cunei · 2017-06-29T13:31:05Z

@eed3si9n any user can run validation now

jvican · 2017-06-29T13:52:56Z

@cunei Dbuild is not resolving dependencies.

@eed3si9n What's dbuild supposed to tell us? It's clear that this PR breaks sbt because the logger interface has changed. Is this something that you want to have for the posterity?

cunei · 2017-06-29T14:24:16Z

At the moment the test is not really run by dbuild; some more work is needed to make it work properly with sbt 1.0.0-M5/6. The test is currently just an 'sbt compile' done by this script: https://github.com/sbt/sbt/blob/1.0.x/sbt-allsources.sh @dwijnand
You can specify in the build form multiple pull requests for the various components, and the script (and in the future dbuild) will check the changes in all the referenced modules together. For example: sbt/sbt#3294 (comment)

jvican · 2017-06-29T14:25:34Z

@cunei Thanks for the explanation. My question really is what's the relation between dbuild and getting this PR merged. /cc @eed3si9n

eed3si9n · 2017-06-29T19:18:06Z

@jvican See sbt/util#85 (comment). This allows us to validate multiple PRs together and make sure that in conjunction they are able to compile sbt. This is to make sure that modules changes propagate within short window of time instead of catching breaking changes at the last minute.

jvican · 2017-06-29T19:19:40Z

This allows us to validate multiple PRs together and make sure that we in conjunction it's able to compile sbt.

Gotcha, so this would include sbt PRs too.

Can we please proceed with the merge of this PR? I have work on top of this and I need it to be in so that I can continue at a fast pace.

eed3si9n · 2017-06-29T19:27:14Z

Can we please proceed with the merge of this PR?

As I wrote 22h ago, could you open a corresponding PR for sbt so either Dbuild or I can validate it?

jvican · 2017-06-29T19:32:19Z

As I wrote 22h ago, could you open a corresponding PR for sbt so either Dbuild or I can validate it?

Didn't get what you were asking me. So you are asking me to create a PR to sbt migrating to my new Zinc changes. Okay.

eed3si9n · 2017-06-29T19:33:41Z

Yea. FWIW I asked Martin to do the same for his watcher stuff too sbt/sbt#3295

jvican · 2017-06-29T19:34:51Z

@cunei Is dbuild also doing Test/compile?

cunei · 2017-06-29T20:57:20Z

As I wrote, it's not really dbuild running in the validator, right now. At the moment the validator script it is only running 'sbt compile';my understanding is that 'sbt test', in that script, would not run the scripted tests. Once the real dbuild is working ok with 1.0.0-M6, validation will also run all the tests of the various modules.

jvican · 2017-06-29T21:01:48Z

As I wrote, it's not really dbuild running in the validator, right now. At the moment the validator script it is only running 'sbt compile';my understanding is that 'sbt test', in that script, would not run the scripted tests. Once the real dbuild is working ok with 1.0.0-M6, validation will also run all the tests of the various modules.

I'm asking if dbuild compiles the tests too (hence my previous reference to Test/compile). You said that dbuild is only doing sbt compile and you should compile tests too if you're compiling sources.

cunei · 2017-06-29T23:19:24Z

No testing is currently done, just compilation of the main code. The tests are not currently being compiled, I believe.

jvican · 2017-06-29T23:24:18Z

No testing is currently done, just compilation of the main code. The tests are not currently being compiled, I believe.

Can you please compile tests too? Otherwise, we don't have the guarantee that the PR passes at least all the compilation steps.

cunei · 2017-06-29T23:32:56Z

I changed sbt compile to sbt ";compile;test:compile"

This does not change semantics.

This API is inspired by the sbt backend that implements the whole machinery for `ManagedLogger` and instantiates `LoggerReporter`. The provided functionality is to get a default reporter configuration that customises the default instantiation of the reporter. The instantiation is provided in the same API. We use contraband for the default reporter configuration to get the builder pattern so that users can modify it more easily.

As explained in the issue and discussions [here](sbt#304 (comment)).

* Change `Reporter` to log problems (zxsbti.Problem`). * Move `convert` to object (needs to be accessible from both logger and bridge).

The use of display should be completely private, no matter who uses it. It's weird that a logger has a concept of `display`. It's a bad abstraction because the underlying logger could not "display". Note that `display` in `LoggerReporter` has been made private despite the fact that sbt uses it for printing warnings via `printWarnings`. The use of this function reveals a small bug in the sbt codebase. In `Defaults.scala`, sbt does the following: ``` def printWarningsTask: Initialize[Task[Unit]] = Def.task { val analysis = compile.value match { case a: Analysis => a } val max = maxErrors.value val spms = sourcePositionMappers.value val problems = analysis.infos.allInfos.values.flatMap(i => i.getReportedProblems ++ i.getUnreportedProblems) val reporter = new LoggerReporter(max, streams.value.log, foldMappers(spms)) problems foreach { p => reporter.display(p) } } ``` There's an error in the previous code since position mappers are not used when "displaying" logs to the user, the use of the underlying position mapper (provided by `foldMappers`) happens in `log`. As we have already rewritten the `log` interface to accept `Problem`, sbt can depend directly do instead: ``` problems foreach { p => reporter.display(p) } ``` which will solve that minor bug. Note that there's a semantic difference between `display` and `log`. `log` will persist every problem in a `problems` list. This however does not change the semantics of the only caller of sbt since a new reporter is created every time `printWarnings` is invoked, so that extra work does not bother us.

The bridge has been compiled with the sbt module dependencies on the classpath for ages. It seems that these dependencies trace back to the time where Zinc was developed in sbt/sbt. https://github.com/sbt/zinc/blame/4d4d83c636fb74516b6686f199fa19174adaea2d/build.sbt#L150 Having dependencies for the compiler bridge means two things: * Users pay a penalty for resolving those artifacts. * Compiler may be less efficient because of extra classpath entries (?) * Zinc developers will be confused if they depend on something from those modules since bridge compilation will fail at runtime because dependencies will fail to link. Previous to this change, the compiler bridge was being compiled with the following classpath entries: ``` Vector(/tmp/sbt_af3b3cae/jars/org.scala-sbt/compiler-interface/compiler-interface-1.0.0-X16-SNAPSHOT.jar, /tmp/sbt_af3b3cae/jars/org.scala-sbt/util-interface/util-interface-1.0.0-M24.jar, /tmp/sbt_af3b3cae/jars/org.scala-sbt/io_2.12/io_2.12-1.0.0-M11.jar, /tmp/sbt_af3b3cae/jars/org.scala-lang/scala-library/scala-library-2.12.1.jar, /tmp/sbt_af3b3cae/jars/org.scala-sbt/util-logging_2.12/util-logging_2.12-1.0.0-M24.jar, /tmp/sbt_af3b3cae/jars/org.scala-sbt/util-collection_2.12/util-collection_2.12-1.0.0-M24.jar, /tmp/sbt_af3b3cae/jars/com.eed3si9n/sjson-new-core_2.12/sjson-new-core_2.12-0.7.1.jar, /tmp/sbt_af3b3cae/jars/jline/jline/jline-2.14.3.jar, /tmp/sbt_af3b3cae/jars/org.apache.logging.log4j/log4j-api/log4j-api-2.8.1.jar, /tmp/sbt_af3b3cae/jars/org.apache.logging.log4j/log4j-core/log4j-core-2.8.1.jar, /tmp/sbt_af3b3cae/jars/com.lmax/disruptor/disruptor-3.3.6.jar, /tmp/sbt_af3b3cae/jars/com.eed3si9n/sjson-new-scalajson_2.12/sjson-new-scalajson_2.12-0.7.0.jar, /tmp/sbt_af3b3cae/jars/org.mdedetrich/scala-json-ast_2.12/scala-json-ast_2.12-1.0.0-M6.jar, /tmp/sbt_af3b3cae/jars/org.spire-math/jawn-parser_2.12/jawn-parser_2.12-0.10.4.jar, /tmp/sbt_af3b3cae/jars/org.scala-lang/scala-reflect/scala-reflect-2.12.1.jar) ``` Right now, only two jars are brought in, and the two are Java dependencies (the compiler interface and the util/logging interface).

sbt#224 introduced support for event logging. However, this support forced downstream users depending on `LoggerReporter` to provide a `ManagedLogger`. It also made it impossible for Zinc users to create their own reporter because `ManagedLogger` is part of the internal sbt API. This commit factors out the event logging functionality out of the `LoggerReporter`. Users that wish to pass a `ManagedReporter` must use `ManagedLoggerReporter` which will set the logger up with all the requirements to successfully log problems.

Since we have changed the signature of `LoggerReporter`, we don't want our users to pass in a `ManagedLogger` that hasn't been properly set up with the required event codecs. Currently, people depending on `LoggerReporter` will assume that logger reporter will set the logger up, but now it will not, since its signature has been changed to accept a `xsbti.Logger`, which is a super type of `ManagedLogger`. As the types match, typechecking will work and people will find crappy exceptions at runtime because their `ManagedLogger`s have not been set up. Therefore, we change the name from `LoggerReporter` to `LoggedReporter` to avoid this scenario. I also find the new name a little bit more meaningful that the combination of two nouns. It's clear that it's a reporter that uses an underlying log.

The managed filtered reporter is the counterpart of filtered reporter but appropriately setting up a managed logger.

Contrary to the previous API, now it's possible to get a reporter out of a dumb `xsbti.Logger` that any user of the API can easily extend. This new method covers almost all the needs that third-party developers need to get Zinc working.

jvican · 2017-07-13T15:17:02Z

@Duhemm I've addressed all your comments.

@jastice You can have a look at this one now, it's been updated. The required changes concern only the compiler logger API in sbt/sbt, changes to get this in should be really small, sbt only needs ManagedLoggedReporter.

sbt/zinc#325

typesafe-tools · 2017-07-14T12:51:15Z

dbuild has checked the following projects against Scala 2.12:

Project	Reference	Commit
sbt	pull/3316/head	sbt/sbt@`0cc3cfb`
zinc	pull/325/head	`510d64d`
io	1.0	sbt/io@`f55849d`
librarymanagement	74d8a3835d535d3d787e9294539a0a3efc47674d	sbt/librarymanagement@`74d8a38`
util	1.0	sbt/util@`fe77438`
website	1.0.x	sbt/website@`7e644bc`

❌ The result is: FAILED
(restart)

Enable more compiler warnings, apply respective fixes

jvican commented Jun 28, 2017

View reviewed changes

jvican requested review from eed3si9n, dwijnand and Duhemm June 28, 2017 11:05

jvican closed this Jun 28, 2017

jvican reopened this Jun 28, 2017

eed3si9n reviewed Jun 28, 2017

View reviewed changes

jvican added the binary api label Jun 30, 2017

jvican added 14 commits July 13, 2017 16:58

Use identity and remove unnecessary param

d5c3cb8

This does not change semantics.

Fix sbt#319: Add a filtered compiler reporter

92521ac

As explained in the issue and discussions [here](sbt#304 (comment)).

Use ReporterManager for AnalyzingCompiler

cfcdee1

Add getReporter methods to ReporterUtil

d7924b9

Add some code style changes

1e08108

Clean up DelegatingReporter

ec7b873

* Change `Reporter` to log problems (zxsbti.Problem`). * Move `convert` to object (needs to be accessible from both logger and bridge).

Add way to disable scripted buffer from sbt

f0a5221

Add a ManagedFilteredReporter

f6fb64b

The managed filtered reporter is the counterpart of filtered reporter but appropriately setting up a managed logger.

Add public API to get reporter from Logger

923bd3d

Contrary to the previous API, now it's possible to get a reporter out of a dumb `xsbti.Logger` that any user of the API can easily extend. This new method covers almost all the needs that third-party developers need to get Zinc working.

jvican force-pushed the fix-loggers branch from b6da4d5 to bf1a634 Compare July 13, 2017 15:15

Address Martin's feedback

510d64d

jvican force-pushed the fix-loggers branch from bf1a634 to 510d64d Compare July 13, 2017 15:26

jvican added a commit to scalacenter/sbt that referenced this pull request Jul 14, 2017

Synchronize with changes to Zinc logger API

0cc3cfb

sbt/zinc#325

jvican mentioned this pull request Jul 14, 2017

Synchronize with changes to Zinc logger API sbt/sbt#3316

Merged

typesafe-tools mentioned this pull request Jul 14, 2017

Detangle the dependencies sbt/librarymanagement#122

Merged

eed3si9n approved these changes Jul 14, 2017

View reviewed changes

eed3si9n merged commit 9b0a4e4 into sbt:1.0 Jul 14, 2017

stuhood mentioned this pull request Jul 14, 2017

Design a Java-friendly, safe Zinc API for build tools #300

Closed

6 tasks

cunei pushed a commit to cunei/zinc that referenced this pull request Oct 25, 2017

Merge pull request sbt#325 from non/sirthias-no-warnings

99c0123

Enable more compiler warnings, apply respective fixes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Zinc logging API #325

Improve Zinc logging API #325

jvican commented Jun 28, 2017

jvican commented Jun 28, 2017

jvican Jun 28, 2017

jvican commented Jun 28, 2017

dwijnand commented Jun 28, 2017

jvican commented Jun 28, 2017

stuhood commented Jun 28, 2017

eed3si9n left a comment

jvican commented Jun 28, 2017

eed3si9n commented Jun 28, 2017

cunei commented Jun 29, 2017

jvican commented Jun 29, 2017

cunei commented Jun 29, 2017

jvican commented Jun 29, 2017

eed3si9n commented Jun 29, 2017 •

edited

Loading

jvican commented Jun 29, 2017

eed3si9n commented Jun 29, 2017

jvican commented Jun 29, 2017

eed3si9n commented Jun 29, 2017 •

edited

Loading

jvican commented Jun 29, 2017

cunei commented Jun 29, 2017 •

edited

Loading

jvican commented Jun 29, 2017

cunei commented Jun 29, 2017 •

edited

Loading

jvican commented Jun 29, 2017

cunei commented Jun 29, 2017

jvican commented Jul 13, 2017

typesafe-tools commented Jul 14, 2017

Improve Zinc logging API #325

Improve Zinc logging API #325

Conversation

jvican commented Jun 28, 2017

jvican commented Jun 28, 2017

jvican Jun 28, 2017

Choose a reason for hiding this comment

jvican commented Jun 28, 2017

dwijnand commented Jun 28, 2017

jvican commented Jun 28, 2017

stuhood commented Jun 28, 2017

eed3si9n left a comment

Choose a reason for hiding this comment

jvican commented Jun 28, 2017

eed3si9n commented Jun 28, 2017

cunei commented Jun 29, 2017

jvican commented Jun 29, 2017

cunei commented Jun 29, 2017

jvican commented Jun 29, 2017

eed3si9n commented Jun 29, 2017 • edited Loading

jvican commented Jun 29, 2017

eed3si9n commented Jun 29, 2017

jvican commented Jun 29, 2017

eed3si9n commented Jun 29, 2017 • edited Loading

jvican commented Jun 29, 2017

cunei commented Jun 29, 2017 • edited Loading

jvican commented Jun 29, 2017

cunei commented Jun 29, 2017 • edited Loading

jvican commented Jun 29, 2017

cunei commented Jun 29, 2017

jvican commented Jul 13, 2017

typesafe-tools commented Jul 14, 2017

eed3si9n commented Jun 29, 2017 •

edited

Loading

eed3si9n commented Jun 29, 2017 •

edited

Loading

cunei commented Jun 29, 2017 •

edited

Loading

cunei commented Jun 29, 2017 •

edited

Loading