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.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) >>>>> at >>>>> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) >>>>> at java.lang.reflect.Method.invoke(Method.java:498) >>>>> at >>>>> org.codehaus.groovy.tools.GroovyStarter.rootLoader(GroovyStarter.java:114) >>>>> at >>>>> org.codehaus.groovy.tools.GroovyStarter.main(GroovyStarter.java:136) >>>>> Caused by: groovy.lang.ReadOnlyPropertyException: Cannot set readonly >>>>> property: parser for class: groovy.util.CliBuilder >>>>> at >>>>> groovy.lang.MetaClassImpl.setProperty(MetaClassImpl.java:2746) >>>>> at >>>>> groovy.lang.MetaClassImpl.setProperty(MetaClassImpl.java:3782) >>>>> at >>>>> groovy.lang.MetaClassImpl.setProperties(MetaClassImpl.java:1759) >>>>> at >>>>> org.codehaus.groovy.runtime.callsite.ConstructorSite$NoParamSite.callConstructor(ConstructorSite.java:125) >>>>> at >>>>> org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallConstructor(CallSiteArray.java:59) >>>>> at >>>>> org.codehaus.groovy.runtime.callsite.AbstractCallSite.callConstructor(AbstractCallSite.java:238) >>>>> at >>>>> org.codehaus.groovy.runtime.callsite.AbstractCallSite.callConstructor(AbstractCallSite.java:250) >>>>> at gant.Gant.processArgs(Gant.groovy:463) >>>>> at gant.Gant$processArgs.call(Unknown Source) >>>>> at >>>>> org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:47) >>>>> at >>>>> org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:116) >>>>> at >>>>> org.codehaus.groovy.runtime.callsite.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 >>>>> >>>> >>> >
