Commit Graph

2853 Commits

Author SHA1 Message Date
Ethan Atkins fe397351df Remove dead external hooks code
These settings were only used by the external hooks implementation that
was removed.
2020-06-24 19:19:06 -07:00
eugene yokota 778264a319
Merge pull request #5618 from adpi2/topic/metals-support
Add suggestion about semanticdb when Metals connect to sbt
2020-06-24 14:19:04 -04:00
Ethan Atkins 3c51f01872 Dealias baseDirectory in AppConfiguration
In Load.scala and Defaults.scala, the AppConfiguration.baseDirectory is
dealiased when it is a symlink. This commit dealiases the
AppConfiguration.baseDirectory if it is a symlink so that sbt
`appConfiguration.value.baseDirectory` should be the same as
`baseDirectory.value`.
2020-06-23 16:48:14 -07:00
Ethan Atkins 33d5bce75a Filter all watch settings for unused key check
Rather than enumerate all of the watch keys that may appear unused
though they can be used by the `~` command, rework lintUnused to take a
function `String => Boolean` instead of `Set[String] => Boolean`.
2020-06-23 16:01:08 -07:00
Ethan Atkins 95221ed837 Remove dead external hooks code
These settings were only used by the external hooks implementation that
was removed.
2020-06-23 15:06:53 -07:00
Ethan Atkins 63cd8f53c6 Use real paths for zinc base path
The AppConfiguration.baseDirectory is dealiased during project loading.
Not dealiasing the symlink here could cause a discrepancy between the
`baseDirectory` key and the value of the base key in the root paths map.
2020-06-23 15:06:53 -07:00
eugene yokota e323f1f713
Merge pull request #5637 from adpi2/feature/bsp-custom-config
Add BSP support for IntegrationTest and other custom configs
2020-06-23 16:11:14 -04:00
Maksim Ochenashko 71ea117a39 Use contraband for generation of RemoteCacheArtifact 2020-06-21 09:41:56 +03:00
Maksim Ochenashko 0c07fa1851 Allow customization of remote cache artifacts 2020-06-20 14:05:41 +03:00
adpi2 56eed20b34 add support for IntegrationTest and custom configs in BSP
In global bspWorkspace setting, retrieve all projects and all configurations that contain the bspTargetIdentifier setting, so that:
- the IntegrationTest configuration, when added to a project, is automatically associated to a BSP target
- a custom configuration that contains the `Defaults.configSettings` is also associated to a BSP target
2020-06-18 16:32:10 +02:00
eugene yokota 9f9bb9daa2
Merge pull request #5634 from eed3si9n/wip/missingok
Fixes missingOk under Coursier
2020-06-17 21:06:40 -04:00
Eugene Yokota 5cce074b83 Fixes missingOk under Coursier
Fixes https://github.com/sbt/sbt/issues/4707
Ref https://github.com/coursier/sbt-coursier/pull/212
2020-06-17 20:06:25 -04:00
Maksim Ochenashko 49e7d71cbc Delete META-INF folder after remote cache pull 2020-06-17 13:01:47 +03:00
eugene yokota 87e57b5390
Merge pull request #5619 from bjaglin/patch-1
exclude effectful initialize setting key from linting
2020-06-16 19:53:07 -04:00
Adrien Piquerez 0738edc9a5
Apply logging style
Co-authored-by: eugene yokota <eed3si9n@gmail.com>
2020-06-16 09:43:22 +02:00
Maksim Ochenashko 3e8073151b Unify calculation of remoteCacheId and remoteCacheIdCandidates 2020-06-16 09:15:42 +03:00
Brice Jaglin 561d0ef602 exclude effectful initialize setting key from linting 2020-06-16 01:08:15 +02:00
adpi2 159171bba5 Add suggestion about semanticdb when Metals connect to sbt
Try parse the required semanticdbVersion in the initialization request metadata
Issue a warning if the semanticdb plugin is not enabled
Issue a warning if the semanticdb version is lower than the required
2020-06-15 16:57:49 +02:00
Eugene Yokota 8789d20724 -Dsbt.semanticdb=true to enable SemanticDB 2020-06-14 17:14:02 -04:00
Eugene Yokota 837dcbb5a6 SemanticDB 4.3.15 2020-06-14 16:53:36 -04:00
Eugene Yokota 5a37ef14fc Zinc 1.4.0-M6 2020-06-14 15:55:37 -04:00
Eugene Yokota 033ff1d8a5 Make JSON parsing errors more consistent 2020-06-11 20:31:13 -04:00
Eugene Yokota cad84afc6d Drop old application/sbt-x1 protocol 2020-06-11 16:22:07 -04:00
eugene yokota 975e3db43e
Merge pull request #5558 from eed3si9n/wip/selective
Selective functor
2020-06-10 17:39:13 -04:00
eugene yokota a83be809ab
Merge pull request #5552 from eed3si9n/wip/promise
implement Def.promise
2020-06-10 17:38:23 -04:00
Eugene Yokota c8f52e6281 Add atMost parameter 2020-06-10 15:30:39 -04:00
Eugene Yokota b78e4f0919 implement Def.promise
This adds `Def.promise` a facility that wraps `scala.concurrent.Promise`. Project layer, there's an implicit for task-that-returns-promise (`Def.Initialize[Task[PromiseWrap[A]]]`) that would inject `await` method, which returns a task. This is a special task that is tagged with `Tags.Sentinel` so that it will bypass the concurrent restrictions. Since there's no CPU- or IO-bound work, this should be ok.

The purpose of this promise for long-running task to communicate with another task midway.
2020-06-10 15:16:25 -04:00
Eugene Yokota 6dd39e7ab8 try last 5 commits to look for remote cache
In case there are a few local commits ahead of the remote cache, this would still grab the close point in the history to resume the build.
2020-06-10 12:55:30 -04:00
Eugene Yokota 585f8399ba implement RemoteCache
This adds `pushRemoteCache`, `pushRemoteCacheTo`, `pullRemoteCache`, etc to implement cached compilation facility.

In addition, the analysis file location is now made more clear.
2020-06-09 14:28:40 -04:00
eugene yokota a109f3d76d
Merge pull request #5526 from 3rwww1/fix/npe-coursier-null-cred-realm
Fix coursierint NPE when credential realm is null
2020-06-09 14:24:24 -04:00
Eugene Yokota 0d15fe1162 Remove HTTP support without explicit opt-in
Ref https://github.com/sbt/sbt/issues/4905
2020-06-07 01:50:41 -04:00
Eugene Yokota ada490e61d Fixes appResolvers returning None
Fixes https://github.com/sbt/sbt/issues/5582
Ref https://github.com/sbt/sbt/pull/5576

