Allow sbt-scoverage to work on windows

[stevedlawrence]

The -Xplugin option expects a classpath string with multiple paths
separated by a semicolon or colon depending on the operating system.
However, this currently always uses a colon, which doesn't work on
Windows. This means scalac cannot find the scoverage plugin jars and
leads to errors about "bad options".

This modifies the -Xplugin logic to use File.pathSeparator when building
the Xplugin classpath, allowing it to work regardless of operating
system.

Also modifies excludedPackages when on Windows to replace Unix path
separators with Windows path separators (which are escaped because it is
treated as a regular expression). Modifies tests to remove unnecessary
escaping of Unix separators that results in an invalid regex due to this
replacement on Windows.

Note that two tests currently prevent CI from working on Windows:
coverage-off and preserve-set. The reason for these failures on Windows
is not clear, but these changes at least allow the common uses to work
on Windows.

[stevedlawrence]

Seems like the CI fails on windows, but is unrelated to this change since the coverage is actually running. Should the CI change be reverted?

[ckipp01]

Thanks a lot for the pr @stevedlawrence. This looks to make sense, but I'm not fully sure what's going on with CI. I for sure don't want to turn it off though as if we're adding support for windows I want to ensure that it's working as intended with tests. Are you developing locally on windows? If so, are all the scripted tests passing?

[stevedlawrence]

I don't really have a good windows dev environment so testing is very difficult/slow. I tried to run the scripted tests in a VM and it looks like I get the similar failures as the GitHub actions, though it never finishes I think because of my environment.

Looks like there might be an unrelated issue with the coverageExcludedPackages/Files options. I'm not sure what it is, but the regexes in the tests have escaped slashes--probably needs some other kind of escaping for linux vs windows?

[stevedlawrence]

I've pushed an update that fixes one of the failing tests.

However, two test still fail:

• scoverage/coverage-off: the issue with this test is that something is holding open a handle to the target/scala-2.13/scoverage-data/scoverage.measurements.XXXX file. Because the handle is open, the second clean in the test can't remove this file and so the test thinks that coverageOff didn't correctly turn off the coverage because that file exists. I'd guess a file.close() is just missing somewhere, but I haven't been able to figure out where. I don't think the scripted test is even causing that file to be read--maybe this is an issue missing in the scalac plugin?
• scoverage/preserve-set: this seems to be an intermittent failure. I haven't been able to figure out the cause

Any thoughts on these remaining issues? Any chance we could disable the windows github action until these final issues are resolved? Not being able to upgrade this plugin is preventing us from upgrading other plugins due to conflicting transitive dependencies.

[stevedlawrence]

@ckipp01, could you approve the workflow to confirm this fixes additional issues?

[ckipp01]

scoverage/coverage-off: the issue with this test is that something is holding open a handle to the target/scala-2.13/scoverage-data/scoverage.measurements.XXXX file. Because the handle is open, the second clean in the test can't remove this file and so the test thinks that coverageOff didn't correctly turn off the coverage because that file exists. I'd guess a file.close() is just missing somewhere, but I haven't been able to figure out where. I don't think the scripted test is even causing that file to be read--maybe this is an issue missing in the scalac plugin?

That could be the case although we do test for windows on the actual scalac plugin. So I'm not really sure.

scoverage/preserve-set: this seems to be an intermittent failure. I haven't been able to figure out the cause

🤔 I feel like I've seen this before, but only on windows. So no idea.

Any thoughts on these remaining issues? Any chance we could disable the windows github action until these final issues are resolved? Not being able to upgrade this plugin is preventing us from upgrading other plugins due to conflicting transitive dependencies.

I'd honestly really love to just get windows nailed down, but I understanding that this currently isn't working at all on windows. I've got one question I left down there, but after that run formatting again and I think we can just merge as is. If this unlocks people this is better than nothing. Then just turn off the windows CI. However, remove the #fixes from the pr so it doesn't close the issue.

[ckipp01]

How come this changes?

[stevedlawrence]

These escapes aren't needed on linux, and cause issues on windows.

Forward slashes aren't a special character in regular expressions so they don't need to be escaped. The only reason the test works now is because this string becomes the regex .*\/two\/GoodCoverage, and while you don't need to escape forward slashes, an escaped forward slash is just a forward slash and works as expected.

However, on windows our excludedFiles regex needs to look for a backslash instead of a forward slash due to windows using backslashes for paths. However, unlike forward slashes, backslashes do need to be escaped in regular expressions. So this commit adds a change to replace / with \\ (i.e. replace a single forward slash with two literal backslashes).

If we do this replacement, with the original regex in the test, the regex on windows becomes .*\\\two\\\GoodCoverage with three literal backslashes. This means the regex contains an escaped backslash followed by an escaped t, and another escaped backslash followed by an escaped G. The escaped t and G break the regex.

With these changes in the test and the new string replace stuff, on linux the regex is .*/two/GoodCoverage, and on windows the regex is .*\\two\\GoodCoverage.

[ckipp01]

Gotcha thanks for the explanation.

[stevedlawrence]

That could be the case although we do test for windows on the actual scalac plugin. So I'm not really sure.

I did notice in the scalac plugin in Invoker.scala that it has a threadFiles variable that holds a bunch of FileWriters. I wonder if these are holding on to handles? Or maybe scalac plugin tests don't test deleting files? Or if it does so, it does it in a way that ensures all handles are closed and sbt just does things differently? Just guesses, I don't really have any idea.

I'd honestly really love to just get windows nailed down, but I understanding that this currently isn't working at all on windows

Agreed. I think I just have hit my knowledge limit when it comes to Windows, and I don't have a good dev set up to properly debug these issues.

[stevedlawrence]

Updates made based on comments

[stevedlawrence]

These escapes aren't needed on linux, and cause issues on windows.

Forward slashes aren't a special character in regular expressions so they don't need to be escaped. The only reason the test works now is because this string becomes the regex .*\/two\/GoodCoverage, and while you don't need to escape forward slashes, an escaped forward slash is just a forward slash and works as expected.

However, on windows our excludedFiles regex needs to look for a backslash instead of a forward slash due to windows using backslashes for paths. However, unlike forward slashes, backslashes do need to be escaped in regular expressions. So this commit adds a change to replace / with \\ (i.e. replace a single forward slash with two literal backslashes).

If we do this replacement, with the original regex in the test, the regex on windows becomes .*\\\two\\\GoodCoverage with three literal backslashes. This means the regex contains an escaped backslash followed by an escaped t, and another escaped backslash followed by an escaped G. The escaped t and G break the regex.

With these changes in the test and the new string replace stuff, on linux the regex is .*/two/GoodCoverage, and on windows the regex is .*\\two\\GoodCoverage.

[ckipp01]

Thanks for this @stevedlawrence. I'll merge this in and cut a new release shortly.