Change startup script name generation #1020

atrosinenko · 2017-08-28T09:24:56Z

This PR tries to lower the chance of generation of multiple scripts
with equal names in one archive. Fixes #1016.

Additionally, an attempt was made to improve the word splitting
for the script names, so, for example UITest becomes ui-test
and not u-i-test.

I had to edit ForwarderScripts.apply(...) as well but I'm not sure I did it the right way.

lightbend-cla-validator · 2017-08-28T09:24:56Z

Hi @atrosinenko,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

atrosinenko · 2017-08-28T11:47:30Z

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/ScriptUtils.scala

+      (qualifiedClassName, lowerCased.replace('.', '-'), lowerCased.split("\\.").last)
+    }
+
+    names.groupBy(_._3).toSeq.flatMap {


What is the preferred way to rephrase this?

The issue that _._3 is hard to read as this parameter has no name.
A better way would be

names.groupBy { case (_, _, foo) => foo }.toSeq.flatMap { ... }

Oh, I didn't thought about it. It really looks more readable.

atrosinenko · 2017-08-28T14:52:55Z

Strange, at least one of Travis failures looks like some misconfiguration.

muuki88

Thanks for your well crafted PR ( refactoring logic and unit tests ).

I think the logic is way to complex. You already have an amazingly simple idea in your code 😄
Instead of applying heuristics to hope for the best, we can actually be certain what name to use.

Create a tuple of (simpe name, fully qualified name)
Group by the simple name -> Map[simple name, List[fully qualified name]
Partition `(fully qualified names length == 1, fully qualified names length > 1)
Extend simple name and start from step 1 with the fully qualified names length > 1 partition

muuki88 · 2017-08-29T20:57:12Z

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/ScriptUtils.scala

+      (qualifiedClassName, lowerCased.replace('.', '-'), lowerCased.split("\\.").last)
+    }
+
+    names.groupBy(_._3).toSeq.flatMap {


The issue that _._3 is hard to read as this parameter has no name.
A better way would be

names.groupBy { case (_, _, foo) => foo }.toSeq.flatMap { ... }

atrosinenko · 2017-08-30T18:53:49Z

If you propose to extend the simpleNames just for the classes that generate conflicting script names without this, then it may introduce additional conflicts, something like

pkg1.Pkg1Test -> pkg-1-test
pkg1.Test     -> test -> pkg-1-test
pkg2.Test     -> test -> pkg-2-test

So it may be not so simple. On the other hand, when these heuristics fail, it does not render resulting zip completely unusable (though, it may became unusable in fully automatic setups). So there may be another option to drop the most nested numbering logic completely (not very good, does the other means lower the chance of failure to some negligible level?). Or apply some iterative algorithm.

Another problem, if we decide to iteratively extend the simpleName to not make it very long: consider the following classes:

com.company.feature1.core.MainClass
com.company.feature2.ui.MainClass

If we just extend it from right to left, then we probably end up with meaningless core-main-class and ui-main-class. In fact, we would prefer feature-1-main-class and feature-2-main-class. Another option may be to strip equal prefix from the left, then extend it as main-class -> feature-1-main-class -> feature-1-core-main-class.

muuki88 · 2017-08-30T19:19:16Z

If you propose to extend the simpleNames just for the classes that generate conflicting script names without this, then it may introduce additional conflicts.

This is true if we change the class name. In your example the camel cased name Pkg1Test is transformed to pkg1-test, which we shouldn't do. We should only transform . (separating packages) to - in the script name. The compiler then ensures that if we expand from right to left in the end every bash script is unique.

Another problem, if we decide to iteratively extend the simpleName to not make it very long

I'm willing to choose this trade off. I consider this rather an edge case as this could be fixed on an application level by renaming classes or moving it to another package. I know naming is one of the harder problems in software engineering, but users of your scripts will be happy to have meaningful script names instead of weird package names ;)

I first thought of adding a "resolve conflict strategy" as sbt-assembly does it, but I think it's an overkill as we need to document and maintain it.

Long story short: A recursive algorithm extending only the conflicting classes until all conflicts are resolved seems the most stable and simplest solution :)

atrosinenko · 2017-08-30T19:25:36Z

So should the lower casing algorithm be completely dropped? Previously, class names were splitted into words separated with - (but this failed with abbreviations), if I got it right.

muuki88 · 2017-08-30T19:42:13Z

Either this or we replace the dots with underscored. This would keep backwards compatibility and wouldn't generate toooo ugly script names.

lightbend-cla-validator · 2017-09-04T08:17:14Z

Hi @atrosinenko,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

atrosinenko · 2017-09-04T08:23:31Z

Please excuse me for overcomplicating everything once again, but here is another try: now heuristics are even worse :( but they should give more meaningful results (see tests). Based on your review I have to either try to simplify it to some informally provable form or drop it in favor of simpler prefix-based strategy.

muuki88 · 2017-09-04T19:38:56Z

Thanks for your time and dedication @atrosinenko :)
The results make sense (stripping common packages and keep script names short).

Still I think this in an edge case and we should rather add the ability to configure script names then a complicated algorithm that can't be explain in two sentences. Maintaining and explaining this will cost more time I fear ;)

What do you thing of this implementation?

  private case class MainClass(scriptName: String, fullyQualifiedClassName: String) {
    val parts: Array[String] = toLowerCase(fullyQualifiedClassName).split("\\.")
    def asTuple: (String, String) = (fullyQualifiedClassName, scriptName)
  }

  def getMainClasses(discoveredMainClasses: Seq[String]): Seq[(String, String)] = {
    val mainClasses = discoveredMainClasses.map { qualifiedClassName =>
      val lowerCased = toLowerCase(qualifiedClassName)
      val parts = lowerCased.split("\\.")
      MainClass(parts.last, qualifiedClassName)
    }
    disambiguateNames(mainClasses, 1).map(_.asTuple)
  }

  private def disambiguateNames(mainClasses: Seq[MainClass], expansionIndex: Int): Seq[MainClass] =
    if (mainClasses.isEmpty) {
      // end of recursion
      mainClasses
    } else {
      // find duplicates
      val (duplicates, uniques) = mainClasses
        .groupBy(_.scriptName)
        .partition {
          case (_, classes) => classes.length > 1
        }

      // expand names for duplicated script names
      val expandedScriptNames = for {
        (_, classes) <- duplicates
        extendedClasses <- classes.map(expandClassName(expansionIndex))
      } yield extendedClasses

      uniques.toSeq.flatMap(_._2) ++ disambiguateNames(expandedScriptNames.toSeq, expansionIndex + 1)
    }

  private def expandClassName(expansionIndex: Int)(mainClass: MainClass): MainClass = {
    val scriptName = mainClass.parts.takeRight(expansionIndex + 1).mkString("_")
    mainClass.copy(scriptName = scriptName)
  }

atrosinenko · 2017-09-17T12:10:43Z

Please excuse me for the delay and thank you for code advice. I have rewritten disambiguation logic once again, now heavily based on your proposal. Now it looks much simpler than from the second try. I decided to expand from the left after stripping common prefix instead of expanding from the right -- looks like it can give more meaningful distinction by features instead of some technical innermost packages (but I don't insist it is the most correct approach).

I had to manually rebase previous commits as well, so it may be worth to minimally glance on them.

atrosinenko · 2017-09-17T12:25:26Z

Oops, fixed the corner case of single main class (the common prefix included the class name itself) -- that didn't caught by the tests on Linux.
PS: The CLA signing bot suggested me to notify the maintainer that CLA is signed for already opened PRs. :)

muuki88 · 2017-09-17T12:25:40Z

Thanks @atrosinenko I'll try to take a look this week. I'll be away for 4 weeks on vacation then and will merge it after that :)

muuki88 · 2017-09-21T18:02:31Z

I'll be on vacation un 20.10. Thanks for your understanding if this can't be merged until then

muuki88

Thanks for all your patience and passion you put into this. Once again I'm requesting more simplification 😉

The main reasons is that the behaviour is non-obvious for the user. As a way forward I would suggest that we simply keep the disambiguateNames method and remove all the prettifying stuff. The next step would be a pull request (if you have time and motivation) and add a setting like

executableScriptNames: SettingKey[Map[String, String]]

which provides a way to customize the generated script names.

muuki88 · 2017-10-30T18:44:36Z

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/ScriptUtils.scala

+    */
+  def toLowerCase(qualifiedClassName: String): String = {
+    // suppose list is not very huge, so no need in tail recursion
+    def split(chars: List[Char]): List[Char] = chars match {


I still think this is too much for an edge case. Prettifying should be done by the user.
We could do that in a second pull request where we provide a setting that let's the user
configure executable script names for each main class.

muuki88 · 2017-10-30T18:44:53Z

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/ScriptUtils.scala

+
+  private[this] def disambiguateNames(
+                                       mainClasses: Seq[MainClass],
+                                       expansionIndex: Int


I like that parameter name 👍

muuki88 · 2017-10-30T18:48:46Z

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/ScriptUtils.scala

+      val parts = lowerCased.split("\\.")
+      MainClass(qualifiedClassName, parts)
+    }
+    val commonPrefixLength = mainClasses.map(_.parts.init.toList).reduce(commonPrefix).size


I would leave out every prettifying here. As mentioned below the way forward would be
a user facing setting where a mapping between mainClass and executable script name
can be configured.

muuki88 · 2017-10-30T18:49:20Z

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/ScriptUtils.scala

+      (fullyQualifiedClassName, scriptName(expansionIndex))
+  }
+
+  private[this] def disambiguateNames(


Well done implementation. This the bare minimum for these feature to work

This commit tries to lower the chance of generation of multiple scripts with equal names in one archive. Fixes #1016. Additionally, an attempt was made to improve the word splitting for the script names, so, for example "UITest" becomes "ui-test" and not "u-i-test".

atrosinenko · 2018-01-02T15:57:12Z

Please excuse me for huge delay. I have stripped some of complicated logic.

For now, I have left lowercasing logic as-is , but changed other beautifying to simply the following:

split lower-cased names by uniqueness of their last parts (simple names)
a) unique names replace by lower-cased simple names
b) non-unique names replace by lower cased fully qualified names with . replaced by -

I cannot prove right now that lower-casing of fully qualified class names is reversible (that is, cannot introduce name clashes), but names that differ just in letter case are already clashed on Windows. Supposing all lower-cased qualified names are distinct, this trivial disambiguation seems to not introduce clashes:

the part of resulting name after the last _ (if any) is generated from the simple class name, so no clashes for names from a) among them and with names from b) (even from the default package)
names from b) are all distinct because they are just lower-cased qualified names and we supposed they are all distinct :)

muuki88 · 2018-01-02T18:54:01Z

No need to apologise 😄 . I have to thank you for keeping up with all my change requests ❤️

The changes are looking maintainable (great docs, thank you 👍 ) and reasonable.
I have only a last request :) :) :)
Could you add a small section to the java app multiple apps documentation that the start scripts are disambiguated. Ideally with a couple of examples?

atrosinenko · 2018-01-03T14:31:16Z

I will try to make requested fixes soon.

I spotted that name lower-casing logic can introduce clashes, at least in some corner cases like Test1Class and Test1class will both end up in test-1-class (for this particular case there is already clash for case-insensitive file system). I want to show warnings in such cases. I also want to slightly factor out common logic from BatStartScriptPlugin and BashStartScriptPlugin, so it will be easier to implement this warning.

atrosinenko · 2018-01-03T14:44:44Z

But since this logic just inserts some -s and changes characters to lower case, it seems to introduce new clashes only if there are already some "case-insensitive clashes" like pkg1.A and pkg1.a (or class names contain a - :) ). Meanwhile, subsequent disambiguation logic can introduce new clashes, if class names contain a _, but it is too many coincidences.

muuki88 · 2018-01-04T10:20:21Z

But since this logic just inserts some -s and changes characters to lower case, it seems to introduce new clashes only if there are already some "case-insensitive clashes" like pkg1.A and pkg1.a (or class names contain a - :) ).

True, and you are right that these are too many coincidences ( or rather obscure class namings ).
So keeping things simple is a better option :)

Factor out common code from `BashStartScriptPlugin` and `BatStartScriptPlugin`

atrosinenko · 2018-01-05T12:27:04Z

I have added a note to the docs about name disambiguation and added a warning on script name conflicts (although, I did not test the warning itself at all, only added a test on duplication detection). When adding a warning, I refactored BashStartScriptPlugin and BatStartScriptPlugin and I hope they still work :) -- ^validate and ^validateOSX pass on my Ubuntu, but I did not tested that these scripts works. I can test it in real life setting next week.

muuki88

Nice refactoring! I like the shape this becomes. The documentation is also pretty neat.

During the refactoring you added an interesting type. I didn't quite get it, but if necessary this is good to go.

Thanks for you dedication ❤️

muuki88 · 2018-01-05T15:25:10Z

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/CommonStartScriptGenerator.scala

+    IO.write(script, scriptContent)
+    // TODO - Better control over this!
+    if (makeScriptsExecutable)
+      script.setExecutable(true)


You you simply do

script.setExecutable(makeScriptsExecutable)

Yes, assuming script.setExecutable(false) is no-op and works well on Windows :) Meanwhile, does building zips with executable bits set on Windows works at all?

Of course, I don't hesitate to simplify this line, if it is possible.

Good point. No idea what happens on windows. We can leave this as is and should put a comment in there that setExecutable may behave differently on different platforms.

muuki88 · 2018-01-05T15:25:16Z

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/CommonStartScriptGenerator.scala

+
+        IO.write(file, scriptContent)
+        if (makeScriptsExecutable)
+          file.setExecutable(true)


You you simply do

script.setExecutable(makeScriptsExecutable)

muuki88 · 2018-01-05T15:27:36Z

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/CommonStartScriptGenerator.scala

+    def withScriptName(scriptName: String): SpecializedScriptConfig
+  }
+
+  protected[this] type SpecializedScriptConfig <: ScriptConfig


What's the purpose of this type?

Some template methods (or how they call it?) can depend on fields from the specialized type. Particularly, it was the way I allow Replacements.appDefines() to access configLocation and extraDefines from specialized config.

muuki88 · 2018-01-05T15:27:48Z

src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/CommonStartScriptGenerator.scala

+                                                      mainClasses: Seq[String],
+                                                      config: SpecializedScriptConfig): Seq[(String, String)]
+
+  protected[this] trait ScriptConfig {


I like binding the script config type to the actual instance.

muuki88 · 2018-01-05T15:28:25Z

src/sphinx/archetypes/java_app/index.rst

@@ -223,6 +223,48 @@ For two main classes ``com.example.FooMain`` and ``com.example.BarMain`` ``sbt s

 Now you can package your application as usual, but with multiple start scripts.

+A note on script names


well done 👍

atrosinenko · 2018-01-05T16:06:21Z

Forgot to say, there were some hardcoded bin/s and scripts/s in the paths for main (and, probably, forwarder) scripts, sometimes script was saved to scripts/ and the second element of tuple referenced bin/. I'm not sure I got it right, but for now I simply changed it to scriptTargetFolder (== "bin/") everywhere.

muuki88 · 2018-01-07T14:46:16Z

All tests seem to pass, so the bin refactoring worked ( and is quite nice ).
As mentioned in the comment above we can leave the setExecutable code as is, but should put an inline comment why.

Then this is good to merge ❤️

atrosinenko · 2018-01-08T15:49:09Z

I have added an explanation for the SpecializedScriptConfig type and changed executable bit logic to unconditionally setting some particular value, as you proposed first, because it is simpler and, after all, it is worth to not only set an executable bit on shell script, but also to explicitly unset it on .bat script. I don't know how this behaves on Windows, but AFAIK the shell scripts were previously generated when building on Windows too, weren't they? If so, this should not error out, but will fail to set that bit if I get the documentation on setExecutable(...) right (it can throw an exception, but it is when SecurityManager disallows an operation).

muuki88 · 2018-01-08T19:08:08Z

Nice nice nice 😎

I retriggered the one build that timed-out. If this is green, then we can merge this.
I'm very happy about the output. Thanks for all your time and patience ☺️

atrosinenko · 2018-01-08T19:36:20Z

Thanks for all your time and assistance! :)

atrosinenko commented Aug 28, 2017

View reviewed changes

muuki88 added rpm universal Zip, tar.gz, tgz and bash issues and removed rpm labels Aug 29, 2017

muuki88 mentioned this pull request Aug 29, 2017

Add validatePackageConfiguration task #1026

Closed

muuki88 requested changes Aug 29, 2017

View reviewed changes

sbt deleted a comment Sep 4, 2017

muuki88 requested changes Oct 30, 2017

View reviewed changes

atrosinenko added 6 commits January 2, 2018 16:56

Heuristics for disambiguating names: second try

e4dd547

Implement simpler disambiguating heuristics

5715d02

Fix indentation

b1a8e0c

Fix for the case of single main class

4eb41dc

Drop some beautifying logic

a3d6576

Fix formatting

ae7ba07

atrosinenko added 3 commits January 5, 2018 13:56

Add note to the documentation on script name generation

6ac929e

Refactor Universal JavaAppPackaging

2514626

Factor out common code from `BashStartScriptPlugin` and `BatStartScriptPlugin`

Show warning when script name collision is detected

8a02f3d

Fix docs formatting

91cb97e

muuki88 requested changes Jan 5, 2018

View reviewed changes

Fix comments and simplify executable bit logic

f7c07fd

muuki88 approved these changes Jan 8, 2018

View reviewed changes

muuki88 merged commit e0b0a13 into sbt:master Jan 8, 2018

atrosinenko deleted the universal-script-names branch January 9, 2018 13:25

		@@ -223,6 +223,48 @@ For two main classes ``com.example.FooMain`` and ``com.example.BarMain`` ``sbt s

		Now you can package your application as usual, but with multiple start scripts.

		A note on script names

Change startup script name generation #1020

Change startup script name generation #1020

Conversation

atrosinenko commented Aug 28, 2017

lightbend-cla-validator commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atrosinenko commented Aug 28, 2017

muuki88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atrosinenko commented Aug 30, 2017

muuki88 commented Aug 30, 2017

atrosinenko commented Aug 30, 2017

muuki88 commented Aug 30, 2017

lightbend-cla-validator commented Sep 4, 2017

atrosinenko commented Sep 4, 2017

muuki88 commented Sep 4, 2017

atrosinenko commented Sep 17, 2017

atrosinenko commented Sep 17, 2017

muuki88 commented Sep 17, 2017

muuki88 commented Sep 21, 2017

muuki88 left a 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

atrosinenko commented Jan 2, 2018

muuki88 commented Jan 2, 2018

atrosinenko commented Jan 3, 2018

atrosinenko commented Jan 3, 2018

muuki88 commented Jan 4, 2018

atrosinenko commented Jan 5, 2018

muuki88 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atrosinenko commented Jan 5, 2018

muuki88 commented Jan 7, 2018

atrosinenko commented Jan 8, 2018

muuki88 commented Jan 8, 2018

atrosinenko commented Jan 8, 2018