The Reload exception that I added in the sbt package really wasn't
intended to be public. It's only meant to be used by
checkMetaBuildSources, which the users shouldn't override. I put it in
the top package though because I wanted it to be next to FullReload. I
also am not sure why the Reload object in Watch was private[sbt], but
while writing documentation, I realized that users couldn't access it.
While writing documentation for the watch subsystem, I realized that
it's awkward to configure watch to clear the screen before task
evaluation. To make this easier, I added a setting watchBeforeCommand
which is an arbitrary function that will run before the watch process
evaluates the command(s).
I also added helper functions for adding clear screen functionality.
I also realized that we weren't using the watchOnEnter or
watchOnExit callbacks anywhere. I had added these to support setting up
some state before watch starts and cleaning it up before it exits for
plugin authors. It makes sense to remove that functionality for 1.3.0
and only if a need presents itself re-add it in a later version of sbt.
I also made a few apis private[sbt] that I'm not sure about. Writing
documentation made me realize that some of these are redundant and/or
not ready for general consumption.
I had previously set some of the watch settings at the global level and
some at the project level. While writing documentation for the new watch
subsystem, I realized that the defaults should be set globally so that
they can be overridden at the ThisBuild level.
I also moved watchTriggers to sbt.nio.Keys. It was an oversight that it
wasn't moved there in a5cefd45be.
The classpath filter for test and run was added in #661 to ensure that
the classloaders were correctly isolated. That is no longer necessary
with the new layering strategies that are more precise about which jars
are exposed at each level. Using the filtered classloader was causing
the reflection used by spark to fail when using java 11.
This test ensures that a simple spark app will work with all of the
classloader layering strategies. Spark is an important part of the scala
ecosystem and heavily taxes classloading.
The test just checks that the app runs. I verified manually that the
first time that run is evaluated takes about 8 seconds regardless of the
layering strategy. It drops to 6 seconds with the ScalaLibrary strategy.
It drops to 1 seconds with AllLibraryJars.
We were incorrectly building the dependency layer in the run task using
the raw jars from dependencyClasspath rather than the actual classpath
jars (which may be different if bgCopyClasspath is true -- which it is
by default). This was preventing spark from working with AllLibraryJars
because it would load its classes and resources from the coursier cache
but the classpath filter would reject the resources because they came
from the coursier cache instead of the classpath.
The docs for ClassLoader,
https://docs.oracle.com/javase/8/docs/api/java/lang/ClassLoader.html
say that all non-hierarchical custom classloaders should be registered
as parallel capable. The docs also suggest that custom classloaders
should try to only override findClass so I reworked LayerdClassLoader to
only override findClass. I also added locking to the class loading to
make it safe for concurrent loading.
All of the custom classloaders besides LayeredClassLoader either
subclass URLClassLoader or LayeredClassLoader but don't override
loadClass. Because those two classloaders are parallel capable, the
subclasses should be as well. It isn't possible to make classloaders
that are implemented in scala parallel capable because scala 2 doesn't
support jvm static blocks (dotty does support this with an annotation).
To work around this, I re-worked some of the classloaders so that they
are either directly implemented in java or I subclassed a scala
implementation class in java.
Jave reflection did not work with layered classloaders if a dependency
attempted to load a class that was below the dependency layer in the
layered classloader hierarchy. The underlying problem was (in general) a
call to Class.forName somewhere. If the classloader parameter is not
specified, then Class.forName locates the ClassLoader for the caller
using reflection. It ultimately delegates to that ClassLoader's
loadClass method. With the previous LayeredClassLoader class, there was
no way for that classloader to access a URL that was below it in the
class loading hierarchy. I reworked LayeredClassLoader so that if it
fails to load the class, it will check the Thread's context classloader
and see if there are other LayeredClassLoader instances below it. If so,
it will then check if any of those classloaders would be able to load
the class by using findResource. If the descendant loader can load the
class, then we manually load it with findClass.
For best caching performance, we want to use the scala-reflect.jar that
is found in the scala instance. Also, in the runtime configuration,
caching didn't work correctly because we filtered the scala reflect
library from the dependency jars. We really only wanted to filter out
the library jars.
It also was problematic to use a LayeredClassLoader for the scala
reflect layer because in a subsequent commit I add the capability for a
layered classloader to load classes from its descendant loaders. This
caused problems when the scala-reflect layer was a LayeredClassLoader.
Instead, I add the ScalaReflectClassLoader class for better reporting.
I noticed that sometimes this test hangs in ci. I think it may be
because sometimes the event for A got handled while the cache entry for
B.scala was invalidated. Because the test runs ~compile, the next run of
compile would then reset the cache entry for B.scala. The next run of
watchOnIteration invalidtes B.scala again, but this time to the same
hash, so it doesn't actually trigger a file input event. By changing the
content of B.scala every time, we ensure that it should still cause a
file event.
I haven't confirmed that this will fix the sporadic hangs because I
can't reproduce on my machine, but this change won't hurt.
I had previously not though there was much reason to support commands in
continuous builds. This was primarily because there were a number of
questions regarding semantics. Commands cannot have fileInputs
specifically assigned to them because they don't have an associated
scope. They also can arbitrarily modify state so what is the expectation
when running ~foo where foo is a command that, for example, replaces
itself. I settled on the following semantics:
1) Commands run in a continuous build cannot modify the sbt execution
state which is to say that the state that is returned by continuous
is the same that was passed in (unless a reload occurred or we exited
the build with an exception)
2) Any global watchTriggers or fileInputs apply to a watched command.
They automatically inherit any fileInputs that are queried when
running tasks in a command. So, for example, ~+compile does what
you'd expect.
The implementation is fairly straightforward. If we can successfully
parse a command, but we cannot parse a scopedKey from it, we assign it a
private ScopedKey. When computing the watch settings for that key, we
will select the global settings through delegation. This is how it picks
up the global watchTriggers.
To run the command, I had to rework the task evaluation portion because
a command may return a state with additional commands to run. The cross
build command works this way. We recursively run all of the commands
starting with the original until we run out of commands to run. As part
of this work, I was able to remove the three argument version of
Command.processCommand that I'd previously added to support my old
approach to evaluating commands. This was a nice bonus.
I added scripted tests that check that global watchTriggers are picked
up and that commands that delegate to a command that uses fileInputs
automatically pick up those inputs during the watch. I also added a test
that approximates the ~+compile use case and ensures that the failure
semantics are what we expect and that the task runs for all defined
scala versions.
I'm debugging some test failures on travis in ServerSpec and I noticed
that very often noisy stack traces were printed because the process was
not given time to exit. There also was a confusing error message that
said that it would wait 30 seconds for the process to start, but it
would actually wait 90 seconds. I rewrote waitForPortfile to use
durations instead of integers to make it more clear. I changed
process from type Future[Process] to Process. There was no reason to
make it a future. I also made waitForString a blocking function because
it also didn't need to be implemented with a future.
I noticed that sbt 1.3.0 was using more cpu when idling (either at the
shell or while waiting for file events) than 1.2.8. This was because I'd
reduced a number of timeouts to 2 milliseconds which was causing a
thread to keep waking up every 2 milliseconds to poll a queue. I thought
that this was cheaper than it actually is and drove the cpu utilization
to O(10%) of a cpu on my mac.
To address this, I consolidated a number of queues into a single queue
in CommandExchange and Continuous. In the CommandExchange case, I
reworked CommandChannel to have a register method that passes in a Queue
of CommandChannels. Whenever it appends an exec, it adds itself to the
queue. CommandExchange can then poll that queue directly and poll the
returned CommandChannel for the actual exec. Since the main thread is
blocking on this queue, it does not need to frequently wake up and can
just poll more or less indefinitely until a message is received. This
also reduces average latency compared to older versions of sbt since
messages will be processed almost as soon as they are received.
The continuous case is slightly more complicated because we are polling
from two sources, stdin and FileEventMonitor. In my ideal world, I'd
have a reactive api for both of those sources and they would just write
events to a shared queue that we could block on. That is nontrivial to
implement, so instead I consolidated the FileEventMonitor instances into
a single FileEventMonitor. Since there is now only one FileEventMonitor
queue, we can block on that queue for 30 milliseconds and the poll
stdin. This reduces cpu utilization to O(2%) on my machine while still
having reasonably low latency for key input events (the latency of file
events should be close to zero since we are usually polling the
FileEventMonitor queue when waiting).
I actually had a TODO about the FileEventMonitor change that this
resolves.
I noticed that ~run in the Play plugin relied on the presence of the
ContinuousEventMonitor key. Rather than completely break that feature, I
re-added the ContinuousEventMonitor attribute to the state in a
continuous build. That being said, the play team does need to update
their plugin because reading from the console no longer works in 1.3.0 so the
user has to Ctrl-C to exit the watch. I think the best way for them to
fix this is to override the '~' command in their plugin and if the input
is 'run', then they do their custom thing, otherwise they delegate to
the default '~' command.
The intent was to bring prominence to the desires expressed in the
contributing guidelines. But nowadays GitHub has ways in which it
informs users (e.g. "the contributing guidelines have changed since you
last contributed"). So I think we can drop this now.
Not caching scala reflect is extremely painful if the build uses
scalatest. It adds O(1second) to my watch performance benchmarks. It
actually made sbt 1.3.0 much slower than 0.13.17
I was benchmarking sbt with turbo mode on and found that tests weren't
running. This was because we were inadvertently excluding all of the
dependency jars from the dynamic classpath. I have no idea why the
scripted tests didn't catch this.
The scalatest scripted test didn't catch this because 'test' just
automaticaly succeeds if no test frameworks are found. To guard against
regression, I had to ensure that 'test' failed for every strategy if a
bad test file was present.
We want to check the build sources before any command runs, not just
tasks. To achieve this, I moved the logic for checking for build source
changes to CommandProcess.processCommand. Also, @smarter had noticed
that if a user modified a build file and then ran reload, a warning
would be displayed about changed build sources even though they had just
ran reload. This was because running reload didn't update the previous
cache for checkBuildSources / fileInputStamps. I fixed that bug by
running 'checkBuildSources / changedInputFiles' instead of
'checkBuildSources' when the user runs reload.
I verified that after this change:
- If I changed a build file and ran 'show version' a warning was printed
before it displayed the version. If I also set
global / onChangedBuildSource := ReloadOnSourceChanges, it
automatically reloaded before displaying the version.
- If I changed a build source and ran 'reload', followed by
'show version', no warnings were ever displayed.
As an implementation detail, I had to add the Aggregation.suppressShow
attribute key. We set this key to true before checking the build
sources. Without this, log.success is called whenever we check the build
sources which is both confusing and noisy.
Because we are sharing the scala library classloader with test and run,
it is possible that sbt will be competing with for resources with the
test and run tasks when trying to get threads from the global execution
context. Also, by using our own execution context, we can shut it down
when sbt exits.
The motivation for this change is that I was looking at the active jvm
threads of an idle sbt process and noticed a bunch of global execution
context threads.
Ethan Atkins
eugene yokota
Dale Wijnand
Ethan Atkins