In #5576 I added `m.allowInsecureProtocol`, which causes reflection error for older launcher.jar, which then falls back to None appResolvers.
2020-05-30 17:52:39 -04:00
Eugene Yokota 9264b128ef Lower-case some messages 2020-05-29 02:55:12 -04:00
Eugene Yokota 2bf1bcc884 Add welcome banner with Java version
Fixes https://github.com/sbt/sbt/issues/5544
2020-05-29 02:35:59 -04:00
eugene yokota 24f367fa07
Merge pull request #5576 from eed3si9n/wip/bumplauncher
update to launcher 1.1.4
2020-05-28 17:00:00 -04:00
Eugene Yokota 4bb4c3a9b6 launcher 1.1.4
Forward `m.allowInsecureProtocol` to `MavenRepository`.
2020-05-28 16:21:46 -04:00
Adrien Piquerez a498a20627
Merge branch 'develop' into topic/build-server-protocol 2020-05-28 08:57:23 +02:00
Adrien Piquerez 0789fd7be6 Use java command in BspConnectionDetails 2020-05-25 13:32:48 +02:00
Adrien Piquerez b184be860f Add headers 2020-05-25 10:43:54 +02:00
Adrien Piquerez 914b592fb2 scalafmt 2020-05-25 09:40:54 +02:00
Adrien Piquerez 4f00abf1ba BSP is part of the JVM plugin 2020-05-22 13:39:59 +02:00
Adrien Piquerez c11ee3269c Add BspCompilationTaskProgress 2020-05-22 11:15:47 +02:00
Dale Wijnand ff97fb6068 Avoid making NPEs out of OOMEs!
(cherry picked from commit ba29f65f17c7b9d9ac5bf79d0934cf23438a16b4)
2020-05-19 21:30:53 +01:00
Adrien Piquerez a31747758c Create BSP connection file at server startup 2020-05-18 09:35:14 +02:00
Eugene Yokota 45e8426d27 def taskIf[A](a: A): Initialize[Task[A]]
Make `Def.taskIf` accept pure `A` type, as opposed to `Def.task { ... }`.
2020-05-17 23:44:35 -04:00
Eugene Yokota c6f62293f1 Def.taskIf macro
Def.taskIf accepts an if-expression or a block ending in an if-expression.
2020-05-17 23:36:04 -04:00
Eugene Yokota fc791cc23e Rewrite tasks using Def.ifS 2020-05-17 23:36:04 -04:00
Adrien Piquerez c80fe525c6 add BspClient 2020-05-16 09:52:21 +02:00
Adrien Piquerez 2c8a322bd8 bspWorkspace is a setting 2020-05-16 09:52:21 +02:00
Adrien Piquerez f3bce7e976 fix BuildServerReporter 2020-05-16 09:52:21 +02:00
Adrien Piquerez 24f6a6f290 add BSP buildTarget/dependencySources 2020-05-16 09:52:21 +02:00
Adrien Piquerez baa47c88c3 fix BSP logging and response 2020-05-16 09:52:21 +02:00
Adrien Piquerez 98750817c6 fix internal transitive mgmt in BSP 2020-05-16 09:52:21 +02:00
Adrien Piquerez 068fe2ad0a add BuildServerCapabilities 2020-05-16 09:52:21 +02:00
Adrien Piquerez 3ae42ae9c6 fix scala jars and add capabilities in BuildTarget 2020-05-16 09:52:21 +02:00
Adrien Piquerez 5172775b80 fix classpath in buildTarget/scalacOptions 2020-05-16 09:52:21 +02:00
Adrien Piquerez 454ee61289 separate BSP and LSP handlers + add bspWorkspace task 2020-05-16 09:52:21 +02:00
Adrien Piquerez a415cd0cfc replace LSP compiler reporter by BSP one 2020-05-16 09:52:20 +02:00
Adrien Piquerez f89cef1fd0 add bspCompile task 2020-05-16 09:52:20 +02:00
Eugene Yokota f6ff4da1c5 Disable aggregation for global input tasks 2020-05-16 09:52:20 +02:00
Eugene Yokota d2d0a9ca80 buildTarget/scalacOptions 2020-05-16 09:52:20 +02:00
Eugene Yokota 10b2154d2e Refactor bspBuildTargetSources to be an input task + ScopeFilter 2020-05-16 09:52:20 +02:00
Eugene Yokota 1ccff0ca6d Add Scala Build Target 2020-05-16 09:52:20 +02:00
Eugene Yokota 95f3877bae Add stubs for build/shutdown and build/exit 2020-05-16 09:52:20 +02:00
Eugene Yokota cb93d20492 build server protocol
Initial draft for bsp support.

This shows two communication pattern around BSP.
First, if the request can be handled with the build knowledge is readily available in `NetworkChannel` we can reply immediately. `BuildServerImpl#onBspBuildTargets` is an example for that.

Second, if the request requires `State`, then we can forward the parameter into a custom command, and reply back from a command. `BuildServerProtocol.bspBuildTargetSources` is an example of that since it needs to invoke tasks to generate sources.
2020-05-16 09:52:20 +02:00
Alexandre Archambault f850a9c966 Update coursier to 2.0.0-RC6-4
And warn at start-up if ~/.coursier/cache is found.
2020-05-14 15:54:43 +02:00
Adrien Piquerez ae1df53033 fix relay appender 2020-05-13 14:11:45 +02:00
eugene yokota 4592493617
Merge pull request #5549 from adpi2/issue/json-response
Prevent more than one response per json RPC request
2020-05-12 22:07:11 -04:00
Adrien Piquerez c221f57812 code review 2020-05-12 19:20:43 +02:00
Adrien Piquerez 8df754eeb1 rename publish to either respond or notify 2020-05-12 16:26:33 +02:00
Adrien Piquerez 255a0a6ea6 send response to the source channel only 2020-05-12 14:44:10 +02:00
Adrien Piquerez df293fbfd5 prevent multiple response to a single request 2020-05-12 10:38:47 +02:00
Adrien Piquerez 781584d137 id is mandatory in json rpc responses 2020-05-11 16:51:34 +02:00
Brice Jaglin 331ca6ec9b enable ServiceLoader discovery across classloader layers
java.util.ServiceLoader uses findResources(), which was not
overriden in ReverseLookupClassLoader, causing resources available
in the descendant classloader not to be discovered when a service
loader instance was using the top classloader.
2020-05-08 12:41:44 +02:00
eugene yokota f5eae27c69
Merge pull request #5531 from eatkins/optimize-task-progress
Optimize task progress performance
2020-05-06 23:07:39 -04:00
eugene yokota d2fa0f100e
Merge pull request #5533 from eatkins/ivy-xml
Make IvyXml.writeFiles private[sbt]
2020-05-06 11:46:29 -04:00
Ethan Atkins e3e53df797 Make IvyXml.writeFiles private[sbt]
I got confused an changed the access for the wrong method to support
https://github.com/sbt/sbt/pull/5364.

Partially reverts 5c55393f1b.
2020-05-06 07:43:22 -07:00
Ethan Atkins 9b2bbdd4cc Report progress on background thread
Having the progress reports directly generated in beforeWork delays the
task from being submitted to the executor. This commit moves all of the
reporting onto the background thread to avoid these delays since
progress is less important than task evaluation.
2020-05-05 21:28:08 -07:00
Ethan Atkins f4c11a63ab Re-implement meta build source checking
The old implementation of checkBuildSources can easily take 20ms to run
when called in MainLoop.processCommand. It is rarely faster than 4-5ms.
To reduce this overhead, I stopped using the checkBuildSources task in
processCommand. Instead, I manually cache the build source hashes in a
global state variable and add a file monitor that invalidate the entire
set of source hashes if any changes are detected. This could probably be
more efficient, but I figure that build sources change infrequently
enough that it's fine to just invalidate the entire list of source
hashes.

Because the CheckBuildSources instance is already watching the meta
build, I reworked Continuous to use that FileTreeRepository for the
build sources if it is available.

Bonus: fixes https://github.com/sbt/sbt/issues/5482
2020-05-05 21:26:04 -07:00
Ethan Atkins b829ac9796 Remove FileManagement
This was unused.
2020-05-05 21:25:56 -07:00
Ethan Atkins 0a06bfe2d5 Optimize TaskProgress.containsSkipTasks
I was surprised to find this method in a flamegraph* so I optimized it.
TaskProgress was actually on the hotpath of task evaluation so every
single task was slower to be enqueued with the CompletionService. After
this change, containsSkipTasks dropped out of the flamegraph.

*The flamegraph was for a compile loop where sbt constantly modified a
single source file and re-compiled it. The containsSkipTasks method
appeared in over 2% of the method calls.
2020-05-05 21:04:09 -07:00
frosforever d1cf37b2ca Update Tests.Group to be bin compat by manually implementing case class methods 2020-05-04 09:29:31 -04:00
frosforever 18d51eac58 Add tags to inProccess group as well 2020-05-04 09:29:31 -04:00
frosforever b2942f6321 Add user defined tags to Tests.Group 2020-05-04 09:29:31 -04:00
Erwan Queffelec 23fdb324cb Fix coursierint NPE when credential realm is null
As stated in #5492 and #2366 some artifact hosting services (at least
Azure Artifacts, seems to affect GCP as well) do not offer a stable HTTP
Auth realm that can be safely set in SBT credentials, hence the need to
support null or empty Credentials.

The Ivy (publishing) side of the issue was fixed in sbt/ivy#36

As to the resolving side, This commit is only part of the solution as it
just prevents an NPE and does not consider if coursier itself will fall
back to finding credentials with a null realm that matches the server
hostname.

Related-to: #5492
Related-to: #2366
Signed-off-by: Erwan Queffelec <erwan.queffelec@gmail.com>
2020-05-03 15:53:19 +02:00
Ethan Atkins a1ea5b72f3 Support capital letter options in doLoadFailed
Also display invalid response character.
2020-05-01 13:12:37 -07:00
Ethan Atkins 73235d12e0 Switch to lower case for some messages
There is push to move away from complete sentences in warnings.
2020-05-01 13:12:13 -07:00
Ethan Atkins 24d4b3549b Set default supershell blank zone to 1
We do not need a large blank zone with the virtual system.out.
2020-05-01 12:35:43 -07:00
Ethan Atkins 5c42c20cdc Simplify setting dumbTerm variable 2020-05-01 12:35:43 -07:00
Ethan Atkins 58822cc3f5 Add virtual System.out for supershell
In order to make supershell work with println, this commit introduces a
virtual System.out to sbt. While sbt is running, we override the default
java.lang.System.out, java.lang.System.in, scala.Console.out and
scala.Console.in (unless the property `sbt.io.virtual` is set to
something other than true). When using virtual io, we buffer all of the
bytes that are written to System.out and Console.out until flush is
called. When flushing the output, we check if there are any progress
lines. If so, we interleave them with the new lines to print.

The flushing happens on a background thread so it should hopefully not
impede task progress.

This commit also adds logic for handling progress when the cursor is not
all the way to the left. We now track all of the bytes that have been
written since the last new line. Supershell will then calculate the
cursor position from those bytes* and move the cursor back to the
correct position. The motivation for this was to make the run command
work with supershell even when multiple main classes were specified.

* This might not be completely reliable if the string contains ansi
cursor movement characters.
2020-05-01 12:35:43 -07:00
Ethan Atkins a0012bab75 Apply minimum task progress length threshold
To reduce the noise of supershell, this commit imposes a 10 millisecond
minimum duration before a task will appear in the supershell lines.
2020-05-01 12:35:43 -07:00
Ethan Atkins 9218d3c087 Redraw command prompt after network command
Presently if a server command comes in while in the shell, the client
output can appear on the same line as the command prompt and the command
prompt will not appear again until the user hits enter. This is a
confusing ux. For example, if I start an sbt server and type
the partial command "comp" and then start up a client and run the clean
command followed by a compile, the output looks like:

[info] sbt server started at local:///Users/ethanatkins/.sbt/1.0/server/51cfad3281b3a8a1820a/sock
sbt:scala-compile> comp[info] new client connected: network-1
[success] Total time: 0 s, completed Dec 12, 2019, 7:23:24 PM
[success] Total time: 0 s, completed Dec 12, 2019, 7:23:27 PM
[success] Total time: 2 s, completed Dec 12, 2019, 7:23:31 PM

