Skip to content

More detailed logging in ReportWriter #1

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kwestor
Copy link

@Kwestor Kwestor commented Jan 30, 2015

Log messages with paths to files with reports added for each version.

THE FOLLOWING DISCLAIMER APPLIES TO ALL SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE:
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS “AS IS” AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.
ONLY THE SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE, IF ANY, THAT ARE ATTACHED TO (OR OTHERWISE ACCOMPANY) THIS SUBMISSION (AND ORDINARY COURSE CONTRIBUTIONS OF FUTURES PATCHES THERETO) ARE TO BE CONSIDERED A CONTRIBUTION. NO OTHER SOFTWARE CODE OR MATERIALS ARE A CONTRIBUTION.

@Kwestor
Copy link
Author

Kwestor commented Jan 30, 2015

@maiflai It's very useful to have those messages in log and not to look for report files, especially if you work on non-standard environment where gradle output is not simply placed in build.

@maiflai
Copy link
Owner

maiflai commented Jan 31, 2015

Sorry, I think it would be preferable to use an slf4j Logger rather than println as the messages can then be disabled if required?

@Kwestor
Copy link
Author

Kwestor commented Jan 31, 2015

Sure. Do you know if there is a logger already set up in scalac-scoverage or do I have to add one?

@maiflai
Copy link
Owner

maiflai commented Jan 31, 2015

SBT: org.slf4j:slf4j-api:1.7.7:jar is present

@Kwestor
Copy link
Author

Kwestor commented Jan 31, 2015

I just checked scalac-scoverage code, and it uses println everywhere to log messages (see plugin.scala:85).

I think if you want to include some kind of logger in it should be done on separate task and to whole plugin, not only this one file. What do you think, @sksamuel?

@sksamuel
Copy link

I don't want to pull in dependencies into the compiler plugin, as then people must add those to the build as well. I could perhaps log to the reporter instead which is what the compiler uses.

@Kwestor
Copy link
Author

Kwestor commented Jan 31, 2015

I've changed log messages to use reporter in scoverage#94, if this is ok for you I can change this PR also.

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

This is good, but lets update to reporter like you said.

@Kwestor
Copy link
Author

Kwestor commented Feb 1, 2015

Hm.. but the reporter sits in Global, which is inside main plugin class. ReportWriter is called from Reports, which does not have a Global instance anywhere, am I correct?

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

Maybe we should add an abstraction so that sbt/gradle/maven call it with
some instance of "Log"

On 1 February 2015 at 20:06, Jerzy Müller notifications@github.com wrote:

Hm.. but the reporter sits in Global, which is inside main plugin class.
ReportWriter is called from Reports, which does not have a Global
instance anywhere, am I correct?


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

I think that the slf4j logger is already on the classpath? Gradle uses slf4j, but SBT might need a little help to mediate between the TaskStream Logger and the slf4j one.

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

Not in the scalac-compiler-plugin its not. Two options - a separate report
module, used only by the build tools, which can have any deps we want. Ie,
the report generators move to scalac-scoverage-reporters. Or second option
is that we have a trait called Logger which has a method called log which
we can send an anon instance of when invoking report methods.

On 1 February 2015 at 20:54, maiflai notifications@github.com wrote:

I think that the slf4j logger is already on the classpath? Gradle uses
slf4j, but SBT might need a little help to mediate between the TaskStream
Logger and the slf4j one.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

ah, sorry - agree it's only a test dependency. I think I prefer having a separate project with its own dependencies, rather than defining another Logger.

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

Too many logging frameworks = plight of Java. A trait with one method might
not be so bad.
The only downside to the reporters module is that we then need a domain
module for sharing the domain classes between reporters and the scalac
plugin. That's why I didn't do it before.

On 1 February 2015 at 21:13, maiflai notifications@github.com wrote:

ah, sorry - agree it's only a test dependency. I think I prefer having a
separate project with its own dependencies, rather than defining another
Logger.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

I think I could just about live with the reporting module bringing a scala tools dependency via the plugin module - it's not as if this is going to be used in isolation (clients will have already downloaded and cached those dependencies for the compilation).

Presumably with separate projects you'd rename the existing project, move plugin.scala to a new plugin project and move report/* to a new report project?

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

The project is already called plugin, at the moment you have

