Conversion of JDKPackager to use JDK-provided Ant tasks. #583

metasim · 2015-05-13T18:29:32Z

A major overhaul, replacing the command-line tool based package generation with an Ant task based one, thereby exposing new capabilities, including:

Better file mapping support
JVM arguments
Application (i.e. main) arguments
System properties
File associations

The task jdkpackager:antBuildDefn can be intercepted, allowing advanced customization via XML DOM processing. jdkpackager:writeAntBuild generates the file target/jdkpackager/build.xml, which can be inspected for debugging.

muuki88 · 2015-05-13T22:37:49Z

src/main/scala/com/typesafe/sbt/packager/jdkpackager/JDKPackagerAntHelper.scala

+   *
+   * see: https://docs.oracle.com/javase/8/docs/technotes/guides/deploy/javafx_ant_task_reference.html
+   */
+  private[jdkpackager] def makeAntBuild(


Issue found: Method is longer than 50 lines

Issue found: The number of parameters should not exceed 8

@muuki88 How strict are you looking to be with the coding checks? I think the method "longer than 50 lines" in question is the one containing the Ant XML DOM. I can split it up into multiple functions, but you'd loose the one-to-one correspondence with what ends up getting written to the build.xml file.

Also, in order to reduce the number of parameters to the DOM construction function I'd have to either introduce new intermediate container classes, or add some other level of indirection that I actually think adds complexity. That said, I'll submit something.

I'll have a closer look. But for good reasons (e.g. clearly more readable), then rules should be broken.

So. This is a lot of XML inside a class. I know scala supports creating xml. However what do you think, putting this into an jdkpackager-ant.xml.template and insert the dynamic parts with place holders?

Would hate to looses the type checking, which has saved me a number of times. See what you think of the latest update, where I modularized it some.

see #583 (comment)

Yeah, that looks cleaner.

muuki88 · 2015-05-14T14:28:12Z

src/main/scala/com/typesafe/sbt/packager/jdkpackager/JDKPackagerPlugin.scala

-  object autoImport extends JDKPackagerKeys {
-    val JDKPackager = config("jdkPackager") extend Universal
-  }
+  object autoImport extends JDKPackagerKeys


The autoImport should also contain the JDKPackagerToolkit so the user doesn't have to import it manually.

Would hate to looses the type checking, which has saved me a number of times. See what you think of the latest update, where I modularized it some.

muuki88 · 2015-05-14T14:31:31Z

One thing regarding the PR. Could you squash the commits into one?

metasim · 2015-05-14T16:46:23Z

@muuki88 Not sure I know how to collapse commits. Pointers? Sorry for such a n00b question.

muuki88 · 2015-05-14T18:23:30Z

Sure :) In essence you do a git rebase -i origin/master, choose the squash option and push -f your branch. For a bit longer description:

metasim · 2015-05-14T21:39:30Z

@muuki88 Thanks for your patience with this one. Glad to know how to squash now! :-)

muuki88 · 2015-05-14T21:49:00Z

No problem :) Always happy to help.

muuki88 · 2015-05-15T20:45:29Z

In the test project on windows I get

[error] (jdkPackager:packageBin) E:\sbt-native-packager\test-project-jdkpackager\target\jdkpackager\build.xml:25: fx:info doesn't support the nested "association" element.

On Ubuntu 15.04 with java version "1.8.0_45 I get this

[error] (jdkPackager:antBuildDefn) Please set key `antPackagerTasks in JDKPackager` to `ant-javafx.jar` path, which should be find in the `lib` directory of JDK 8 installation.
[error] Total time: 7 s, completed 15.05.2015 22:44:22

Needed to set it manually with antPackagerTasks in JDKPackager := Some(file("/usr/lib/jvm/java-8-oracle/lib/ant-javafx.jar"))

metasim · 2015-05-15T20:52:40Z

Ooof. Sorry about that. Thanks for the extra check. Will address soon.

Wonder why Travis CI didn't catch the Linux one?

muuki88 · 2015-05-16T08:41:40Z

Yeah, that's weird as travis uses the oracle jdk as well. However they have ubuntu 14.04 LTS I guess.

muuki88 · 2015-05-16T09:52:38Z

This seems to be an Ubuntu specific problem. The old jdk-packager version doesn't work as well.

muuki88 · 2015-05-19T16:34:44Z

src/sphinx/formats/jdkpackager.rst

+    jdkPackagerAssociations := Seq(
+        FileAssociation("foobar", "application/foobar", "Foobar file type"),
+        FileAssociation("barbaz", "application/barbaz", "Barbaz file type", jdkAppIcon.value)
+    )



Could you add one small section here, showing or to add other jdk-locations (if not found by default), like this

jdkPackagerTool := jdkPackagerTool.value orElse Some(file("/usr/lib/jvm/java-8-oracle/bin/javapackager"))

and how setting JDK_HOME or JAVA_HOME may help ;)

Will do! I assume you mean in the example build.sbt file contents in jdkpackager.rst? Or are you suggesting some more aggressive searching in the code?

muuki88 · 2015-05-19T16:35:34Z

one last comment. I think a small documentation hint should do the trick as jvm detection is already sophisticated and we can make it more sophisticated in follow up pull requests.

metasim · 2015-05-19T17:16:51Z

@muuki88 On your last comment, do you mean additional info in the Sphinx docs, or do you mean in the error message?

muuki88 · 2015-05-19T17:31:50Z

@metasim I meant sphinx. I think the error message is explanatory enough

metasim · 2015-05-19T17:38:35Z

@muuki88 Regarding this:

In the test project on windows I get

[error] (jdkPackager:packageBin) E:\sbt-native-packager\test-project-jdkpackager\target\jdkpackager\build.xml:25: fx:info doesn't support the nested "association" element.

What version of Java were you using? I'm wondering if the file association feature was added in JDK 8 build 40 (yes... Oracle added major new features in a build-increment release).

It works on my build VM with JDK 1.8.0_45.

muuki88 · 2015-05-19T17:52:32Z

I think I have an older one. Will try again, but as this is still experimental and working on your machine, we will refine later on.

metasim · 2015-05-19T19:03:49Z

@muuki88 In addition to some updates to the documentation, I added debug mode logging of the search process for ant-javafx.jar (which replaces searching for javapackager). Also added settings key docs on file associations needing JDK 8 build >= 40.

Are you OK with antPackagerTasks being scoped to JDKPackager config?

muuki88 · 2015-05-20T14:02:38Z

brilliant. can you squash again and I'll merge it right away :)

Provides more features and better configurability. History: * Fixed scripted property checking messages. * Reworked settings to use `.value` over `<<=` + `map` (and related constructs). * Initial attempt at resourcing mappings. * Updating tests for JDKPackagerPlugin. * Added call to Ant library to build package. * Added explicit icon to Ant build.xml. * Removed command-line mechanism for building package. Now completely Ant based. * Added ability to specify JVM user args and application args. * Added specification of runtime properties. * Added ability to define file associations. * Updated documentation. * Fixed expected output type. * Modularized Ant DOM building to appease static code checker. * Moved support case classes into autoImport. * Updated documentation in general, and specifically on specifying location of `ant-javafx.jar`. * Added debug logging messages during search for `ant-javafx.jar`.

metasim · 2015-05-20T14:49:40Z

@muuki88 Squashed. :-)

metasim · 2015-05-20T15:03:57Z

@muuki88 Just so you're aware (as the project lead), this plugin now depends on the LauncherJarPlugin, depending upon the Main-Class and Class-Path manifest attributes defined therein. I've tried to rely on the by-products of the other packaging plugins whenever I can.

Conversion of JDKPackager to use JDK-provided Ant tasks.

muuki88 · 2015-05-20T20:51:16Z

Awesome work ( as always ). Thanks for highlighting the dependency. The classpath plugins are getting used more and more, so using these is totally fine.

muuki88 · 2015-05-20T20:51:46Z

I'll make an release ASAP

metasim · 2015-05-21T13:11:01Z

@muuki88 Thanks so much. Don't forget to release the docs too (don't know if it's automatic).... old ones would really throw off users.

File associations should be a winning feature for some.

muuki88 reviewed May 13, 2015
View reviewed changes

muuki88 added the jdkpackager label May 14, 2015

muuki88 reviewed May 14, 2015
View reviewed changes

metasim force-pushed the jdkpackager/ant-packaging branch 2 times, most recently from 5930052 to 30c8430 Compare May 14, 2015 21:31

muuki88 reviewed May 19, 2015
View reviewed changes

metasim force-pushed the jdkpackager/ant-packaging branch from 4c48060 to c683dd6 Compare May 20, 2015 14:47

metasim force-pushed the jdkpackager/ant-packaging branch from c683dd6 to 6a40111 Compare May 20, 2015 14:48

muuki88 added a commit that referenced this pull request May 20, 2015

Merge pull request #583 from metasim/jdkpackager/ant-packaging

403d286

Conversion of JDKPackager to use JDK-provided Ant tasks.

muuki88 merged commit 403d286 into sbt:master May 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conversion of JDKPackager to use JDK-provided Ant tasks. #583

Conversion of JDKPackager to use JDK-provided Ant tasks. #583

metasim commented May 13, 2015

muuki88 May 13, 2015

muuki88 May 13, 2015

metasim May 14, 2015

muuki88 May 14, 2015

muuki88 May 14, 2015

muuki88 May 14, 2015

muuki88 May 14, 2015

muuki88 May 14, 2015

metasim May 14, 2015

muuki88 commented May 14, 2015

metasim commented May 14, 2015

muuki88 commented May 14, 2015

metasim commented May 14, 2015

muuki88 commented May 14, 2015

muuki88 commented May 15, 2015

metasim commented May 15, 2015

muuki88 commented May 16, 2015

muuki88 commented May 16, 2015

muuki88 May 19, 2015

metasim May 19, 2015

muuki88 commented May 19, 2015

metasim commented May 19, 2015

muuki88 commented May 19, 2015

metasim commented May 19, 2015

muuki88 commented May 19, 2015

metasim commented May 19, 2015

muuki88 commented May 20, 2015

metasim commented May 20, 2015

metasim commented May 20, 2015

muuki88 commented May 20, 2015

muuki88 commented May 20, 2015

metasim commented May 21, 2015

Conversion of JDKPackager to use JDK-provided Ant tasks. #583

Conversion of JDKPackager to use JDK-provided Ant tasks. #583

Conversation

metasim commented May 13, 2015

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

muuki88 commented May 14, 2015

metasim commented May 14, 2015

muuki88 commented May 14, 2015

metasim commented May 14, 2015

muuki88 commented May 14, 2015

muuki88 commented May 15, 2015

metasim commented May 15, 2015

muuki88 commented May 16, 2015

muuki88 commented May 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muuki88 commented May 19, 2015

metasim commented May 19, 2015

muuki88 commented May 19, 2015

metasim commented May 19, 2015

muuki88 commented May 19, 2015

metasim commented May 19, 2015

muuki88 commented May 20, 2015

metasim commented May 20, 2015

metasim commented May 20, 2015

muuki88 commented May 20, 2015

muuki88 commented May 20, 2015

metasim commented May 21, 2015