Now, if I type "ile\n", I get:
[info] sbt server started at local:///Users/ethanatkins/.sbt/1.0/server/51cfad3281b3a8a1820a/sock
ile
[success] Total time: 0 s, completed Dec 12, 2019, 7:23:34 PM
sbt:scala-compile>

Following the same set of inputs after this change, I get:
[info] sbt server started at local:///Users/ethanatkins/.sbt/1.0/server/51cfad3281b3a8a1820a/sock
sbt:scala-compile> comp
[info] new client connected: network-1
[success] Total time: 0 s, completed Dec 12, 2019, 7:25:58 PM
sbt:scala-compile> comp
[success] Total time: 0 s, completed Dec 12, 2019, 7:26:14 PM
sbt:scala-compile> comp
[success] Total time: 1 s, completed Dec 12, 2019, 7:26:17 PM
sbt:scala-compile> compile
[success] Total time: 0 s, completed Dec 12, 2019, 7:26:19 PM
sbt:scala-compile>

To implement this change, I added the redraw() method to LineReader
which is a wrapper around ConsoleReader.drawLine; ConsoleReader.flush().
We invoke LineReader.redraw whenever the ConsoleChannel receives a
ConsolePromptEvent and there is a running thread.

To prevent log lines from being appended to the prompt line, in the
CommandExchange we print a newline character whenever a new command is
received from the network or a network client connects and we believe
that there is an active prompt.
2020-05-01 12:35:43 -07:00
Ethan Atkins 08091d64c1 Log server commands
Prior to this change, if a network command came in, it would run in the
background with no real feedback in the server ui. Prior to this change,
running compile from the thin client would look like:

sbt:scala-compile>
[success] Total time: 1 s, completed Dec 12, 2019, 7:24:43 PM
sbt:scala-compile>

Now it looks like:
sbt:scala-compile>
[info] Running remote command: compile
[success] Total time: 1 s, completed Dec 12, 2019, 7:26:17 PM
sbt:scala-compile>
2020-05-01 12:35:43 -07:00
Ethan Atkins 5afc0f0fdf Don't require newline for load failed commands
It's a bit annoying to have to hit enter here.

Also, this should fix https://github.com/sbt/sbt/issues/5162 because if
there is no System.in attached, the read will return -1 which will cause
sbt to quit.
2020-05-01 12:35:43 -07:00
Ethan Atkins c7b52203a0 Don't require newlines for main classes
There typically are fewer than 10 main classes in a project so allow the
user to just input a single digit in those cases. Otherwise fallback to
a line reader.
2020-05-01 12:35:43 -07:00
Ethan Atkins 44ef718448 Improve run command warning
When the user inputs `run` or `runMain` we shouldn't print the warning
about multiple classes because in the case of run they already will be
prompted to select the classes and in the case of runMain, they are
already required to specify the class name.

Bonus:
 * improve punctuation
 * add clear screen to selector dialogue
 * print selector dialogue in one call to println -- this should prevent
   the possibility of messages from other threads being interlaced with
   the dialogue
2020-05-01 12:35:43 -07:00
Ethan Atkins 7902ec3b7d Add Terminal abstraction
This commit aims to centralize all of the terminal interactions
throughout sbt. It also seeks to hide the jline implementation details
and only expose the apis that sbt needs for interacting with the
terminal.

In general, we should be able to assume that the terminal is in
canonical (line buffered) mode with echo enabled. To switch to raw mode
or to enable/disable echo, there are apis: Terminal.withRawSystemIn and
Terminal.withEcho that take a thunk as parameter to ensure that the
terminal is reset back to the canonical mode afterwards.
2020-05-01 12:35:43 -07:00
Ethan Atkins cd65543d10 Deprecate unused ConsoleUnpromptEvent 2020-05-01 12:28:44 -07:00
Ethan Atkins 2f4c603be6 Stop continuous input thread if System.in is closed
The watch tests take forever on windows because wrapped.read() always
returns -1 during scripted.
2020-05-01 12:28:44 -07:00
Ethan Atkins 7d07bbabbf Fix DefinesClass implementation for jdk > 8
When trying to use any jdk > 8 with the latest sbt, sbt will die in some
projects because it tries to call Locate.defineClass on rt.jar, which
is represented with a DummyVirtualFile and causes a crash.
2020-04-30 20:43:04 -07:00
Eugene Yokota 7c4b01d9f7 Comment out external hooks
Zinc now uses farm hash to invalidate the virtual paths. To use watch to detect initial changes, we need to revalidate using content hash.
2020-04-24 17:44:15 -04:00
Eugene Yokota 2396b449fe Contraband 0.4.6 2020-04-24 17:44:15 -04:00
Eugene Yokota 3ce4d22b84 integrate with VirtualFile changes
Ref https://github.com/sbt/zinc/pull/712
2020-04-24 17:44:14 -04:00
Eugene Yokota 0a5c2edddf Fix strict switch command so +task is fixed 2020-04-24 01:18:16 -04:00
Eugene Yokota 11a403251a Fix cross + input task 2020-04-24 01:08:21 -04:00
Eugene Yokota 063b32bbba Fix cross + scoped task 2020-04-24 01:06:18 -04:00
Eugene Yokota 588d01b2dd Fix switch command -v flag 2020-04-24 01:04:05 -04:00
Anil Kumar Myla 41c3f433e6
Update semanticdb to work with scala 2.12.11 2020-03-20 16:12:07 -07:00
João Ferreira 09b2113379
fix units 2020-03-19 15:20:22 +00:00
João Ferreira 030252b653
Fix TaskTimings scaladocs 2020-03-19 15:07:26 +00:00
Anil Kumar Myla 887eb17f9e
Update scala to 2.12.11 2020-03-18 00:49:14 -07:00
Dale Wijnand b58c99efee Make ScriptedLauncher support Scala pre-releases
Also add a dash of sanity checks here and there.
2020-03-12 15:32:02 +00:00
eugene yokota 0b12862caf
Merge pull request #5439 from dwijnand/introduce-Taskable
Introduce Taskable & toTaskable
2020-02-28 10:43:16 -05:00
eugene yokota a2563f0088
Merge pull request #5447 from eed3si9n/wip/ordering
Make bare setting loading order alphabetical
2020-02-26 16:25:24 -05:00
Eugene Yokota 4b847b148f Make bare setting loading order alphabetical
Fixes https://github.com/sbt/sbt/issues/2697
Ref https://twitter.com/not_xuwei_k/status/1230140477959286848

