groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Keegan Witt <keeganw...@gmail.com>
Subject Re: Groovy 2.5.0-rc-2 breaks Gant
Date Wed, 09 May 2018 23:42:24 GMT
 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 <paulk@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.popma@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 <paulk@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 <keeganwitt@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.popma@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.popma@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 <keeganwitt@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
>>>>>>>
>>>>>>
>>>>>
>>>
>

Mime
View raw message