switch to picocli #572

ancho · 2018-11-17T00:00:17Z

using https://github.com/remkop/picocli as a replacement for args4j.

coveralls · 2018-11-17T00:12:57Z

Coverage increased (+0.3%) to 81.087% when pulling 24fd40e on ancho:feature/picocli-integration into 1903219 on jbake-org:master.

coveralls · 2018-11-17T00:12:58Z

Coverage increased (+1.04%) to 81.818% when pulling 9cb42b5 on ancho:feature/picocli-integration into c9989a3 on jbake-org:master.

ancho · 2018-11-17T13:28:57Z

only openjdk 10 build is failing with the following error:

FAILURE: Build failed with an exception.
* What went wrong:
Could not resolve all files for configuration ':jbake-core:compileClasspath'.
> Could not resolve info.picocli:picocli:3.8.0.
  Required by:
      project :jbake-core
   > Could not resolve info.picocli:picocli:3.8.0.
      > Could not get resource 'https://jcenter.bintray.com/info/picocli/picocli/3.8.0/picocli-3.8.0.pom'.
         > Could not GET 'https://jcenter.bintray.com/info/picocli/picocli/3.8.0/picocli-3.8.0.pom'.
            > sun.security.validator.ValidatorException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

See https://travis-ci.org/jbake-org/jbake/jobs/456215263

manikmagar · 2020-05-10T19:37:43Z

@ancho There is change is one behavior - With args4j version -t had dependency on -i. Running jbake -t groovy used to fail with error description that it must be used with -i. With picocli version, runing jbake -t groovy just prints the help section with no clues to why -t did not work.

lefou · 2020-05-10T21:46:46Z

Looks like it's time to really replace args4j? Yay! I haven't reviewed this one, but just wanted to note that I once created another replacement in pull request #120. It must be really really outdated by now, but just in case you want to compare I thought I provide a link here. Feel free to ignore it!

ancho · 2020-05-11T17:13:11Z

@manikmagar thanks for catching this. I will restore that behaviour.

ancho · 2020-05-11T17:17:16Z

@lefou interesting. will have a look. even though i think that picocli is the way to go. it has a lot of nice features like creating manpages, autocomplete scripts and supports internationalization. see https://picocli.info/autocomplete.html and https://picocli.info/#_generate_man_page_documentation

manikmagar · 2020-05-12T01:38:25Z

Something related to what @lefou also mentioned in the discussion on his PR, make it easier when using JBake as library. Main.main() method causes System.exit, which could be fine when using from CLI. Should we make run(String[] args) method as public instead of protected? This then enables to use it as a library and call new Main().run(... args ..) from another code. This method nor rightly throws exception instead of causing System.exit.

ancho · 2020-05-12T06:13:18Z

Yeah. A cleanup of everything cli related needs to be done. And to be honest I would move everything cli related to the jbake-dist repo (which was originaly named jbake-cli).
I don't really see a benefit for Main to be exposed to the public as an api.

What use case do you have in mind?

manikmagar · 2020-05-13T02:18:37Z

No specific use case right now. Seeing Tobias's comment reminded me of the similar issue I ran into when using Liquibase as a library. Good that their run method is public and I could switch from main to calling run directly. We could defer it until someone ask for it.

lefou · 2020-05-13T07:08:50Z

Yeah, having a public static run method alongside the main is a common pattern. The only difference: main uses System.exit whereas run throws a App-specific exception, ideally also containing the desired exit code (that allows to exit the app early disregarding the presence of errors).

ancho · 2020-05-13T07:22:10Z

Allright. That's what the proteced run method already does ;)
Except the thing with the desired exit code. Thats a nice idea.
But I still don't see why a developer wants to use the Main class in it's own code.

lefou · 2020-05-13T07:56:27Z

It's more the integration point (like the use as a library). E.g. jbake is integrated in different build tools. The easiest integration is to just run a sub-process and evaluating the exit code. This is the most stable integration, but also gives you the least control, it's also the slowest and heaviest, as you need to create a new process, need more memory, etc.

Next level we allow with the pulbic run method: We can just load jbake as library or use an isolated classloader, but still don't need to learn the whole JBake API. We can still (re-)use the construction logic for the commandline, but without the extra process. We can use the same (hot) JVM. We can even give the user the choice to decide (classloader or process) without much efforts. But the integration logic is very simple, just some strings. It's even easy to do in reflection (e.g. when using an isolated classloaded without any extra glue API). It's also very stable against all API refactorings as long as the cmdline keeps compatible.

lefou · 2020-05-13T08:06:41Z

See https://github.com/lefou/mill-jbake.

From the plugin documentation:

def jbakeProcessMode: ProcessMode - Specify how the JBake tool should be executed.

Supported value:

ClassLoader runs JBake in an isolated classpath in the same JVM.
SubProcess spawns a new Java sub-process of each JBake tool invokation.

Defaults to ClassLoader which is also faster.
If you experience stability or out-of-memory issues, try to change this to SubProcess.

The current implementation needs to use a custom SecurityManager to intercept the System.exit calls, which does work, but not so well when used in mills parallel evaluation mode.

https://github.com/lefou/mill-jbake/blob/a538ffb5f177c56ec25de996c59558c2fad9004d/jbake/src/de/tobiasroeser/mill/jbake/JBakeWorkerClassloaderImpl.scala#L35

ancho · 2020-05-13T09:05:09Z

Ok. Got it. Thanks for the clarification. That shifts my perspective. So lets do this.
How would you extend the JbakeException to add the desired exit value?
Just an attribute with a default value of 1 with the possibility to define it on exception instantiation?

Like new JBakeException("something went wrong") // ex.exit == 1
new JBakeException("serious error", SystemExit.SERIOUS) // ex.exit == 2

Where SystemExit has predefined error codes.

lefou · 2020-05-13T11:02:17Z

Yeah, looks reasonable. Personally, I would move the exit code to the first argument and avoid the default.

gradle.properties

lefou · 2020-05-13T19:32:08Z

jbake-core/src/main/java/org/jbake/app/JBakeException.java

