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

Add option to Ignore scalac options change #548

Merged
merged 9 commits into from Jun 7, 2018

Conversation

Projects
None yet
6 participants
@lukaszwawrzyk
Contributor

lukaszwawrzyk commented Jun 4, 2018

This PR adds externalCompileSetupEquiv field to IncOptions. Ths is a function that tells if two MiniSetup s are equivalent.
This allows to do an override of this as in IncHandler.scala:499 to e.g. ignore the change of scalac options for recompilation decision. Without that all the files would be recompiled, with the change, only those that actually changed.

I also resolved the problem with hardcoded implicits in MiniSetupUtil.scala. That was caused by a missing import.

The PR seems to also solve #406

@typesafe-tools

This comment has been minimized.

typesafe-tools commented Jun 4, 2018

A validation involving this pull request is in progress...

@typesafe-tools

This comment has been minimized.

typesafe-tools commented Jun 4, 2018

The validator has checked the following projects against Scala 2.12,
tested using dbuild, projects built on top of each other.

Project Reference Commit
sbt 1.x sbt/sbt@2848770
zinc pull/548/head adf63dc
io 1.x sbt/io@0e4ee5a
librarymanagement 1.x sbt/librarymanagement@6e4ad6e
util 1.x sbt/util@b412c9c
website 1.x

The result is: FAILED
(restart)

@romanowski

This comment has been minimized.

Contributor

romanowski commented Jun 4, 2018

This option is need for compiler plugins development. If your plugin accepts parameter every change causing everything to be recompiled. In many cases all you need it increase e.g. logging verbosity.

Moreover when debugging compiler (as a part of compiler plugin development) it is really important to have option to compile single file (regardless how much files should be recompiled). The only scenario that cannot be implemented with current hooks is compiler option change - and we aim to fix that in this PR.

@romanowski

This comment has been minimized.

Contributor

romanowski commented Jun 4, 2018

@jvican, @dwijnand @eed3si9n is PR verification known to be flaky? Failures in this build does not seem to be related to this PR.

@dwijnand

This comment has been minimized.

Member

dwijnand commented Jun 4, 2018

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGBUS (0x7) at pc=0x00007f605e9e2a32, pid=34, tid=0x00007f5cc0dad700
#
# JRE version: Java(TM) SE Runtime Environment (8.0_102-b14) (build 1.8.0_102-b14)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (25.102-b14 mixed mode linux-amd64 compressed oops)
# Problematic frame:
# C  [libzip.so+0x11a32]  newEntry+0x62
#
# Core dump written. Default location: /drone/src/github.com/sbt/zinc/core or core.34
#
# An error report file with more information is saved as:
# /drone/src/github.com/sbt/zinc/hs_err_pid34.log
Compiled method (nm)  615154  139     n 0       java.util.zip.ZipFile::getEntry (native)
 total in heap  [0x00007f6050379290,0x00007f6050379600] = 880
 relocation     [0x00007f60503793b8,0x00007f6050379400] = 72
 main code      [0x00007f6050379400,0x00007f6050379600] = 512
#
# If you would like to submit a bug report, please visit:
#   http://bugreport.java.com/bugreport/crash.jsp
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#
/usr/bin/sbt: line 34:    34 Aborted                 (core dumped) /usr/lib/bin/sbt "$@" -Dsbt.boot.properties=/sbt.boot

Wow

@jvican

This comment has been minimized.

Member

jvican commented Jun 5, 2018

I would propose another approach to this problem: instead of outsourcing the function that computes the equivalence between two mini setups, let's create a wishlist of flags that should not be detected to invalidate sources/products.

We can have a default whitelist with all the flags you deem appropriate, plus a configurable user-defined whitelist that can come from a build tool.

@romanowski

This comment has been minimized.

Contributor

romanowski commented Jun 5, 2018

We can have a default whitelist with all the flags you deem appropriate, plus a configurable user-defined whitelist that can come from a build tool.

This is not as powerful as proposed approach and can be implemented with it (it may be good idea to include such implementation in this PR). The situation I've got in mind (mentioned in original issue as well #408) is not recompiling everything when order of options change. It happens sometimes especially when incremental compilation artifacts (a'la Hoarder) are reused between IDE and build tool.

@jvican

This comment has been minimized.

Member

jvican commented Jun 5, 2018

This is not as powerful as proposed approach and can be implemented with it (it may be good idea to include such implementation in this PR). The situation I've got in mind (mentioned in original issue as well #408) is not recompiling everything when order of options change. It happens sometimes especially when incremental compilation artifacts (a'la Hoarder) are reused between IDE and build tool.

If this is the case, why don't we just re-implement the equivalence function to ignore order of compilation flags? We can have this + the whitelist. I'm aware it's not as powerful as the solution proposed here, but I think it's more elegant and helps all the clients of Zinc reuse all of this information (instead of requiring every client to repeat the same task over and over again).

@dragos

This comment has been minimized.

dragos commented Jun 5, 2018

We're using a similar technique in our plugin, and we needed to test for flags that start with a certain prefix, not just equality (think about adding or removing -Ylog:parser or other phase names). Just something to consider if you go for the whitelist approach.

@lukaszwawrzyk

This comment has been minimized.

Contributor

lukaszwawrzyk commented Jun 5, 2018

So how about keeping things as they are in the current PR but adding some means to implement more common use cases conveniently. For example to provide method that takes a function that compares two arrays of scalac opts, or takes a whitelist.
To show one example of how it could be used:

incOptions := incOptions.value.withExternalCompileSetupEquiv(ignoreScalacOptionsChange("-Xprint:typer", "-Xfatal-warnings")))

If that is feasible, what would be a good place to put such helper functions?

@jvican

This comment has been minimized.

Member

jvican commented Jun 5, 2018

So how about keeping things as they are in the current PR but adding some means to implement more common use cases conveniently. For example to provide method that takes a function that compares two arrays of scalac opts, or takes a whitelist.

What other cases do you need aside from detecting changed order in compile options and filtering out some flags from the equivalence to merit the inclusion of this hook?

The way I see it, we solve all the problems by manually ignoring order in the default equivalence function and introducing the concept of whitelist. The whitelist can support regex and we can provide a default for all our clients.

@dragos

This comment has been minimized.

dragos commented Jun 5, 2018

The way I see it, we solve all the problems by manually ignoring order in the default equivalence function and introducing the concept of whitelist.

It's a can of worms. Parameters are not atomic, think about -classpath foo:bar and the like. You will need to know exactly which ones take a second argument and keep them together. It will probably break once someone adds a new argument in Scala that takes a second "part" separated by space.

@jvican

This comment has been minimized.

Member

jvican commented Jun 5, 2018

The whitelist can be conservative. That’s fine, it’s something it will need to be done anyway if there’s a hook for it.

@lukaszwawrzyk

This comment has been minimized.

Contributor

lukaszwawrzyk commented Jun 7, 2018

I am a bit confused on what is the conclusion. Do we want to go with that hook equiv function or only with the whitelist?
As I already had it almost done, now I replaced hook with the whitelist/regex mechanism.
There is one non obvious thing I implemented. The taken assumption is that all options start with - and option can have 0 or 1 parameters that does not start with -. Base on that, if option with a parameter is passed, I combine those with a single space for the analysis. This means that one can e.g. specify to ignore -cores .* and change -cores 1 to -cores 2 without full recompilation. See the tests I added for more details. The order is also ignored. I guess this covers everything that was needed.

@romanowski

Looks good I've added few minor style comments and asked to improve tests a bit

def groupWithParams(opts: Array[String]): Set[String] = {
def isParam(s: String) = !s.startsWith("-")
def recur(opts: List[String], res: Set[String]): Set[String] = opts match {
case opt :: param :: rest if isParam(param) => recur(rest, res + (opt + " " + param))

This comment has been minimized.

@romanowski

romanowski Jun 7, 2018

Contributor

Maybe string interpolation? It will be more readable (especially that we are using + to add elements to Set

new Equiv[Array[String]] {
override def equiv(opts1: Array[String], opts2: Array[String]): Boolean = {
dropIgnored(groupWithParams(opts1)) == dropIgnored(groupWithParams(opts2))

This comment has been minimized.

@romanowski

romanowski Jun 7, 2018

Contributor

Empty linie

equiv.equiv(before, after) shouldBe false
}

This comment has been minimized.

@romanowski

romanowski Jun 7, 2018

Contributor

We are missing test case for option ordering

@@ -0,0 +1 @@
scalac.options = -Xfatal-warnings

This comment has been minimized.

@romanowski

romanowski Jun 7, 2018

Contributor

+ignoredScalacOptions = -Xfatal-warnings IMHO should be also added there
I would also add another options and change order - to have better coverage

@jvican

jvican approved these changes Jun 7, 2018 edited

Excellent contribution, this is what I meant by the whitelist. I think this will be useful to downstream users and it's in a shape in which it can be immediately used. Thank you @lukaszwawrzyk.

/cc @dwijnand @eed3si9n for another review + merge.

@dwijnand

LGTM. Thanks @lukaszwawrzyk.

@dwijnand dwijnand added this to the 1.2.0 milestone Jun 7, 2018

@dwijnand dwijnand merged commit ae02d7e into sbt:1.x Jun 7, 2018

1 check passed

continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment