So do you want to change this back for RC3? I know it's late to change this, but arguably it was late to change it in RC2 too.
-Keegan On Thu, May 10, 2018, 11:08 AM Remko Popma <remko.po...@gmail.com> wrote: > I updated the PR to make the signature of `setPosix` accept Boolean > instead of boolean and added tests to ensure that nulls are handled > correctly. Thanks for pointing that out! > > The picocli version of CliBuilder has tests to verify that Java-like > properties such as -Dkey=value are processed correctly in the new version > just like in the commons-cli version. (For multiple properties the option > can be specified multiple times: -Da=b -Dx=z ) > > I have not created a dummy setter for `setOptions` for the reason you > mentioned: > if users use this to configure the options it is probably better to fail > with a compiler error than silently ignoring such options. > > It's hard to tell how many people would be impacted by this, but if we > want 2.5 to be compatible with 2.4 we cannot make picocli the default in > 2.5. > > > > On Thu, May 10, 2018 at 8:42 AM, Keegan Witt <keeganw...@gmail.com> wrote: > >> 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.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.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 >>>>>>>>> >>>>>>>> >>>>>>> >>>>> >>> >> >