+    public enum SystemExit {
+        ERROR(1),
+        CONFIGURATION_ERROR(2),
+        INIT_ERROR(3),
+        SERVER_ERROR(4);


You forgot the success case (exit code 0).

Yeah. I can add that. But I don't see why I would throw an exception and exit with 0.

I've found these useful, but my understanding of JBakes codebase is a very dated, so there is probably currently no use for it. E.g. I sometime use it to exit early, e.g. when the user added a -h to the cmdline to get the help.

-h or calling jbake without arguments prints the usage information. As there was no exception thrown the status code of the cli is implicitly 0.

jbake -h; echo "status: $?" JBake v2.7.0-SNAPSHOT (2020-05-13 22:55:59nachm.) [http://jbake.org] Usage: jbake [-i[=-i] [-t=<template>]] [-bhs] [--reset] [<source>] [<destination>] JBake is a Java based, open source, static site/blog generator for developers & designers [<source>] source folder of site content (with templates and assets), if not supplied will default to current directory [<destination>] destination folder for output, if not supplied will default to a folder called "output" in the current directory -b, --bake performs a bake -h, --help prints this message --reset clears the local cache, enforcing rendering from scratch -s, --server runs HTTP server to serve out baked site, if no <value> is supplied will default to a folder called "output" in the current directory JBake initialization -i, --init[=-i] initialises required folder structure with default templates (defaults to current directory if <value> is not supplied) -t, --template=<template> use specified template engine for default templates (uses Freemarker if <value> is not supplied) status: 0

I could move the internal SystemExit class and add the SUCCESS status so a user can use it. What do you think?

Success execution with 0 looks good to me.

jonbullock

Looks good, lots of scope for improvement of the CLI with this.

jonbullock · 2021-05-06T23:08:49Z

Marked it for 2.7.0 so it's reviewed.

…th an exit code

…te given

… width

ancho · 2021-05-12T19:53:02Z

Should be merged and aligned after #615

ancho force-pushed the feature/picocli-integration branch from 621d58e to 6ac0eb6 Compare May 1, 2020 13:41

ancho requested review from jonbullock and manikmagar May 3, 2020 13:18

manikmagar reviewed May 13, 2020

View reviewed changes

gradle.properties Outdated Show resolved Hide resolved

ancho changed the title ~~switch to picocli~~ [WIP] switch to picocli May 13, 2020

ancho marked this pull request as draft May 13, 2020 17:36

ancho marked this pull request as ready for review May 13, 2020 19:09

ancho changed the title ~~[WIP] switch to picocli~~ switch to picocli May 13, 2020

ancho force-pushed the feature/picocli-integration branch from a57a960 to 3956f60 Compare May 13, 2020 20:24

lefou reviewed May 13, 2020

View reviewed changes

ancho force-pushed the feature/picocli-integration branch from 3956f60 to ee490b4 Compare May 13, 2020 21:26

jonbullock mentioned this pull request May 14, 2020

Switch to use JCommander #110

Closed

jonbullock approved these changes May 14, 2020

View reviewed changes

manikmagar approved these changes May 15, 2020

View reviewed changes

ancho mentioned this pull request Jun 1, 2020

Wrap usage at 80 characters instead of 100 #646

Merged

ancho force-pushed the feature/picocli-integration branch from 9cb42b5 to 5391a3b Compare February 24, 2021 22:46

jonbullock added this to the v2.7.0 milestone May 6, 2021

ancho added 8 commits May 11, 2021 22:18

switch to picocli

164f2db

tell user init is missing if template used without init

1d3e573

public accessible run method in Main class. extends JBakeException wi…

e2958af

…th an exit code

remove confusing arity for init option

35753aa

make template option optional use freemarker as fallback if no templa…

68918b1

…te given

extract SystemExit class

ef45bbf

set usage help autowidth option and align usage help long options max…

ba02709

… width

update picocli to 4.6.1

96156b0

ancho force-pushed the feature/picocli-integration branch from 5391a3b to 96156b0 Compare May 11, 2021 20:25

ancho added this to PR requiring manual merge in v2.7.0 Release May 12, 2021

jonbullock moved this from PR requiring manual merge to PR ready to merge in in v2.7.0 Release May 19, 2021

jonbullock merged commit dcc0e83 into jbake-org:master May 20, 2021

jonbullock moved this from PR ready to merge in to Done in v2.7.0 Release May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch to picocli #572

switch to picocli #572

ancho commented Nov 17, 2018 •

edited

coveralls commented Nov 17, 2018

coveralls commented Nov 17, 2018 •

edited

ancho commented Nov 17, 2018

manikmagar commented May 10, 2020

lefou commented May 10, 2020

ancho commented May 11, 2020

ancho commented May 11, 2020 •

edited

manikmagar commented May 12, 2020

ancho commented May 12, 2020

manikmagar commented May 13, 2020

lefou commented May 13, 2020

ancho commented May 13, 2020

lefou commented May 13, 2020

lefou commented May 13, 2020 •

edited

ancho commented May 13, 2020

lefou commented May 13, 2020

lefou May 13, 2020

ancho May 13, 2020

lefou May 13, 2020 •

edited

ancho May 13, 2020

ancho May 14, 2020

manikmagar May 15, 2020

jonbullock left a comment

jonbullock commented May 6, 2021

ancho commented May 12, 2021

switch to picocli #572

switch to picocli #572

Conversation

ancho commented Nov 17, 2018 • edited

coveralls commented Nov 17, 2018

coveralls commented Nov 17, 2018 • edited

ancho commented Nov 17, 2018

manikmagar commented May 10, 2020

lefou commented May 10, 2020

ancho commented May 11, 2020

ancho commented May 11, 2020 • edited

manikmagar commented May 12, 2020

ancho commented May 12, 2020

manikmagar commented May 13, 2020

lefou commented May 13, 2020

ancho commented May 13, 2020

lefou commented May 13, 2020

lefou commented May 13, 2020 • edited

ancho commented May 13, 2020

lefou commented May 13, 2020

lefou May 13, 2020

Choose a reason for hiding this comment

ancho May 13, 2020

Choose a reason for hiding this comment

lefou May 13, 2020 • edited

Choose a reason for hiding this comment

ancho May 13, 2020

Choose a reason for hiding this comment

ancho May 14, 2020

Choose a reason for hiding this comment

manikmagar May 15, 2020

Choose a reason for hiding this comment

jonbullock left a comment

Choose a reason for hiding this comment

jonbullock commented May 6, 2021

ancho commented May 12, 2021

ancho commented Nov 17, 2018 •

edited

coveralls commented Nov 17, 2018 •

edited

ancho commented May 11, 2020 •

edited

lefou commented May 13, 2020 •

edited

lefou May 13, 2020 •

edited