scalac-scoverage-plugin
scalac-scoverage-runtime

so you could then have

scalac-scoverage-plugin
scalac-scoverage-runtime
scalac-scoverage-reporters
scalac-scoverage-domain

Or

trait ScoverageLogger {
def log(msg:String)
}

On 1 February 2015 at 21:41, maiflai notifications@github.com wrote:

I think I could just about live with the reporting module bringing a scala
tools dependency via the plugin module - it's not as if this is going to be
used in isolation (clients will have already downloaded and cached those
dependencies for the compilation).

Presumably with separate projects you'd rename the existing project, move
plugin.scala to a new plugin project and move report/* to a new report
project?


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

I wondered if we might log things at more than one level - otherwise you can just keep println as Gradle allows you to control how STDOUT is handled.

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

That's true, we can do the usual warn error etc.

On 1 February 2015 at 21:46, maiflai notifications@github.com wrote:

I wondered if we might log things at more than one level - otherwise you
can just keep println as Gradle allows you to control how STDOUT is handled.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

Actually, the reporters module could depend on plugin, then you wouldn't
need domain.

On 1 February 2015 at 21:47, Stephen Samuel (Sam) sam@sksamuel.com wrote:

That's true, we can do the usual warn error etc.

On 1 February 2015 at 21:46, maiflai notifications@github.com wrote:

I wondered if we might log things at more than one level - otherwise you
can just keep println as Gradle allows you to control how STDOUT is handled.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

Yes, but then it will bring in the Plugin class and its dependencies, i.e. the scala tool chain. It's not the end of the world, but it's not clean.

Thinking again about the original problem, I'm not sure the reporting module should print out the location of its files. It is told where to put the reports, so I wonder if it's more appropriate for Gradle, SBT etc to notify the user in an appropriate manner??

@Kwestor
Copy link
Author

Kwestor commented Feb 1, 2015

Yes, but the whole reason of this ReportWriter is not to duplicate code in all those plugins. You have 4 different outputs, all can be turned on/off and duplicating this ifs in all build tools just not to have println but some logger output seems like an overkill to me.

@Kwestor
Copy link
Author

Kwestor commented Feb 1, 2015

An btw - is the case where you want to fine-tune your build output this frequent?

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

The lesser of all evils in my mind is to have the build tools do the
reporting or have the trait.
The module split up, whilst good in theory, might be ugly in practice. But
those are our current three options.

On 1 February 2015 at 21:59, Jerzy Müller notifications@github.com wrote:

An btw - is the case where you want to fine-tune your build output this
frequent?


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

I typically configure TeamCity or Jenkins to collect the coverage report for me, so I don't need the exact location of the entry point to the report to be printed on the command line. Just knowing the report directory, and then following the folder structure from there would be enough.

Typically I prefer the builds to be quiet, so that any output is an indication that something has gone wrong, and contains information to help find the issue.

Others have indicated that they want to upload the reports into Sonar Qube, which again would be automated.

@Kwestor
Copy link
Author

Kwestor commented Feb 1, 2015

I've added those logs because I like to crtl+click in the terminal/copy path and go into browser while I'm working on adding scoverage to new projects (and I have a lot of such work right now, like 70+sub-modules).

I you think it will be better to include a bunch of if's in 3 plugins and copy those messages there I can create a PR to sbt and gradle version (I know nothing about maven) after you conclude in which form you want scoverage#82 merged into scalac-scoverage.

@Kwestor
Copy link
Author

Kwestor commented Feb 1, 2015

I also think for the newcomers it's nice when gradle reportScoverage ends with nice message saying that coverage report was generated here and there. sbt plugin already does this and in scoverage#84 whole method was copied from sbt plugin to here, but without those nice messages.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

I think it's good that we've exchanged println for the reporter when compiling, and I think from a Gradle perspective I can live with println when reporting.

@Kwestor - I think that Gradle will allow us to show / hide those printlns as required here. It would become messy if we then have many different things being written to stdout, and only want to see some of them. Do you only build one sub-module at a time? If you have 70 sub-modules all being covered then would you prefer to aggregate them into a single report?

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

I think output should be configurable - defaulting to on.
I think the easiest thing to do is add the trait I've been on about, and
each of the build tool maintainers (me, Maiflai and Greg) can pass in an
anon instance. It's simple, and avoids repeating ourselves.

Any objections? If not, Jerzy do you want to update your PR to do this, or
if you'd rather I do then close your PR and I'll add it (tomorrow, I'm off
for night now).

On 1 February 2015 at 22:20, maiflai notifications@github.com wrote:

I think it's good that we've exchanged println for the reporter when
compiling, and I think from a Gradle perspective I can live with println
when reporting.

@Kwestor https://github.com/Kwestor - I think that Gradle will allow us
to show / hide those printlns as required here. It would become messy if we
then have many different things being written to stdout, and only want to
see some of them. Do you only build one sub-module at a time? If you have
70 sub-modules all being covered then would you prefer to aggregate them
into a single report?


Reply to this email directly or view it on GitHub
#1 (comment)
.

@Kwestor
Copy link
Author

Kwestor commented Feb 1, 2015

@maiflai right now yes, because we have some problems with macros and other compiler plugin getting into the way of scoverage, so I'm working on those one by one.

I would be really happy to have aggregate, but I'll look into this one only after everything else works.

@sksamuel if you have time, please do update it, I'll focus my work on searching for compiler problems with our other compiler plugin. But I think if you want to use this from command line you would have to have default implementation of this trait provided.

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

Default impl can use println like we have now.

On 1 February 2015 at 22:29, Jerzy Müller notifications@github.com wrote:

@maiflai https://github.com/maiflai right now yes, because we have some
problems with macros and other compiler plugin getting into the way of
scoverage, so I'm working on those one by one.

I would be really happy to have aggregate, but I'll look into this one
only after everything else works.

@sksamuel https://github.com/sksamuel if you have time, please do
update it, I'll focus my work on searching for compiler problems with our
other compiler plugin. But I think if you want to use this from command
line you would have to have default implementation of this trait provided.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

@sksamuel - sorry, ideally I don't want to compile against the plugin at all.

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

Aren't you doing that now anyway?
On 1 Feb 2015 22:33, "maiflai" notifications@github.com wrote:

@sksamuel https://github.com/sksamuel - sorry, ideally I don't want to
compile against the plugin at all.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

Trying not to :-)

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

Is it an issue because people have the dep anyway
On 1 Feb 2015 22:40, "maiflai" notifications@github.com wrote:

Tryng not to :-)


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

It's more a case of trying to avoid loading a scala version into the build classpath.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

Hence I currently fork a new java process, and then I have to provide my reporting classes and the plugin jar.

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

Why don't you want scala in there? It's a scala code coverage tool :)
On 1 Feb 2015 22:45, "maiflai" notifications@github.com wrote:

Hence I currently fork a new java process, and then I have to provide my
reporting classes and the plugin jar.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

:-) I don't want to build cross-version. Don't get me wrong, I like Scala, but compatibility is painful.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

Ideally I'd release this gradle wrapping plugin once, and it will work without me needing to touch it again, regardless of scala 2.12, 2.13 etc.

It should also allow changes upstream to flow without me needing to do a corresponding release.

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

Ok I see. I guess you don't need to for reports. For example sbt only uses
2.10.
On 1 Feb 2015 22:50, "maiflai" notifications@github.com wrote:

:-) I don't want to build cross-version. Don't get me wrong, I like Scala,
but compatibility is painful.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

Yes, I had fun learning that. It hasn't always used 2.10.

When they release a new version of SBT I think every plugin maintainer has to release a new version, and there's always one plugin you need that hasn't been refreshed.

@sksamuel
Copy link

sksamuel commented Feb 1, 2015

That's just because they fuck up the sbt api not because of scala. We just
dumped sbt at work.
On 1 Feb 2015 22:58, "maiflai" notifications@github.com wrote:

Yes, I had fun learning that. It hasn't always used 2.10.

When they release a new version of SBT I think every plugin maintainer has
to release a new version, and there's always one plugin you need that
hasn't been refreshed.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 1, 2015

:-) what have you picked instead?

@Kwestor
Copy link
Author

Kwestor commented Feb 1, 2015

Btw - why dump sbt now? I thought the are making it better and more stable with each release. And next one should be 1.0, with stable API and bin compat.

@sksamuel
Copy link

sksamuel commented Feb 2, 2015

Because it's just not very good is it. And I say that as a bit of an SBT
fan really. But I'm implicitly responsible for the build (as I'm ostensibly
the scala 'expert') and I was sick of supporting the rest of the team
(through no fault of their own). We switched to gradle and despite never
having used it before, within 4 hours, the entire build was converted, not
a single issue, and all the front end developers stopped complaining. It
just "does what you want". Literally within a week I'm a total gradle
fanboi now.

To be fair to SBT it's always been fine for me in my OSS projects (although
writing plugins is a joke due to zero documentation, and weird
operators/symbols that no one understands). And I do work for a bank so
infrastructure is more locked down than you'd see anywhere else. But I
don't understand why typesafe have people on SBT when they could have said,
"here's a gradle plugin, and we're off to build more Scala stuff. Cheers."

On 1 February 2015 at 23:12, Jerzy Müller notifications@github.com wrote:

Btw - why dump sbt now? I thought the are making it better and more
stable with each release. And next one should be 1.0, with stable API and
bin compat.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@sksamuel
Copy link

sksamuel commented Feb 2, 2015

And that's no criticism of the guys who work on SBT (maybe the people who
designed it), as I know Josh and Eugene are two of the hardest working and
best developers in Scalaland, but after using Gradle, I do think the whole
effort might have been a bit of a waste of time, and has probably
contributed to some people not using Scala. I'm also evaluating Kotlin for
projects at work and they've just embraced Gradle and Maven.

On 2 February 2015 at 08:41, Stephen Samuel (Sam) sam@sksamuel.com wrote:

Because it's just not very good is it. And I say that as a bit of an SBT
fan really. But I'm implicitly responsible for the build (as I'm ostensibly
the scala 'expert') and I was sick of supporting the rest of the team
(through no fault of their own). We switched to gradle and despite never
having used it before, within 4 hours, the entire build was converted, not
a single issue, and all the front end developers stopped complaining. It
just "does what you want". Literally within a week I'm a total gradle
fanboi now.

To be fair to SBT it's always been fine for me in my OSS projects
(although writing plugins is a joke due to zero documentation, and weird
operators/symbols that no one understands). And I do work for a bank so
infrastructure is more locked down than you'd see anywhere else. But I
don't understand why typesafe have people on SBT when they could have said,
"here's a gradle plugin, and we're off to build more Scala stuff. Cheers."

On 1 February 2015 at 23:12, Jerzy Müller notifications@github.com
wrote:

Btw - why dump sbt now? I thought the are making it better and more
stable with each release. And next one should be 1.0, with stable API and
bin compat.


Reply to this email directly or view it on GitHub
#1 (comment)
.

@maiflai
Copy link
Owner

maiflai commented Feb 2, 2015

Yes, that echoes my experience too.

I found a simple build was trivial with SBT, but it swiftly became confusing with lovely palindromic lines like this:

settings(addArtifact(artifact in (Common, packageBin), packageBin in Common): _*) // Publish the common artifact

Gradle didn't win me over at first, but it seems to scale from that first trivial build right up to a complex multi-language multi-artifact deployment tool. For the most part I found it straightforward and discoverable, the documentation is generally useful and the API changes slowly and predictably.

@sksamuel
Copy link

sksamuel commented Feb 2, 2015

The documentation on gradle is mad compared to SBT. SBT is like a text
readme in comparison.

And that SBT snippet? That's sensible. I understand stuff like that. How
about this:

calacOptions in Scct <++= (name in Scct, baseDirectory in Scct, update) map
{ (n, b, report) =>

I wish the Scala guys had concentrated on what's important. A build tool is
never going to make someone switch to Scala. It's money wasted on nothing.

On 2 February 2015 at 20:34, maiflai notifications@github.com wrote:

Yes, that echoes my experience too.

I found a simple build was trivial with SBT, but it swiftly became
confusing with lovely palindromic lines like this:

settings(addArtifact(artifact in (Common, packageBin), packageBin in Common): _*) // Publish the common artifact

Gradle didn't win me over at first, but it seems to scale from that first
trivial build right up to a complex multi-language multi-artifact
deployment tool. For the most part I found it straightforward and
discoverable, the documentation is generally useful and the API changes
slowly and predictably.


Reply to this email directly or view it on GitHub
#1 (comment)
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants