Resolve canonical path for src in BaseReportWriter.relativeSource

[lloydmeta]

We need the canonical path for the given src because our formattedSourcePaths are canonical.

The sources we iniitally store are non-canonical, provided directly through SBT's settings keys, which could refer to things like simlinks and '.' or '..' characters. This causes problems when trying to resolve sources because we try to match against canonical formattedSourcePaths.

The use case is when you have unmanagedSourceDirectories in projects that have non-standard base directories, such as in @lihaoyi's sourcecode. Before this fix, trying to run coverage reports would cause java.lang.RuntimeException: No source root found errors.

Misc other changes:

• Bump SBT version
• Bump PGP plugin version

[gslowikowski]

Related to #150

I analyzed this issue. I personally would prefer removing getCanonicalPath from formattedSourcePaths, read my comment. Unfortunately nobody was interested in this issue since then.

It would be more efficient, it would work with SBT, but unfortunately it wouldn't in some configurations in Maven plugin with Maven Scala Plugin, because Maven Scala Plugin by default canonizes all source paths.
Canonical paths are safer (they should always work ), but require more processing. Report generation, may slow down visibly in large projects IMO.

[lloydmeta]

Ah, I didn't know this was already pointed out and discussed.

If I had to choose between "always works" and "theoretically faster by unknown amounts but does not always work", I would always prefer the former, especially because we are trying to fix something that is not always working already. The current fix seems simple enough and I've tested it locally to make sure it works. Still, as long as the problem is fixed (preferably soon), I don't mind how :)

If you want to explore other ways of fixing this, I'll close this PR.

[gslowikowski]

Don't close. I've already explored different solutions (among others version just like your PR). I'm not the owner of this repo, I don't decide what to merge and why.

[lloydmeta]

Ah ok, I thought you were a maintainer :) In that case it might be worthwhile for you to throw up your different versions of the fix so whoever is the maintainer can pick from a few options?

[sksamuel]

I think we should stick with canonical paths moving forward, why don't we change the input paths to be canonical in the first place, rather than change in the report writer?

[gslowikowski]

1. SBT internally, for compilation, does not canonize paths because it's just not needed.
2. Scoverage does not need canonized paths for checks, aggregates, etc. only for reports.
3. Personally I don't like canonical paths because:
   a) File.getCanonicalPath() throws IOException which must be handles somehow in Java. I don't know, how Scala handles it, but in Scala you can treat them as runtime exceptions
   b) If the path is virtual (contains links), this is done for a reason. Why should I, as a user, see the real path?

[sksamuel]

Those are fair points. So you advocate just changing the writers.

On 14 April 2016 at 15:22, Grzegorz Slowikowski [email protected]
wrote:

1. SBT internally, for compilation, does not canonize paths because
   it's just not needed.
   2.

   Scoverage does not need canonized paths for checks, aggregates, etc.
   only for reports.
   3.

   Personally I don't like canonical paths because:
   a) File.getCanonicalPath() throws IOException which must be handles
   somehow in Java. I don't know, how Scala handles it, but in Scala you can
   treat them as runtime exceptions
   b) If the path is virtual (contains links), this is done for a reason.
   Why should I, as a user, see the real path?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#159 (comment)

[gslowikowski]

I would throw away getCanonicalPath for source here. I tried this. It works with SBT, and partially with Maven.
In Maven "partially", because there are two Scala compiler plugins:

• SBT Compiler Maven Plugin - no problems
• Scala Maven Plugin - by default canonizes all source file paths (this is parametrizable, but turned on by default), this is very bad design decision IMO, but what can I do. If source files have canonized paths, source roots have to have canonized paths too. Although, one user of Scoverage + Scala Maven Plugin reported (on Gitter) this issue in his project when building in Jenkins. IMO current Scoverage implementation it should work for him. This is strange.

[lloydmeta]

@sksamuel I would advocate just changing BaseReportWriter#relativeSource to resolve canonical paths because:

1. Lowest risk of regressions (vs changing it everywhere) as it minimises impact.
2. Works for all foreseeable cases; even now, with minimal-ish investigation we already know of one case where not using canonical paths and using just absolute paths won't work. I don't think anyone here has the time or resources to go through even 10% of the plugins out there to come up with a more accurate number though.

I'm not too worried about the fact that getCanonicalPath might throw exceptions because if a file's canonical path can't be resolved, you'll probably have problems accessing it later...

Some related findings:

• This commit seems to be the point at which this project switched to using canonical paths for everything
• This comment says that Coveralls didn't work well with relative paths, which is another precedent of relative paths being somewhat troublesome.

[gslowikowski]

My comments to your findings:

• canonical paths were introduced by this commit on 13 Nov 2014
• I prefer absolute paths, not relative

[lloydmeta]

I prefer absolute paths, not relative

Ah, when you said you would throw away getCanonicalPath, you meant to replace it with getAbsolutePath instead of just throwing it away. So long as we're dealing with paths as simple Strings, this is going to be somewhat confusing...

One way of looking at it is by definition, a canonical path is an absolute path. Therefore, if some (possibly downstream) logic expects an absolute path and gets a canonical path, it will work. However, logic that expects canonical paths but gets absolute paths may not work. This means canonical paths result in less potential problems (aka whack-a-mole).

[sksamuel]

We're all in agreement we shouldn't be using relative paths. So now its
just a matter of canonical vs absolute. There might be issues with getting
a canonical path from an absolute one.

On 14 April 2016 at 16:58, Lloyd [email protected] wrote:

I prefer absolute paths, not relative

Ah, when you said you would throw away getCanonicalPath, you meant to
replace it with getAbsolutePath instead of just throwing it away. So long
as we're dealing with paths as simple Strings, this is going to be
somewhat confusing...

One way of looking at it is by definition, a canonical path is an
absolute path, just by definition. Therefore, if some (possibly downstream)
logic expects an absolute path and gets a canonical path, it will work.
However, for logic that expects canonical paths but gets absolute paths, it
may not work. This means canonical paths result in less potential problems
(aka whack-a-mole).

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#159 (comment)

[lloydmeta]

For what it's worth I found a workaround for my use case; add a .map(_.getCanonicalFile) to the list of added directories:

 unmanagedSourceDirectories in Compile ++= {
    CrossVersion.partialVersion(scalaVersion.value) match {
      case Some((2, scalaMajor)) if scalaMajor >= 11 => Seq(baseDirectory.value / ".." / "compat" / "src" / "main" / "scala-2.11").map(_.getCanonicalFile)
      case _ => Nil
    }
}

Substituting in getAbsoluteFile in the above case does not work for obvious reasons. Furthermore, if this plugin switches to using absolute paths for source directories in the writer, anyone using a similar workaround will get their CI broken when upgrading.

[gslowikowski]

I would like to resolve this before 1.2.0.

I found interesting change from absolute to relative paths in SBT project (sbt/io#18), asked questions about it, but unfortunately with no response.

My tests show that paths are not canonized even with this PR applied.

Today I've asked the same questions on SBT mailing list. Waiting for response.

[lloydmeta]

I wrote that PR for SBT IO, so I'll explain why I think it was necessary.

Simply put, operations like relativize or parent on File or Path objects cannot be cleanly and intuitively done using the String representation of those objects if the String representations are not canonical, because there are different ways to refer to the same physical resource. If we use non-canonical paths, we get difficult-to-debug errors at run time when we use things like symlinks or relative paths to refer to the same resource, which I think is undesirable behaviour. On the other hand, if getCanonicalPath throws, at least we have a well known error with an informative error message (and likely is an early warning that there will be problems accessing that resource anyway).

SBT IO's relativize function is used in some places in SBT, and here in SBT-Coveralls. I'm also using it in my personal projects and plugins.

Asides from desiring proper behaviour, I still think that in the interest of not introducing regressions and breaking existing usages of this plugin, resolving canonical path in BaseReportWriter.relativeSource is the safer choice.

[ghost]

Just to add some extra history to this.

• The issue of type "source not in root directory" goes back for some time, especially from IntelliJ that could not handle this. Now it does.
• ScalaJS CrossProject incompatible issue sbt-scoverage#111 was fixed in the scala-js sbt plugin from Source root not found for shared project error scala-js/scala-js#1748 in scala-js/scala-js@09b2291. Not by using canonical directly but via getParentFile
• the example in sourcecode is for its build only, but indeed even that is redundant as sbt now does that automatically

[rorygraves]

What are we proposing, closing without merging?

[ghost]

Well canonical paths look OK to me - perhaps I could/should do a local build and test this out with some real projects?

[lloydmeta]

@inthenow thanks for chiming in :)

the example in sourcecode is for its build only, but indeed even that is redundant as sbt now does that automatically

That's cool, I didn't know about this. It looks like it has been in SBT for some time. From what I can see, it requires you to have a different directory structure, so instead of having code in compat/src/main/scala-2.11, you would have it in something like src-2.11/main?

[gslowikowski]

Give me some time to analyze this once again, please.

[gslowikowski]

OK, I think, we should merge it.

It's definitely safe. getCanonicalPath adds some overhead, bat safety is more important. I've already tested this, so I can confirm that it works.

I have two side questions:

• @lloydmeta, you showed that SBT IO's relativize function is used in many places. OK, but can you confirm my findings, that your PR has nothing to do with source roots and source file paths in SBT compilation process?
• how Scala handles java.net.IOException thrown by getCanonicalPath? It doesn't need to be declared in Scala (it's checked exception in Java).

[lloydmeta]

@gslowikowski

OK, but can you confirm my findings, that your PR has nothing to do with source roots and source file paths in SBT compilation process?

My PR was about fixing the encapsulated behaviour of that method with respect to SBT plugins only, so you should rely on your own findings and perhaps feedback from the SBT team to see if that method is used in the compilation process. My guess is though, with the upcoming SBT client/server split, things like finding resources are likely to be in flux.

how Scala handles java.net.IOException thrown by getCanonicalPath? It doesn't need to be declared in Scala (it's checked exception in Java).

In Scala, all exceptions are unchecked and so are left for the caller to handle or else bubble up (or down, depending on your perspective) the call stack.

[gslowikowski]

Thanks.

[gslowikowski]

If nobody objects, I will merge this PR.

[rorygraves]

Go for it

[gslowikowski]

I wanted to publish new 1.2.0-SNAPSHOT, but cannot compile sbt-scoverage with this version (see my comment #167 (comment)).