Note that this is could potentially break an existing build that was relying on previous behavior, which seem to return `build.sbt` at the end if you have `a.sbt`, `b.sbt`, `c.sbt`, and `build.sbt`.
2020-02-24 18:16:01 -05:00
Alex Zolotko 92b2eaadf8 Prevent RejectedExecutionException during BackgroundRunnable.cleanup 2020-02-21 10:45:51 +01:00
Dale Wijnand 722022ae97 Add conversions from Init/+Task to Taskable 2020-02-17 12:22:53 +00:00
Dale Wijnand fdfdd1ca47 Extract Taskable from ScopedTaskable 2020-02-17 12:22:53 +00:00
Eugene Yokota 75365e13d1 Add getSetting to UpperStateOps
This returns Option[A].
2020-02-16 22:32:06 -05:00
Eugene Yokota 25a79d0ac6 Add State extension
Fixes https://github.com/sbt/sbt/issues/3112

This unpacks Extracted as State's extension methods.

In addition this provides a way of responding via LSP.
2020-02-15 19:32:19 -05:00
eugene yokota 7360e6342e
Merge pull request #5403 from eed3si9n/wip/artifacts
Fix the default artifact of packageSrc for custom configuration
2020-02-03 13:46:01 -05:00
Arnout Engelen 88623828d4 Use project resolvers before dependency resolvers in coursier
csrResolvers.all evaluates all possible scopes in arbitrary order. This change
makes sure at least the project resolvers are placed before any resolvers from
dependency projects.
2020-02-03 13:22:34 +01:00
Eugene Yokota c38181d39f Fix the default artifact of packageSrc for custom configuration
Fixes https://github.com/sbt/sbt/issues/5391
2020-01-26 22:55:26 -05:00
Ethan Atkins 15d3ed1298 Add extra classpath to the metabuild
When sbt was entered through xMain.run and the classloaders do not have
the expected format, sbt recreates the classloaders for itself.
Unfortunately the extra classpath was not added to the classloader. This
caused project/extra to fail if it was entered from RunFromSourceMain
rather than with the launcher.
2020-01-19 09:04:27 -08:00
Ethan Atkins 2f99797bac Fix RunFromSourceMain sbt.Package$ bug
The main reason for having both the RunFromSourceMain and LauncherBased
scripted tests was that RunFromSourceMain would fail for any test that
ended up accessing the sbt.Package$ object. This commit fixes this bug
by reworking the classloader generated by RunFromSourceMain to invoke
sbt, switching from the classpath to jar classpath (by setting exportJars =
true) and entering sbt by calling `new xMain().run` rather than
`xMain.run`.

The reason for switching to the jar classpath is that the jvm seems to
have issues when there are two classes provided in different directories
that have the same case insensitive name, e.g. `sbt.package$` and
`sbt.Package$`. If those classes are instead provided in different
jars, the jvm seems to be able to handle it.

