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 Fri, 18 May 2018 13:14:51 GMT
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.popma@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 <keeganwitt@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 <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.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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>

Mime
View raw message