making scoverage work with classpaths #267

sammy-1234 · 2019-07-02T17:15:34Z

I have created a pull request for this issue. The changes I have made are recommendations given by @stuhood in the comment on the issue.

in Plugin.scala, added an additional option writeToClasspath that, if set to true, stores the instruments to the classpath and invokes Invoker.invokedWriteToClasspath in Invoker.scala at runtime.

In Invoker.scala, creating a new directory from the system option scoverage_measurement_path and copying the instruments there along with storing the measurements. If the system var is not set, it creates a subdirectory in the cwd.

If the option is not set (or is set to False), scoverage works the usual way.

Added a test for invokedUseEnvironment: sbt "testOnly scoverage.InvokerUseEnvironmentTest"

nsaechao · 2019-07-02T19:05:52Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

@@ -35,16 +34,23 @@ class ScoveragePlugin(val global: Global) extends Plugin {
        options.dataDir = opt.substring("dataDir:".length)
      } else if (opt.startsWith("extraAfterPhase:") || opt.startsWith("extraBeforePhase:")) {
        // skip here, these flags are processed elsewhere
-      } else {
+      } else if (opt.startsWith("useEnvironment:")) {


userEnvironment should be renamed to something more meaningful related to classpath --use-classpath-dir.

This option should also be clear that it'll override dataDir.

nsaechao · 2019-07-02T19:07:26Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

@@ -35,16 +34,23 @@ class ScoveragePlugin(val global: Global) extends Plugin {
        options.dataDir = opt.substring("dataDir:".length)
      } else if (opt.startsWith("extraAfterPhase:") || opt.startsWith("extraBeforePhase:")) {
        // skip here, these flags are processed elsewhere
-      } else {
+      } else if (opt.startsWith("useEnvironment:")) {
+        options.useEnvironment = opt.substring("useEnvironment:".length).toBoolean


Can we have some error checking here? .toBoolean could potentially throw an error here.

nsaechao · 2019-07-02T19:11:00Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

+
+    if (opts.exists{p: String => p.contains("useEnvironment") && p.contains("false")} || (!opts.exists(_.startsWith("useEnvironment")))) {
+      if (!opts.exists(_.startsWith("dataDir:")))
+        throw new RuntimeException("Cannot invoke plugin without specifying <dataDir>")


Or without specifying new option?

that is being checked by !opts.exists(_.startsWith("useEnvironment"))

nsaechao · 2019-07-02T19:11:28Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

    instrumentationComponent.setOptions(options)
  }

  override val optionsHelp: Option[String] = Some(Seq(
+    "-P:scoverage:useEnvironment:<boolean>                 if true, use the environment variable to store measurements" +


Can we include some description for overrides dataDir?

nsaechao · 2019-07-02T19:12:38Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

-    if (!opts.exists(_.startsWith("dataDir:")))
-      throw new RuntimeException("Cannot invoke plugin without specifying <dataDir>")
+
+    if (opts.exists{p: String => p.contains("useEnvironment") && p.contains("false")} || (!opts.exists(_.startsWith("useEnvironment")))) {


Is this meant to replace dataDir in the future?

No, this is just a check that if the option to write to the classpath is either set to false p.contains("useEnvironment") && p.contains("false") or is not present (!opts.exists(_.startsWith("useEnvironment")), then make sure that dataDir attribute is given by the user.

Overriding dataDir takes place in def setOptions(options: ScoverageOptions): Unit

nsaechao · 2019-07-02T19:26:16Z

scalac-scoverage-runtime/shared/src/main/scala/scoverage/Invoker.scala

+      // When code is changed in source file, old measurements present
+      // inside the dataDir can skew the results. Thus, clean the dataDir to remove
+      // old measurements.
+      resetDataDir(dataDir)


Is this an atomic operation?

Good call! I'll move this part inside the synchronized block to make it thread safe.

nsaechao · 2019-07-02T19:26:54Z

scalac-scoverage-runtime/shared/src/main/scala/scoverage/Invoker.scala

+
+  def resetDataDir(dataDir: String): Unit = {
+    // Making the directory if it does not exist
+    new File(dataDir).mkdirs()


Is this a safe operation?

It returns true if the directories are made and false otherwise (in case the directories are already present). It raises an exception only if a SecurityManager exists and denies to create the named dir. https://www.tutorialspoint.com/java/io/file_mkdirs.htm
I think it should be safe?

scalac-scoverage-runtime/shared/src/main/scala/scoverage/Invoker.scala

…ng classpathSubdir

stuhood

Thanks!

stuhood · 2019-07-12T16:29:53Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

    instrumentationComponent.setOptions(options)
  }

  override val optionsHelp: Option[String] = Some(Seq(
+    "-P:scoverage:writeToClasspath:<boolean>               if true, use the environment variable to store measurements" +


The way this string is broken means that it won't have spaces in it: need to leave the spaces you want in as trailing, like:

measurements " +

stuhood · 2019-07-12T16:30:37Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

+    "-P:scoverage:writeToClasspath:<boolean>               if true, use the environment variable to store measurements" +
+                                                           "and store instruments to the output classpath where compiler emits classfiles." +
+                                                           "Overrides the datadir with the output classpath.",
+    "-P:scoverage:classpathSubdir:<pathtosubdir>           subdir to attach to classpath. Default: META-INF/scoverage",


This should reference the other option probably: "subdir to prefix to classpath when writeToClasspath is in use"

stuhood · 2019-07-12T16:34:21Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

+       * [invokedUseEnvironment] as it helps differentiating the source
+       * files at runtime, i.e helps in creating a unique subdir for each source file.
+       */
+      if (options.writeToClasspath){


The only difference between these two blocks is the newTermName value. You should probably extract that into a variable to avoid duplicating the blocks.

stuhood · 2019-07-12T16:39:57Z

scalac-scoverage-runtime/jvm/src/test/scala/scoverage/InvokerUseEnvironmentTest.scala

+  }
+
+  private def deleteMeasurementFolders(): Unit = {
+    val d = s"${System.getenv("SCOVERAGE_MEASUREMENT_PATH")}"


This test should not be expect this env var to already be set.

What I'd recommend instead is changing the code in the invoker to consume a Java "system property", rather than an env var, and to then set the property during the test. System properties can be changed at runtime (see "Writing system properties" here: https://docs.oracle.com/javase/tutorial/essential/environment/sysprop.html).

This would avoid setting it in the build.sbt file, which affects all tests: not just this one.

...but before doing this, see the comment below about how the dataDir is initialized. That change would also allow for setting the dataDir in tests.

stuhood · 2019-07-12T16:41:36Z

scalac-scoverage-runtime/shared/src/main/scala/scoverage/Invoker.scala

+  /**
+   * We record that the given id has been invoked by appending its id to the coverage
+   * data file.
+   *


This method (and method doc) duplicates a lot of the def invoke method. You should extract more of the method body in order to dedupe them.

stuhood · 2019-07-12T16:46:22Z

scalac-scoverage-runtime/shared/src/main/scala/scoverage/Invoker.scala

+   * @param instrumentsDir the directory where the instrument data is held and needs to be copied from
+   */
+  def invokedWriteToClasspath(id: Int, instrumentsDir: String): Unit = {
+    // Get the output data directory from the environment variable or write to a subdir.


Computing the dataDir like this is likely to be expensive. You should probably move this out into a @volatile var, and compute it once at startup.

That would also allow/require you to mutate it in unit tests.

stuhood · 2019-07-12T16:47:53Z

scalac-scoverage-runtime/shared/src/main/scala/scoverage/Invoker.scala

+    // and it is possible that a test file tests multiple instrumented binaries. Thus, to
+    // segregate reports for each source file, we need this hash. This was not an issue with
+    // previous version as then we explicitly specified a different [dataDir] each time.
+    dataDir += s"/${safe_name(instrumentsDir)}"


Ditto doing this at startup time. You should probably add a setter for the datadir for tests to use that will do the appropriate concatenation.

This invoke method needs to be very fast since it will be called millions of times per run. Doing too much setup here will definitely slow down an application.

I think this concatenation needs to be at each invocation as it is a parameter to the invokedWriteToClasspath method

stuhood · 2019-07-12T16:49:26Z

scalac-scoverage-runtime/shared/src/main/scala/scoverage/Invoker.scala

+          // When code is changed in source file, old measurements present
+          // inside the dataDir can skew the results. Thus, clean the dataDir to remove
+          // old measurements.
+          resetDataDir(dataDir)


Deleting this directory feels like it attempts to solve a problem that is unrelated to this PR. In particular: a caller should always set a new dataDir if they don't want to merge over top of the old one. Would drop the removal.

stuhood · 2019-07-12T16:55:52Z

scalac-scoverage-runtime/shared/src/main/scala/scoverage/Invoker.scala

+  }
+
+  def safe_name(str: String): String =  {
+    val pattern = "[^/]+".r


This method should have a camel-case name.

But also: you should watch out for turning filepaths into single names, there are filename length limits, and take a bunch of filenames and merging them into one is very likely to violate that.

Instead, you should consider preserving the full name of the file under the directory, and call mkdirs for the parents of each file.

The method does not exists in the latest PR.

…m cp

stuhood

Looks good! But how targetids are chosen might still need some work.

stuhood · 2019-07-15T22:17:39Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

-    if (!opts.exists(_.startsWith("dataDir:")))
-      throw new RuntimeException("Cannot invoke plugin without specifying <dataDir>")
+
+    if (opts.exists{p: String => p.contains("writeToClasspath") && p.contains("false")} || (!opts.exists(_.startsWith("writeToClasspath")))) {


It would be good to extract this entire condition and give it a name: for example: ```
val isUsingWriteToClasspath: Boolean = ???
if (!isUsingWriteToClasspath) { ??? }

with the latest change (below), we always need the dataDir to be set.

stuhood · 2019-07-15T22:25:04Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

+
+    // Overriding the [dataDir] if [writeToClasspath] is true.
+    // If writeToClasspath is true, we set the output directory to output classpath,
+    // i.e to the directory where compiler is going to output the classfiles.


This may not be unique, unfortunately. Even if you include the absolute path to the output directory, two compiles happening on different machines might use the same output directory, and not be identical.

I don't have any great ideas here. Hashing the source files themselves relative to the current working directory might work? Using a random number or UUID would make builds non-deterministic.

Maybe this information does need to be user provided, and you could require that the dataDir is a relative path that you'd use in this position?

Agreed. I have currently set the dataDir to be the targetID in case the writeToClasspath option is set to true.

Should update this comment, and below.

stuhood

Thanks!

stuhood · 2019-07-16T02:41:39Z

scalac-scoverage-runtime/shared/src/main/scala/scoverage/Invoker.scala

+    // which needs to be stored SEPARATELY from each other (but along with THEIR instrument files).
+    // Thus, to segregate (instruments + measurements) for each target, we need this subdir.
+    // This was not an issue with previous version as then we explicitly
+    // specified a different [dataDir] each time we compiled a target.


Is this back to being the case? The dataDir will still be required?

If by dataDir you mean the variable on line 96, then yes, you are correct - dataDir is no longer necessary.

That being said, I still think its a good idea to provide an option for setting the directory prefix at runtime (using the System var in this case) - would help in setting it to runtime classpath. What do you think?

I understand your concern regarding doing additional setup here (its effect in terms of performance)

stuhood · 2019-07-16T02:42:34Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

+
+    // Overriding the [dataDir] if [writeToClasspath] is true.
+    // If writeToClasspath is true, we set the output directory to output classpath,
+    // i.e to the directory where compiler is going to output the classfiles.


Should update this comment, and below.

stuhood · 2019-07-16T02:42:36Z

scalac-scoverage-plugin/src/main/scala/scoverage/plugin.scala

+    if(options.writeToClasspath){
+      settings.outputDirs.getSingleOutput match {
+        case Some(dest) =>
+          //creating targetId out of [dataDir]


Should validate that this is a relative path.

…iteToClaspath

gslowikowski · 2019-08-03T22:18:02Z

Hi guys. Thank you for your effort. I was very busy recently. Analyzing and testing your work will require considerable amount of time. I promise to do it soon.

stuhood · 2019-09-17T03:32:36Z

Thank you @gslowikowski!

callicles · 2020-11-24T13:51:35Z

Is this still needed?

making scoverage work with classpaths

8408485

nsaechao reviewed Jul 2, 2019

View reviewed changes

Sameer Arora added 2 commits July 2, 2019 17:06

changing option name from useEnvironment to writeToClasspath and addi…

dc962d1

…ng classpathSubdir

changing measurement subdir to filepath string

4e7547e

sammy-1234 mentioned this pull request Jul 8, 2019

Scoverage in Pants OSS Proposal pantsbuild/pants#8026

Closed

few changes to comments

edf30b3

sammy-1234 mentioned this pull request Jul 12, 2019

Getting scoverage to work with classpaths. twitter-forks/scalac-scoverage-plugin#1

Merged

stuhood suggested changes Jul 12, 2019

View reviewed changes

Sameer Arora and others added 2 commits July 14, 2019 18:02

adding a targetId to help grab correct instrument file at runtime fro…

626e99e

…m cp

Update Constants.scala

ade3d0f

stuhood reviewed Jul 15, 2019

View reviewed changes

setting dataDir to be targetId in case writeToClasspath is true

7b634b4

stuhood approved these changes Jul 16, 2019

View reviewed changes

cleaning comments and checking relative path of dataDir in case of wr…

bfab282

…iteToClaspath

ckipp01 deleted the branch scoverage:main May 29, 2022 08:38

ckipp01 closed this May 29, 2022

making scoverage work with classpaths #267

making scoverage work with classpaths #267

Conversation

sammy-1234 commented Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sammy-1234 Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sammy-1234 Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sammy-1234 Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

sammy-1234 Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gslowikowski commented Aug 3, 2019

stuhood commented Sep 17, 2019

callicles commented Nov 24, 2020

sammy-1234 commented Jul 2, 2019 •

edited

Loading

sammy-1234 Jul 2, 2019 •

edited

Loading

sammy-1234 Jul 2, 2019 •

edited

Loading

sammy-1234 Jul 16, 2019 •

edited

Loading

sammy-1234 Jul 16, 2019 •

edited

Loading