Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v3. Downgrade commons-cli to fix product run #314

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

aindlq
Copy link

@aindlq aindlq commented Dec 5, 2024

Product runs are currently broken because of the incompatibility between CliBuilder and commons-cli version that is used in gretty.

Gretty is using commons-cli version that is incompatible with CliBuilder from groovy 3.x. CliBuilder in groovy 3.x is accessing numberOfArgs property - https://github.com/apache/groovy/blob/GROOVY_3_0_22/subprojects/groovy-cli-commons/src/main/groovy/groovy/cli/commons/CliBuilder.groovy#L555

but it was renamed in commons-cli version after 1.3.1 to argCount, see https://github.com/apache/commons-cli/blob/cli-1.3.1/src/main/java/org/apache/commons/cli/Option.java#L72 vs currently used one https://github.com/apache/commons-cli/blob/commons-cli-1.5.0/src/main/java/org/apache/commons/cli/Option.java#L76

Exception when trying to run product generated with gretty 3.1.4:

Exception in thread "main" groovy.lang.MissingPropertyException: No such property: numberOfArgs for class: org.apache.commons.cli.Option
	at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.unwrap(ScriptBytecodeAdapter.java:65)
	at org.codehaus.groovy.runtime.callsite.GetEffectivePojoPropertySite.getProperty(GetEffectivePojoPropertySite.java:65)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callGetProperty(AbstractCallSite.java:329)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callGetPropertySafe(AbstractCallSite.java:448)
	at groovy.cli.commons.OptionAccessor.getTypedValue(OptionAccessor.groovy:101)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.codehaus.groovy.runtime.callsite.PlainObjectMetaMethodSite.doInvoke(PlainObjectMetaMethodSite.java:48)
	at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite$PogoCachedMethodSiteNoUnwrapNoCoerce.invoke(PogoMetaMethodSite.java:189)
	at org.codehaus.groovy.runtime.callsite.PogoMetaMethodSite.callCurrent(PogoMetaMethodSite.java:57)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:51)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:171)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:203)
	at groovy.cli.commons.OptionAccessor.getProperty(OptionAccessor.groovy:150)
	at org.codehaus.groovy.runtime.callsite.PogoGetPropertySite.getProperty(PogoGetPropertySite.java:49)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callGetProperty(AbstractCallSite.java:329)
	at org.akhikhl.gretty.Runner.main(Runner.groovy:34)

@boris-petrov
Copy link
Member

Hi @aindlq, thanks for the PR! So why not just use the new variable name instead of downgrading the dependency? Are there more problems?

@aindlq
Copy link
Author

aindlq commented Dec 6, 2024

@boris-petrov

commons-cli is required only for groovy.cli.commons.CliBuilder, there are no direct usage of commons-cli in gretty. CliBuilder is only used in Runner and GrettyStarter, where it is currently broken because of incompatible version.

@aindlq
Copy link
Author

aindlq commented Dec 6, 2024

Groovy 3, that gradle plugins are required to use, is incompatible with commons-cli 1.5. In groovy commons-cli was updated only in v4, see https://issues.apache.org/jira/browse/GROOVY-10331

@boris-petrov
Copy link
Member

Of course, sorry, I got confused that this is in Gretty's code. Thanks for the help!

As for the second commit - do you think it's better than what I've done in master?

@aindlq
Copy link
Author

aindlq commented Dec 6, 2024

essentially it is the same, your version excludes dependency my version force specific version of a dependency. I'll remove my change.

What is the proper way to contribute to v3. Should I rebase on master and target master for the PR?

@boris-petrov
Copy link
Member

master and v3 are developed in parallel because of the javax vs jakarta namespace change. So you might want to cherry-pick the commit from master that I mentioned in this PR and replace your commit (just to be consistent in the two branches) and then I'll merge this. After that if you can also open a PR against master with the downgrade of the commons-cli package - that would be great!

@aindlq aindlq changed the title Downgrade commons-cli to fix product run v3. Downgrade commons-cli to fix product run Dec 6, 2024
@aindlq
Copy link
Author

aindlq commented Dec 6, 2024

done #315

@boris-petrov
Copy link
Member

Thank you! I'll wait for the tests to finish and will merge both. Do you need this released quickly?

@aindlq
Copy link
Author

aindlq commented Dec 6, 2024

Thanks!

It is not ultra urgent but highly appreciated, we use gretty in https://github.com/researchspace/researchspace and currently doing a new major release, would be nice to have things working without too much hacks.

@boris-petrov
Copy link
Member

Will do! I'll create a new release later today. Thanks again for the PRs!

@boris-petrov boris-petrov merged commit 532ea78 into gretty-gradle-plugin:gretty-3.x Dec 6, 2024
7 checks passed
@aindlq
Copy link
Author

aindlq commented Dec 6, 2024 via email

@boris-petrov
Copy link
Member

Released in 4.1.6 and 3.1.5! Thank you again!

@aindlq
Copy link
Author

aindlq commented Dec 6, 2024

Thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants