fix #233 add append and exclude rules to assembly #309

rockjam · 2018-05-02T16:35:23Z

This PR introduces ability to define rules for duplicated files in assembly. Currently, there are two rules: Append and Exclude

TODO:

Tests on append/exclude rules
Update docs about duplication rules

rockjam · 2018-05-02T16:38:38Z

Special thanks to @alexarchambault . Code mostly based on assembly code from coursier

rockjam · 2018-05-02T16:38:49Z

@mlerner Want to join the review?

rockjam · 2018-05-02T16:47:54Z

main/src/mill/modules/Jvm.scala


+object Assembly {


Is it right place to have this rules?
For people who want to define their own rules it will be:

import mill.modules.Assembly._ val myRules = Seq(Rule.Append("foo.conf"), Rule.Exclude("bar.txt"))

rockjam · 2018-05-02T19:55:05Z

I found on use-case that won't work in the current implementation. Since we make assembly in two steps: upstreamAssembly and then assembly itself, in assembly step we don't keep track on files that were already written in upstreamAssembly. So for instance, if reference.conf is present in module we are making assembly for, it will rewrite reference.conf written(and concatenated) in upstreamAssembly. I will later write test for this case and try to fix it

rockjam · 2018-05-13T23:34:16Z

scalalib/src/mill/scalalib/JavaModule.scala

@@ -108,6 +108,8 @@ trait JavaModule extends mill.Module with TaskModule { outer =>
    }
  }

+  def assemblyRules: T[Seq[Assembly.Rule]] = T { Assembly.defaultRules }


Do we need to keep it as Target, or we can go with plain def here? Cause if we can go with def, we can remove serialization code for Assembly.Rule

I don't have any real preference. If we go with def, we could also include rules that are not serializable, e.g. containing lambdas. I leave that up to you

If we make it def, instead of T, and the users change them while working on their build, will it reevaluate for sure? I tend to think that it will, because we invalidate caches on build change

rockjam · 2018-05-13T23:35:18Z

PR is ready for review. I will add section to docs if the API for assembly rules is fine.

lihaoyi · 2018-05-14T03:36:45Z

scalalib/test/src/mill/scalalib/HelloWorldTests.scala

+    object core extends HelloWorldModuleWithMain
+  }
+
+  val akkaHttpDeps = Agg(ivy"com.typesafe.akka::akka-http:10.0.13")


We really need to break up the tests in this file :/ Maybe not in this PR, but if you could do that in a subsequent one that would be great

Agree with you

lihaoyi · 2018-05-14T03:42:46Z

main/src/mill/modules/Jvm.scala

+        case Some(_:Assembly.Rule.Append) =>
+          append()
+        case Some(_:Assembly.Rule.Exclude) =>
+          ctx.log.info(s"Excluding entry: ${mapping}")


I think we can skip the logging here; I find the logging from SBT's assembly task more annoying than useful

lihaoyi · 2018-05-14T03:44:20Z

main/src/mill/modules/Jvm.scala

@@ -257,20 +261,52 @@ object Jvm {
    manifest.write(manifestOut)
    manifestOut.close()

-    def isSignatureFile(mapping: String): Boolean =
-      Set("sf", "rsa", "dsa").exists(ext => mapping.toLowerCase.endsWith(s".$ext"))


Should we maintain this behavior in the new default assemblyRules? IIRC we added this because these files were causing problems for someone (there's an issue)

Yep, I added it in default rules actually, right here: https://github.com/lihaoyi/mill/pull/309/files#diff-8987430aa3e4d7f9373fa4b3c3d5f2d0R13

lihaoyi · 2018-05-14T03:44:46Z

main/src/mill/modules/Jvm.scala

@@ -295,25 +331,45 @@ object Jvm {
    PathRef(output)
  }

+  sealed trait AssemblyEntry {
+    def mapping: String
+    def is: InputStream


Let's call this inputStream rather than .is, for better greppability =)

lihaoyi · 2018-05-14T03:52:01Z

main/src/mill/modules/Jvm.scala

    }
+
+    concatEntries.foreach { case (mapping, entries) =>


Would it be possible to unify the Some(_: Assembly.Rule.Append) logic with the None logic, rather than having them go down a totally different code path? Apart from complexity, I think this would also re-arrange the order of files in the jar, which while harmless is unnecessary and makes debugging harder (e.g. when you jar tf two jars to compare contents)

Perhaps we could make the None case go through concatEntries as well, just over-writing a key rather than appending to it. If we then make concatEntries a LinkedHashMap, that should let us preserve the original ordering of keys

The thing is - we process concatEntries outside the for comprehension that goes through all the classpath entries. If we process both "write once" and append outside for that means we have to keep all the files in memory, before we write them. I guess we want to avoid this, right?

@rockjam aren't we're only keeping the list of files in concatEntries, and not the file contents themselves, since each AssemblyEntry is just a thin wrapper that allows you to open an InputStream on-demand? As long as we're not keeping their byte contents, and not holding open all their file-handles at the same time, I think keeping the wrappers in memory should be acceptable. e.g. Ammonite only has 16710 file within it's assembly, and even an order of magnitude more

Or we could just leave this as is; I guess a bit of reshuffling isn't the end of the world either.

Ah, sure. I forgot that. It was before this PR - we opened input streams in a for loop, but now we don't have to. Yes, that sounds reasonable.

lihaoyi · 2018-05-14T03:53:03Z

@rockjam left a review

lihaoyi · 2018-05-14T16:24:49Z

main/src/mill/modules/Jvm.scala

+        case None =>
+          val path = zipFs.getPath(mapping)
+          if(!excludePatterns.exists(_(mapping))) {
+            if(appendPatterns.exists(_(mapping))) {


I think this logic is somewhat incorrect; we appear to be checking both for AssemblyEntry#mapping == Entry#mapping, and also for AssemblyEntry#mapping.asPredicate().test(Entry#mapping). We should treat AssemblyEntry#mapping properly as a regex and only do one test. Otherwise files that don't match the regex, but do match the literal path of the regex, will be unexpected matched (e.g. for a regex hello [world] which should match hello w or hello d but not the entire string hello [world])

lihaoyi · 2018-05-14T16:31:32Z

scalalib/test/src/mill/scalalib/HelloWorldTests.scala

+    object core extends HelloWorldModuleWithMain {
+      def moduleDeps = Seq(model)
+
+      def assemblyRules = T { Seq(Assembly.Rule.Exclude("reference.conf")) }


Could we have one unit test for the behavior of anchored regexes? e.g. making sure^reference.conf matches only reference.conf and not foo/reference.conf, and that reference.conf$ matches only foo/reference.conf but not foo/reference.confetti

lihaoyi · 2018-05-14T17:02:18Z

Whatever we end up doing, it's probably worth running some basic benchmarks using System.currentTimeMillis to e.g. assemble the Mill jar and make sure we didn't regress the speed

rockjam · 2018-05-30T23:37:32Z

@lihaoyi Updated PR according to your review. Also moved all that append and exclude logic from Jvm to Assembly object - seems to simplify createAssembly code a little bit.

lihaoyi · 2018-05-31T01:26:47Z

@rockjam looks good to me

…ssembly

…ther than T[Seq[_]]

rockjam · 2018-05-31T23:00:09Z

@lihaoyi By the way, made some primitive benchmarking. Did three runs for each version.

Before append rules:

9119 millis
6620 millis
6719 millis

With append rules PR:

9417 millis
7322 millis
7770 millis

So, there is some slowdown introduced by this PR. Do we keep it as is, or it's better to find a way to reduce this slowdown?

rockjam · 2018-05-31T23:10:25Z

Hm, travis shows build as failed, even though it's all steps are fine. Release steps are canceled, but it always happens, if I remember correctly.

lihaoyi · 2018-06-01T02:26:54Z

@rockjam those benchmark numbers look fine; a 10%-ish slowdown is still several times faster than SBT due to our layered jar-building.

Travis is probably just being flaky; feel free to merge

rockjam commented May 2, 2018

View reviewed changes

rockjam force-pushed the assembly-rules branch 3 times, most recently from 310cf99 to 2ead219 Compare May 9, 2018 18:03

rockjam force-pushed the assembly-rules branch 2 times, most recently from 2dfd893 to ab00a06 Compare May 13, 2018 23:19

rockjam commented May 13, 2018

View reviewed changes

rockjam changed the title ~~[WIP]: fix #233 add append and exclude rules to assembly~~ fix #233 add append and exclude rules to assembly May 13, 2018

lihaoyi reviewed May 14, 2018

View reviewed changes

lihaoyi mentioned this pull request May 19, 2018

Appveyor/Windows tests are flaky #341

Closed

rockjam force-pushed the assembly-rules branch from 9e6ccbc to 9261250 Compare May 29, 2018 23:20

rockjam added 5 commits June 1, 2018 01:01

fix com-lihaoyi#233 add append and exclude rules to assembly

8dc9def

handle existing files and concatenation when file already exists in a…

31949a6

…ssembly

add assembly tests for append rules

976f31f

tests for append patterns

3bb201f

tests for exclude patterns

59a61d6

rockjam added 6 commits June 1, 2018 01:01

make append algorithm use single map with fold over classpathIterator

335d110

move assembly rules logic to method

1ea2206

move grouping method to Assembly object, make assemblyRules Seq[_] ra…

d1ab971

…ther than T[Seq[_]]

add test cases for when there are no rules

f1f79e5

keep default parameter in createAssembly not to break CI

435ac29

add one more reference.conf entry to tests

d731f15

rockjam force-pushed the assembly-rules branch from 3119fd1 to d731f15 Compare May 31, 2018 22:01

rockjam merged commit baf2295 into com-lihaoyi:master Jun 1, 2018

rockjam deleted the assembly-rules branch June 1, 2018 08:18

lefou added this to the 0.2.3 milestone May 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #233 add append and exclude rules to assembly #309

fix #233 add append and exclude rules to assembly #309

rockjam commented May 2, 2018 •

edited

Loading

rockjam commented May 2, 2018

rockjam commented May 2, 2018

rockjam May 2, 2018

rockjam commented May 2, 2018

rockjam May 13, 2018

lihaoyi May 14, 2018

rockjam May 27, 2018

rockjam commented May 13, 2018

lihaoyi May 14, 2018

rockjam May 14, 2018 •

edited

Loading

lihaoyi May 14, 2018

lihaoyi May 14, 2018

rockjam May 14, 2018

lihaoyi May 14, 2018

lihaoyi May 14, 2018

rockjam May 14, 2018

lihaoyi May 14, 2018

rockjam May 14, 2018

lihaoyi commented May 14, 2018

lihaoyi May 14, 2018

lihaoyi May 14, 2018

lihaoyi commented May 14, 2018

rockjam commented May 30, 2018

lihaoyi commented May 31, 2018

rockjam commented May 31, 2018 •

edited

Loading

rockjam commented May 31, 2018

lihaoyi commented Jun 1, 2018

fix #233 add append and exclude rules to assembly #309

fix #233 add append and exclude rules to assembly #309

Conversation

rockjam commented May 2, 2018 • edited Loading

rockjam commented May 2, 2018

rockjam commented May 2, 2018

Choose a reason for hiding this comment

rockjam commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockjam commented May 13, 2018

Choose a reason for hiding this comment

rockjam May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi commented May 14, 2018

rockjam commented May 30, 2018

lihaoyi commented May 31, 2018

rockjam commented May 31, 2018 • edited Loading

rockjam commented May 31, 2018

lihaoyi commented Jun 1, 2018

rockjam commented May 2, 2018 •

edited

Loading

rockjam May 14, 2018 •

edited

Loading

rockjam commented May 31, 2018 •

edited

Loading