Exporting the jars is not enough though, I had to rework the
ClassLoader created in the launch method to have a layout that was
recognized by xMainConfiguration. I reimplemented the AppConfiguration
in java so that it could bootstrap itself in a single jar classloader
(the only needed jar is the Scripted.

If we export the jars in the build, then the NoClassDefErrors for
`sbt.Package$` go away during scripted tests using RunSourceFromMain.
This might make running tests in subprojects slightly slower but I think
its a worthy tradeoff.
2020-01-19 09:04:26 -08:00
Ethan Atkins 17deb8b5d6 Make publishLocalBin work without prior publishLocal
In order for the sbt launcher to be able to resolve a local version of
sbt, we must publish the main jar, the sources jar, the doc jar, the pom
and an ivy.xml file. The publish and publishLocal tasks are wired in
IvyXml.scala to create an ivy.xml file before running publish. This
wasn't done with publishLocalBin which made it not work when no ivy.xml
file was already present (which was the case after running clean).
2020-01-18 16:11:37 -08:00
Ethan Atkins cf745255e8 Apply javafmt in sbt project 2020-01-14 14:38:08 -08:00
Ethan Atkins 11174fb382 Use implicit val rather than import
This import was causing an unused import warning during doc.
2020-01-13 10:25:34 -08:00
eugene yokota 9b931c4bf8
Merge pull request #5350 from mrArkwright/fix-4451
introduce SysProp sbt.testing.legacyreport
2020-01-09 13:30:24 -05:00
eugene yokota 6257d90ddc
Merge pull request #5353 from eatkins/run-main-supershell
Add runMain to supershell blacklist
2020-01-09 13:29:51 -05:00
Ethan Atkins ccce238b16 Add runMain to supershell blacklist
See https://github.com/sbt/sbt/issues/5352.
2020-01-08 10:52:26 -08:00
Eugene Yokota 36a16673c0 reduce compiler warnings 2020-01-08 09:41:29 -05:00
Jannik Theiß af245d2494 SysProp sbt.testing.legacyreport: fix integration tests, JUnitXmlTestsListener backward binary compatibility 2020-01-08 15:29:48 +01:00
Jannik Theiß cea516175f introduce SysProp sbt.testing.legacyreport
either create test reports with legacy file names (legacyreport=true) or with standard file names (legacyreport=false or omitted) but not both as suggested in #4451
2020-01-08 12:23:09 +01:00
Eugene Yokota 3be047d050 clear banner 2019-12-31 21:38:28 -05:00
eugene yokota 73b4d4b158
Merge pull request #5328 from dwijnand/message-reboot
Revert "clarify message on sbt.version mismatch"
2019-12-29 03:06:09 -05:00
eugene yokota bf9225bccf
Merge pull request #5344 from eed3si9n/wip/repeatable
Don't emit timestamps when packaging to jar, take 2
2019-12-29 02:58:53 -05:00
Eugene Yokota a8ab4ada68 Replace getResource("") trick
Fixes https://github.com/sbt/sbt/issues/5339

It seems like some tests are using `ClassLoader#getResource("")` to acquire the `classes` directory path. This does not seem to work on sbt 1.3.6, which returns `file:/home/travis/.cache/coursier/v1/https/repo1.maven.org/maven2/org/apache/logging/log4j/log4j-api/2.11.2/log4j-api-2.11.2.jar!/META-INF/versions/9/`. To workaround this issue, I've switched to loading the known folder name instead.
2019-12-27 16:43:20 -05:00
Dale Wijnand f5e73b610a Keep "using" change in sbt.version mismatch messaging. 2019-12-26 08:28:06 +00:00
Arnout Engelen 1d0a415200 SOURCE_DATE_EPOCH is in seconds, let's pass milliseconds 2019-12-24 10:11:09 +01:00
Arnout Engelen 4353098454 Target develop branch of io, support SOURCE_DATE_EPOCH 2019-12-24 10:11:09 +01:00
Arnout Engelen 21533863da Don't emit timestamps when packaging to jar
This makes the build more deterministic.
2019-12-24 10:11:09 +01:00
Ethan Atkins d445590d9d Fix cross multi command performance
In 53788ba356, I changed the cross multi
parser to issue all of the commands sequentially. This caused a
performance regression for many use cases:
https://github.com/sbt/sbt/issues/5321. This commit restores the old
behavior of `+` if the command to run has no arguments.
2019-12-23 14:45:57 -08:00
Dale Wijnand 56aa46308b Revert "clarify message on sbt.version mismatch"
This reverts commit 2f4b6f476a.
2019-12-23 22:25:37 +00:00
Eugene Yokota 556098ec31 Don't close test ClassLoader by default
Fixes https://github.com/sbt/sbt/issues/5262
2019-12-23 13:33:32 -08:00
Ethan Atkins 424fe958e1 Revert "Place scalatest framework jar in its own classloader"
This partially reverts commit 8518c4b4fd.

I left in the useful changes to ReverseLookupClassLoader.
2019-12-23 13:33:04 -08:00
eugene yokota ae01f25bab
Merge pull request #5317 from eed3si9n/wip/cross
workaround client / clean problem
2019-12-20 15:33:59 -05:00
Renato Cavalcanti 2f4b6f476a
clarify message on sbt.version mismatch 2019-12-19 08:21:28 +01:00
Eugene Yokota faa1540009 workaround client / clean problem
Ref https://github.com/sbt/sbt/issues/5314
Ref https://github.com/sbt/sbt/pull/5265

In sbt 1.3.4, it's possible to define a subproject named `client`.
The current parser behaves differently whether we calll `client/clean` or `client / clean` with whitespaces. The one with the whitespace invokes `client` command (as in thin client). This gets triggered by `+clean` because the new implementation uses whitespace.
2019-12-17 12:52:18 -05:00
eugene yokota 19c3b44b59
Merge pull request #5303 from eed3si9n/wip/cache_removal
Fixes update task not invalidating
2019-12-13 05:57:48 -05:00
Eugene Yokota 2b24f05435 Fixes update task not invalidating
Fixes https://github.com/sbt/sbt/issues/5292
Ref https://github.com/sbt/sbt/issues/5142

`update` task checks if the timestamp is still the same from the previous resolution. This no longer works since lm-coursier does not populate the timestamps in `UpdateReport`. See 2e5c8aed5e/modules/lm-coursier/src/main/scala/lmcoursier/internal/SbtUpdateReport.scala (L346-L351)

Since the stamps are empty, this caused `update` not to invalidate when the cache is completely missing. This works around the issue by checking if the file still exists. It also adds a warning that the file is missing.
2019-12-12 22:39:05 -05:00
Ethan Atkins a177c386c0 Add closeClassLoader setting
There have been a number of issues that have come up because of sbt
1.3.0 aggressively closing classloaders. While these issues have been
quite useful in helping us determine some issues related to classloader
lifecycle, we should give users the option to prevent sbt from closing
the classloaders.

I also noticed that the classloader-cache/spark test has been
occasionally segfaulting on travis so I disable classloader closing in
that test.
2019-12-12 17:07:40 -08:00
eugene yokota cba7442618
Merge pull request #5295 from eed3si9n/wip/new
Fixes sbt new by restoring the terminal
2019-12-11 18:24:20 -05:00
eugene yokota c03d70113c
Merge pull request #5289 from eatkins/temporary-directories
Do not use temporary directories in java.io.tmpdir
2019-12-11 13:08:23 -05:00
Eugene Yokota 1ef83e9140 Fixes sbt new by restoring the terminal
Fixes https://github.com/sbt/sbt/issues/5063

This fixes "sbt new" on Ubuntu by restoring the terminal state after supershell querying for the terminal width.
2019-12-11 13:05:20 -05:00
eugene yokota 8178673869
Merge pull request #5287 from eatkins/lint-excludes
Add onLoad and onUnload to project lint excludes
2019-12-10 18:59:42 -05:00
Ethan Atkins 283d486796 Do not use temporary directories in java.io.tmpdir
sbt should not by default create files in the location specified by
java.io.tmpdir (which is the default behavior of apis like
IO.createTemporaryDirectory or Files.createTempFile) because they have a
tendency to leak and it also isn't even guaranteed that the user has
write permissions there (though this is unlikely). Doing so creates the
possibility for leaks

I git grepped for `createTemp` and found these apis. After this change,
the files created by sbt should largely be localized to the project and
sbt global base directories.
2019-12-10 15:05:36 -08:00
eugene yokota f0d1e075db
Merge pull request #5288 from eatkins/shutdown-hook-type-annotation
Add type annotation for shutdown hooks
2019-12-10 15:22:48 -05:00
eugene yokota c46c17b92d
Merge pull request #5278 from hvesalai/develop
No supershell for Emacs and other color supporting dumb terminals
2019-12-10 15:21:44 -05:00
Ethan Atkins 38a56358dc Add type annotation for shutdown hooks
Intellij couldn't handle this without an annotation.
2019-12-10 10:37:52 -08:00
Ethan Atkins cc09294cf3 Add onLoad and onUnload to project lint excludes 2019-12-10 10:24:02 -08:00
Heikki Vesalainen 9e72b1c520 No supershell for Emacs or other dumb terminals that support color 2019-12-10 18:14:19 +00:00
Eugene Yokota 93f1f5464c sbt-giter8-resolver 0.12.0 2019-12-09 01:17:38 -05:00
Ethan Atkins 8518c4b4fd Place scalatest framework jar in its own classloader
Closing the ManagedClassLoader generated by test can cause nonlocal
effects because the jdk shares some JarFile resources across multiple
URLClassLoaders. As a result, if one classloader is trying to load a
resource and the classloader is closed, it might cause the resource
loading to fail (see https://github.com/sbt/sbt/issues/5262). This can
be fixed by moving the scalatest framework jar (and its dependencies)
into an additional classloader layer that sits between the scala library
loader and the rest of the test dependencies.

In addition to adding the new layer, I reworked the
ReverseLookupClassLoader to use its dependent classloader to find
resources that may below it in the classloading hierarchy rather than
constructing an entirely new classloader for resources.

After this change, I was able to run test in the repro project:
https://github.com/rjmac/sbt-5262 1000 times with no failures. Note that
the repro is sensitive to the jdk used. I could not reproduce with jdk11
but I could typically induce a failure within 20 or so runs with jdk8.

I benchmarked this change with
https://github.com/eatkins/scala-build-watch-performance and performance
was roughly the same as 1.3.4 with turbo mode and about 200-250ms faster
in non-turbo mode (which can be explained by the time to load the
scalatest classes).
2019-12-06 11:41:44 -08:00
Guillaume Martres 437950266f Give a more precise type to mkIvyConfiguration
This makes it possible to do mkIvyConfiguration.value.withXXX(...) for
all the methods in InlineIvyConfiguration. (I need this to remove
inter-project resolvers when fetching dotty from sbt-dotty to avoid
accidentally fetching a local project in the build of dotty itself).
2019-12-05 18:14:56 +01:00
Ethan Atkins 1438b79378 Make ZombieClassLoader thread safe
The previous implementation of ZombieClassLoader was not thread safe.
This caused problems because it is possible for the ManagedClassLoader
in test to leak into the coursier thread pool if the test uses bouncy
castle apis. Unfortunately, these apis seem to in some cases assign
static variables using the Thread context class loader. Because the
bouncycastle apis are implemented by the jdk, they are found in the
system classloader and thus the static references leak out of the test
context.

I had a local repro of https://github.com/sbt/sbt/issues/5249 that is
fixed by this change.
2019-12-03 18:53:40 -08:00
Ethan Atkins 53788ba356 Support input tasks in cross (+) command 2019-12-03 10:47:15 -08:00
Ethan Atkins 0cbbee4418 Don't import fields from local variables
I found it hard to reason about where certain local variables, like
currentRef, were coming from. I also changed 'x' to 'extracted' in a few
places for clarity as well.
2019-12-03 10:45:52 -08:00
Ethan Atkins abb3f61ff1
Merge branch 'develop' into background-jobs 2019-12-02 08:58:24 -08:00
Ethan Atkins 1a7d6a84f5
Merge branch 'develop' into state-transform 2019-12-02 08:03:46 -08:00
eugene yokota 56af617908
Merge pull request #5258 from eatkins/lint
Make minor improvements to project setting linting
2019-11-30 23:29:26 -05:00
Ethan Atkins 73a196798f Move background job service directory location
Rather than putting the background job temporary files in whatever
java.io.tmpdir points to, this commit moves the files into a
subdirectory of target in the project root directory.

To make the directory configurable via settings, I had to move the
declaration of the bgJobService setting later in the project
initialization process. I don't think this should matter because
background jobs shouldn't be created until after the project has loaded
all of its settings..
2019-11-30 15:20:00 -08:00
Ethan Atkins 73edc8d4ff Use anonymous function instead of Runnable 2019-11-30 15:20:00 -08:00
Ethan Atkins 7426ae520c Fix background job shutdown
When a user calls sbt exit and there is an active background job, sbt
may not exit cleanly. This was primarily because the
background job service shutdown method depended on the
StandardMain.executionContext which was closed before the background job
service was shutdown. This was fixable by reordering the resource
closing in StandardMain.runManaged.
2019-11-30 15:20:00 -08:00
Ethan Atkins 8d26bc73b4 Shutdown background job on error
When running a main method, if the user inputs ctrl+c then the `run`
task will exit but the main method is not interrupted so it continues
running even once sbt has returned to the shell. If the main method is a
webserver, this prevents run from ever starting again on a fixed port.
To fix this, we can modify the waitForTry method to stop the job if an
exception is thrown (ctrl+c leads to an interrupted exception being
thrown by waitFor).

I rework the BackgroundJobService so that the default implementation of
waitForTry is now usable and no longer needs to be overridden. The side
effect of this change is that waitFor may now throw an exception. Within
sbt, waitFor was only used in one place and I reworked it to use
waitForTry instead. This could theoretically break a downstream user who
relied on waitFor not throwing an exception but I suspect that there
aren't many users of this api, if any at all.
2019-11-30 15:20:00 -08:00
Ethan Atkins a83c280db1 Improve StateTransform
The StateTransform class introduced in
9cdeb7120e did not cleanly integrate with
logic for transforming the state using the `transformState` task
attribute. For one thing, the state transform was only applied if the
root node returned StateTransform but no transformation would occur if a
task had a dependency that returned StateTransform. Moreover, the
transformation would override all other transformations that may have
occurred during task evaluation.

This commit updates StateTransform to act very similarly to the
transformState attribute. Instead of wrapping a `State` instance, it now
wraps a transformation function from `State => State`. This function
can be applied on top of or prior to the other transformations via the
`transformState`.

For binary compatibility with 1.3.0, I had to add the stateProxy
function as a constructor parameter in order to implement the `state`
method. The proxy function will generally throw an exception unless the
legacy api is used. To avoid that, I private[sbt]'d the legacy api so
that binary compatibility is preserved but any builds targeting > 1.4.x
will be required to use the new api.

Unfortunately I couldn't private[sbt] StateTransform.apply(state: State)
because mima interpreted it as a method type change becuase I added
StateTransform.apply(transform: State => State). This may be a mima bug
given that StateTransform.apply(state: State) would be jvm public even
when private[sbt], but I figured it was quite unlikely that any users
were using this method anyway since it was incorrectly implemented in
1.3.0 to return `state` instead of `new StateTransform(state)`.
2019-11-30 15:06:34 -08:00
Ethan Atkins 3bb847fc72 Allow lintUnusedKeys to be disabled
The linting can take a while for large projects because `Def.compiled`
scales in the number of settings. Even for small projects (i.e. scripted
tests), it takes about 50 ms on my computer. This doesn't change the
current behavior because the default value is true.
2019-11-30 15:00:38 -08:00
Ethan Atkins 805fa002a7 Only print unused setting warning if there are any 2019-11-30 15:00:38 -08:00
Ethan Atkins 094d730b06 Bump scalafmt 2019-11-30 14:57:20 -08:00
Jason Pickens 71bc3876d9
Scope compiler bridge to consoleProject 2019-11-28 20:29:31 +13:00
eugene yokota c45e991c6b
Merge pull request #5229 from eed3si9n/wip/addPluginSbtFile
addPluginSbtFile command fixes
2019-11-21 17:02:02 -05:00
Frank S. Thomas 16860b5273 Include description and homepage in ivy.xml files
This PR includes the values of the `description` and `homepage`
settings into the `ivy.xml` files generated by the `makeIvyXml`
task. It restores the behaviour of sbt 1.2.8 and if `useCoursier`
is set to `false`.

Two things are changed in this PR:
 * `IvyXml.content` now adds the `homepage` attribute to the
   `description` element if `project.info.homePage` is not empty.
 * `CoursierInputsTasks.coursierProject0` now fills the previous
   empty `CProject.info` field with the description and homepage.

Closes: #5234
2019-11-16 20:18:42 +01:00
Eugene Yokota 033601c393 addPluginSbtFile command fixes
Ref #4211
Fixes #4395
Fixes #4600

This is a reimplementation of `--addPluginSbtFile`. #4211 implemented the command to load extra `*.sbt` files as part of the global plugin subproject. That had the unwanted side effects of not working when `.sbt/1.0/plugins` directory does not exist. This changes the strategy to load the `*.sbt` files as part of the meta build.

```
$ sbt -Dsbt.global.base=/tmp/hello/global --addPluginSbtFile=/tmp/plugins/plugin.sbt
[info] Loading settings for project hello-build from plugin.sbt ...
[info] Loading project definition from /private/tmp/hello/project
sbt:hello> plugins
In file:/private/tmp/hello/
	sbt.plugins.IvyPlugin: enabled in root
	sbt.plugins.JvmPlugin: enabled in root
	sbt.plugins.CorePlugin: enabled in root
	sbt.ScriptedPlugin
	sbt.plugins.SbtPlugin
	sbt.plugins.SemanticdbPlugin: enabled in root
	sbt.plugins.JUnitXmlReportPlugin: enabled in root
	sbt.plugins.Giter8TemplatePlugin: enabled in root
	sbtvimquit.VimquitPlugin: enabled in root
```
2019-11-10 20:03:09 -05:00
Samvel Abrahamyan ff75a21d4f Sleep the current thread when we need to retry background job shutdown 2019-11-05 14:54:55 +01:00
eugene yokota e17c64dfb6
Merge pull request #5153 from eed3si9n/wip/lint
build linting to warn on unused settings during reload
2019-10-30 11:36:43 -04:00
Filipe Regadas 66da2f5926
Merge branch 'develop' into fix/5110 2019-10-19 15:27:34 +01:00
Filipe Regadas 562eae2bff
Add explicit return type to plugin settings 2019-10-19 09:38:54 +01:00
Filipe Regadas d49ced04da
Bump semanticdbVersio to 4.2.3 2019-10-19 09:09:36 +01:00
Filipe Regadas 46b6ad0171
Bump semanticdbVersio to 4.2.4 2019-10-18 21:39:13 +01:00
Filipe Regadas 0ef5b578f8
Fix MiMa 2019-10-18 18:39:39 +01:00
Filipe Regadas a451200bad
Fix #5110: allow semanticdbVersion override 2019-10-18 16:48:36 +01:00
Josh Soref c7bf1a37f2
Remove excess quotation mark 2019-10-17 14:19:20 -04:00
Ethan Atkins d698d6dcdd Don't overwrite nio build settings with injected settings
The current injection of the new nio keys will overwrite any definitions
of those keys in a build source. This is undesirable. The fix is to
create a mapping of scoped keys to settings and for each inject setting
key, if there is a previous key, put that definition after the injected
definition so that it can override it.
2019-10-08 09:47:59 -07:00
Ethan Atkins d12bb2d71e Shutdown progress thread when there are no tasks
It is still possible for progress threads to leak so shut them down if
there are no active tasks. The report0 method will start up a new thread
if a task is added.
2019-10-07 09:43:59 -07:00
Ethan Atkins 6559c3a06d Use only one progress thread during task evaluation
In some circumstances, sbt would generate a number of task progress
threads that could run concurrently. The issue was that the TaskProgress
could be shared by multiple EvaluateTaskConfigs if a dynamic task was
used. This was problematic because when a dynamic task completed, it
might call afterAllCompleted which would stop the progress thread. There
also was a race condition because multiple threads calling initial could
theoretically have created a new progress thread which would cause a
resource leak.

To fix this, we modify the shared task progress so that the `stop()`
method is a no-op. This should prevent dynamic tasks from stopping the
progress thread. We also defer the creation of the task thread until
there is at least one active task. This prevents a thread from being
created in the shell.

The motivation for this change was that I found that sometimes there was
a leaked progress thread that would make the shell not really work for
me because the progress thread would overwrite the shell prompt. This
change fixes that behavior and I was able to validate with jstack that
there was consistently either one or zero task progress threads at a
time (zero in the shell, one when tasks were actually running).
2019-10-07 09:43:59 -07:00
Ethan Atkins 367461e586 Use logger rather than ConsoleOut for TaskTimings
When running sbt -Dtask.timings=true, the task timings get printed to
the console which can overwrite the shell prompt. When we use a logger,
the timing lines are correctly separated from the prompt lines.
2019-10-07 09:43:59 -07:00
Ethan Atkins ae84e162ad Limit scripted page numbers
The completions were generating page numbers that didn't make sense if
there were a small number of scripted tests. For example, suppose that
there were only two tests defined, it would generate *1of3 *2of3 and
*3of3 completions even though there weren't even three tests.
2019-10-06 14:07:30 -07:00
Ethan Atkins 9dff18d736 Fix scripted parser crash
In a local progress, I was able to induce a crash in tab completions
because the group key did not exist in pairMap.
2019-10-06 14:07:30 -07:00
Ethan Atkins 5d8b94de55 Clean ivy resolution cache before regular clean
The way clean was implemented, it was running `clean`, `ivyModule` and
`streams` concurrently. This was problematic because clean could blow
away files needed by `ivyModule` and `streams`. To fix this, move the
cleanCachedResolutionCache into a separate task and run that before the
normal clean.

Should fix https://github.com/sbt/sbt/issues/5067.
2019-10-05 16:42:16 -07:00
Eugene Yokota 460d1f5aa7 Rename to lintUnused for clarification
Address other review comments
2019-10-04 09:04:43 -04:00