Refactored assembly to use in-memory model instead of IO model

[fnqista]

The advantage of this code is streaming IO, with minimal reliance on disk speed. References to files and jar entries are stored as () => InputStream. Unzip to disk is gone, which is slow on some systems/hard drives.

Everything retained:

• all functionalities (shade, merge, caching)
• reproducible build (entires built in a specific order, jar entry timestamps identical for all entries, hash consistent)

The following were new:

• No unzip
• I cannot rename directories, that is the advantage of the unzip function of the current implementation (since files are unzipped, parent renames will reflect automatically to children - I cannot do this efficiently with this implementation). For this implementation I can only rename files. If there is a project file that conflicts, the project name is appended (i.e. LICENSE_foo), where in the current implementation the folder named LICENSE is renamed instead. For dependencies, the old behavior was maintained (i.e.LICENSE_scala-library-2.13.3). Tests were updated to reflect this change
• Corrected the reporting of file merge counts
• I additionally check for conflicts of files with directories that have the same name and signal an error, which the current implementation does not (it only checks for LICENSE conflicts. The current implementation will only throw this error when the jar is being created. New tests were added to reflect this change
• The option to create the jar in parallel which is faster! (1.5 seconds vs 5 seconds) The catch is the reproducible build and caching won't work if the jar is built in parallel. The new property is AssemblyOption.reproducibleBuild
• Dropped cacheUnzip property in AssemblyOptions
• Dropped excludedFiles in AssemblyOptions (?) this is not documented and seems unused
• Caching is now implemented by checking the last update timestamps of all included dependencies, plus some of the properties from AssemblyOptions, and if the output jar exists already
• Documentation updates
• Merge strategy retains the same structure, but the implementation (inputs and outputs) have changed, to support the classes added (Dependency, Project, Library, MergedEntry, JarEntry)
• Newline appending Input Stream (called AppendEofInputStream) added in AssemblyUtils to support MergeStrategy.concat with newlines

[eed3si9n]

@fnqista Thanks for this contribution. Looking forward to the perf improvements.
Since many Spark users use sbt-assembly and I am guessing that they use a lot of JARs, I wonder if we should do some before and after comparison to show case the difference in release notes etc.

[fnqista]

@fnqista Thanks for this contribution. Looking forward to the perf improvements. Since many Spark users use sbt-assembly and I am guessing that they use a lot of JARs, I wonder if we should do some before and after comparison to show case the difference in release notes etc.

When the computer is fast, like in my home computer which was upgraded recently, the difference between the old and new implementation is not that pronounced:

System: AMD Ryzen 5900X 12c/24t with NVME Gen4 drive (7000 MB/sec read)
Command: sbt clean assembly
Project: assembly jar ~90MB in size

• Old implementation
  [success] Total time: 22 s, completed Apr 16, 2022, 10:04:32 PM

• New implementation
  [success] Total time: 20 s, completed Apr 16, 2022, 10:13:26 PM

However in my work computer, I have pre-tested before and something that takes 15 minutes in the old implementation only takes 29 seconds now. I will gather evidence when I am back at work after the Easter.

[eed3si9n]

Here's a result from assembling a fresh Play app on my old mac laptop

sbt-assembly	assembly (median)	assembly (max out of 5)
Baseline (1.2.0)	10s	12s
This PR (default)	7s	9s
This PR (ThisBuild / assemblyRepeatableBuild := false)	5s	5s

[fnqista]

@eed3si9n Release notes draft:
https://gist.github.com/fnqista/56fb6c5957302ba4af32c6c2b7dcb7c8

[fnqista]

@eed3si9n The method signature of MergeStrategy.merge also has changed:

merge(conflicts: Vector[Dependency]): Either[String, Vector[JarEntry]] whereas the old one just uses standard primitive and collection types.

This is exposed to the user because they can create custom merge strategies. Shall we use contraband for these to at least guarantee some level of backward compatibility?

[fnqista]

@eed3si9n
Major PR updates:

• As agreed, I changed the implementation of MergeStrategy as per your suggestion:

val first: MergeStrategy = MergeStrategy("First") { conflicts =>
  Right(Vector(JarEntry(conflicts.head.target, conflicts.head.stream)))
}

• For custom merge strategies (user-provided):

ThisBuild / assemblyMergeStrategy := {
...
case _ => CustomMergeStrategy("First") { conflicts =>
  Right(Vector(JarEntry(conflicts.head.target, conflicts.head.stream)))
}

• I have to change the object for a custom merge strategy because:

  • Primarily to namespace them. It is easy for a user to create a custom merge strategy that is also named First, but properly namespacing them makes us distinguish each other when needed in both the code and in the logs.
  • To prevent future plugin contributors who will create/modify a built-in MergeStrategy in as a "custom" one, again breaking namespacing.
    You can browse the code under sbtassembly.MergeStrategy. I used a sealed trait that extends a Function1 plus two descendants to represent the namespacing. Companion objects are also created for them.

• And another one - if the user includes .class files in a built-in rename strategy, I throw an error now (well, really, return a Left), and inform them that they should use shadeRules instead. In 1.2.0 this was ignored but also silently a no-op, without informing the user. I think it is important to be transparent with the plugin behavior regarding this. Code is in MergeStrategy, plus a new test is added for this.

• I updated the README to clarify that a MergeStrategy doesn't only merge, it also transforms, such as in the case of a rename, where it probably matches only a single file. I think it is important that this is communicated to the users.

• I exposed JarEntry and CustomMergeStrategy as an auto import in AssemblyPlugin, since I have tried this myself and found out that this increases the user's convenience.

• I also created an implicit class conversion from String -> java.nio.file.Path since Path is used by JarEntry, again in AssemblyPlugin.

• I implemented the other review comment about test in assembly := {},, which now properly returns Unit, again in AssemblyPlugin.

• I type-aliased () => InputStream as LazyInputStream in Assembly

• I finally moved the rename logic inside the cache block too. Now, to check the cache, nothing is every really done except to build the Dependency to check if their timestamps have changed and to build a Map of paths to merge strategies. Before, we always process the renames before building the merge strategy map/checking the cache. I don't know what I was thinking before, but it seems that we don't always need to process renames before checking the cache (I hope I am right). Anyway, I tested it quite a few times and it seems solid. Code is in Assembly.

• I created the MergeStrategy.isBuiltIn property to namespace custom ones from, well, built-in ones - and make sure it is included in the cache checking. Also in Assembly.

• Minor code and test updates due to the above changes.

[eed3si9n]

LGTM

[fnqista]

@eed3si9n
So, me and a few colleagues tested this at work over the past week and we found some bugs, all of them related to custom merge strategies.

• The Custom rename strategy method signature must be changed so it can only return a String representing the renamed target, instead of Vector[JarEntry], to prevent users from corrupting the result, when they should only be renaming the target. So the correct signature should be CustomStrategy.rename(f: Dependency => String) instead of the old CustomStrategy.rename(f: Vector[Dependency] => Vector[Either[String, JarEntry]])

• Renames should really be done before evaluating the cache, which I broke in my previous commit. Otherwise, if we do it after checking the cache, the assembly might not execute when it should, in cases where the rename might cause another conflict.

• Custom merge strategies, other than rename, breaks the caching. In this PR, the only way to check if the merge strategies have changed is by creating a Map[String, String] representing the matched target and the merge strategy name. This is mostly ok for built-in strategies and the custom rename because their code/logic doesn't change. But with custom merge strategies, the name means nothing when caching since we cannot predict what the user will do, thereby it breaks caching again by the code thinking that nothing has changed when in fact the user updated the merge strategy logic and assembly should re-run.

I have prepared some fixes but I am still testing them, hopefully I can make a new push within a couple of days.

EDIT: Clarifying point no. 3 about custom renames being OK - If we fix no. 2 (rename before caching), then custom renames will happen before the cache.

[fnqista]

@eed3si9n So, me and a few colleagues tested this at work over the past week and we found some bugs, all of them related to custom merge strategies.

• The Custom rename strategy method signature must be changed so it can only return a String representing the renamed target, instead of Vector[JarEntry], to prevent users from corrupting the result, when they should only be renaming the target. So the correct signature should be CustomStrategy.rename(f: Dependency => String) instead of the old CustomStrategy.rename(f: Vector[Dependency] => Vector[Either[String, JarEntry]])
• Renames should really be done before evaluating the cache, which I broke in my previous commit. Otherwise, if we do it after checking the cache, the assembly might not execute when it should, in cases where the rename might cause another conflict.
• Custom merge strategies, other than rename, breaks the caching. In this PR, the only way to check if the merge strategies have changed is by creating a Map[String, String] representing the matched target and the merge strategy name. This is mostly ok for built-in strategies and the custom rename because their code/logic doesn't change. But with custom merge strategies, the name means nothing when caching since we cannot predict what the user will do, thereby it breaks caching again by the code thinking that nothing has changed when in fact the user updated the merge strategy logic and assembly should re-run.

I have prepared some fixes but I am still testing them, hopefully I can make a new push within a couple of days.

EDIT: Clarifying point no. 3 about custom renames being OK - If we fix no. 2 (rename before caching), then custom renames will happen before the cache.

Fixes committed for the above...hopefully the last one.

EDIT: Also updated draft release notes: https://gist.github.com/fnqista/56fb6c5957302ba4af32c6c2b7dcb7c8

[fnqista]

@eed3si9n Can you review the pending changes and then create an RC1 candidate when you have time?

[eed3si9n]

If the test passes I am good here.