Here's all the differences between 2.4 and 2.5 - setFormatter(org.apache.commons.cli.HelpFormatter) (in the PR) - setParser(org.apache.commons.cli.CommandLineParser) (in the PR) - setOptions(org.apache.commons.cli.Options) - setPosix(java.lang.Boolean) (changed from boolean to Boolean, also it's deprecated)
I'd guess setFormatter() wouldn't be a very common thing to do (I think it'd only really come up if HelpFormatter were subclassed for some reason), so that one's probably not a big risk. But I'm not sure setOptions() is safe to dummy up -- folks might be relying on this to configure the options to parse. This might be a reason not to default to picocli yet. What do you think? I also looked at how current arguments passed on the commandline could break with the move, and the only thing I noticed was Commons CLI says it can do Java-like properties such as -Djava.awt.headless=true (although I didn't see how you'd actually configure these options unless you jammed all of them into 1 giant option). -Keegan On Tue, May 8, 2018 at 1:06 AM, Paul King <pa...@asert.com.au> wrote: > Short answer is we can have as many RCs as needed to get it right but we > don't really want to be chopping and changing too much. Big changes are > supposed to be settling down by now - fate just brought this change late in > the game compared to what we'd like. I'd really like to get +ve feedback > from a SNAPSHOT with that change before doing another release including it. > I can merge in a couple of days time after conference commitments. > > Cheers, Paul. > > On Tue, May 8, 2018 at 2:27 PM, Remko Popma <remko.po...@gmail.com> wrote: > >> >> I realize now that simply removing the `parser` and `formatter` >> properties was a mistake. While the picocli version of CliBuilder cannot >> use commons-cli classes I should have anticipated that some applications >> are setting these properties. >> >> I believe that I have a solution that provides a smoother migration >> path. PR >> https://github.com/apache/groovy/pull/696 adds dummy setters for the >> legacy properties. >> >> This should significantly reduce the disruption for applications like >> Gant that depend on the ability to modify these commons-cli properties in >> CliBuilder. >> >> Would it be possible to have another 2.5-rc release with the above change >> and see what the community feedback is before we decide to revert the >> CliBuilder delegation to point to the commons-cli implementation? >> >> Marking the delegation version class as deprecated is probably a good >> idea regardless of where it points to. >> >> Remko >> >> >> On Tue, May 8, 2018 at 3:19 Paul King <pa...@asert.com.au> wrote: >> >>> >>> Both CliBuilder implementations are there via their full package names, >>> so encouraging people to use the non-delegated implementation of their >>> choice is a good option. They will need to get used to such changes in >>> Groovy 3 anyway. >>> >>> For Groovy 3, we might have a way for the compiler to translate from the >>> existing package names to the new ones brought about by JDK9 modules, in >>> which case we'd do that for CliBuilder and we'd want to point it to the >>> picocli package name by then (though if we do the change below we perhaps >>> wouldn't include CliBuilder in the translations list). >>> >>> For 2.5, we don't have the package translation in place and don't intend >>> to. Because of that, the point of the delegation version is to aid in >>> migration. >>> Maybe it was too big a jump to try to delegate to the Picocli delegation >>> in 2.5. If we find too many problems I think I should revert that change >>> and delegate to the commons cli implementation instead but mark the whole >>> delegation version class as deprecated. >>> >>> Let me know what people think. >>> >>> Cheers, Paul. >>> >>> >>> On Mon, May 7, 2018 at 3:45 PM, Keegan Witt <keeganw...@gmail.com> >>> wrote: >>> >>>> I'll take a look. I'll also see if there are other places that can >>>> break. >>>> >>>> If there are several ways it can break, then maybe we should not use a >>>> delegate to picocli and instead have folks switch the CliBuilder instance >>>> they instantiate. >>>> >>>> -Keegan >>>> >>>> On Sun, May 6, 2018, 7:56 PM Remko Popma <remko.po...@gmail.com> wrote: >>>> >>>>> I think I found a way to fix this. >>>>> See https://github.com/apache/groovy/pull/696 >>>>> This PR adds CliBuilder.setParser and CliBuilder.setFormatter methods >>>>> that ignore the specified value and print a warning to stderr. >>>>> >>>>> On Sun, May 6, 2018 at 9:14 PM, Remko Popma <remko.po...@gmail.com> >>>>> wrote: >>>>> >>>>>> In 2.5.0-rc-2, groovy.util.CliBuilder delegates to >>>>>> groovy.cli.picocli.CliBuilder. The error is that the `parser` property of >>>>>> this class is no longer writable. >>>>>> >>>>>> You can resolve this with 2.5.0-rc-2 by either not setting the >>>>>> `parser` property in the CliBuilder constructor or using the >>>>>> groovy.cli.commons.CliBuilder instead. >>>>>> >>>>>> On the Groovy side I’m not sure what the best way is to make the >>>>>> transition easier. The picocli version of CliBuilder can not make use of >>>>>> the Commons-CLI parser class. We could modify CliBuilder to silently >>>>>> ignore the specified parser. (We’d have to rename the picocli ParserSpec >>>>>> `parser` property in CliBuilder to something else.) >>>>>> >>>>>> Thoughts? >>>>>> >>>>>> >>>>>> On Sun, May 6, 2018 at 20:35 Keegan Witt <keeganw...@gmail.com> >>>>>> wrote: >>>>>> >>>>>>> FYI 2.5.0-rc-2 breaks Gant. Specifically, it's caused by changing >>>>>>> groovy.util.CliBuilder to use Picocli >>>>>>> >>>>>>> java.lang.reflect.InvocationTargetException >>>>>>> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native >>>>>>> Method) >>>>>>> at sun.reflect.NativeMethodAccess >>>>>>> orImpl.invoke(NativeMethodAccessorImpl.java:62) >>>>>>> at sun.reflect.DelegatingMethodAc >>>>>>> cessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) >>>>>>> at java.lang.reflect.Method.invoke(Method.java:498) >>>>>>> at org.codehaus.groovy.tools.Groo >>>>>>> vyStarter.rootLoader(GroovyStarter.java:114) >>>>>>> at org.codehaus.groovy.tools.Groo >>>>>>> vyStarter.main(GroovyStarter.java:136) >>>>>>> Caused by: groovy.lang.ReadOnlyPropertyException: Cannot set >>>>>>> readonly property: parser for class: groovy.util.CliBuilder >>>>>>> at groovy.lang.MetaClassImpl.setP >>>>>>> roperty(MetaClassImpl.java:2746) >>>>>>> at groovy.lang.MetaClassImpl.setP >>>>>>> roperty(MetaClassImpl.java:3782) >>>>>>> at groovy.lang.MetaClassImpl.setP >>>>>>> roperties(MetaClassImpl.java:1759) >>>>>>> at org.codehaus.groovy.runtime.ca >>>>>>> llsite.ConstructorSite$NoParamSite.callConstructor(Construct >>>>>>> orSite.java:125) >>>>>>> at org.codehaus.groovy.runtime.ca >>>>>>> llsite.CallSiteArray.defaultCallConstructor(CallSiteArray.java:59) >>>>>>> at org.codehaus.groovy.runtime.ca >>>>>>> llsite.AbstractCallSite.callConstructor(AbstractCallSite.java:238) >>>>>>> at org.codehaus.groovy.runtime.ca >>>>>>> llsite.AbstractCallSite.callConstructor(AbstractCallSite.java:250) >>>>>>> at gant.Gant.processArgs(Gant.groovy:463) >>>>>>> at gant.Gant$processArgs.call(Unknown Source) >>>>>>> at org.codehaus.groovy.runtime.ca >>>>>>> llsite.CallSiteArray.defaultCall(CallSiteArray.java:47) >>>>>>> at org.codehaus.groovy.runtime.ca >>>>>>> llsite.AbstractCallSite.call(AbstractCallSite.java:116) >>>>>>> at org.codehaus.groovy.runtime.ca >>>>>>> llsite.AbstractCallSite.call(AbstractCallSite.java:128) >>>>>>> at gant.Gant.main(Gant.groovy:668) >>>>>>> ... 6 more >>>>>>> >>>>>>> The line in Gant is >>>>>>> def cli = new CliBuilder(usage: 'gant [option]* [target]*', parser: >>>>>>> new GnuParser()) >>>>>>> >>>>>>> Was this breakage intentional? I think a lot of stuff will break >>>>>>> with parser not being able to be set anymore. >>>>>>> >>>>>>> -Keegan >>>>>>> >>>>>> >>>>> >>> >
