-
Notifications
You must be signed in to change notification settings - Fork 143
Issue #957: Updating docker Script to Use Latest Groovy5 #974
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
base: master
Are you sure you want to change the base?
Conversation
4298e26 to
3232038
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Item
| import java.nio.file.Paths | ||
| import java.nio.file.Path | ||
| import java.util.regex.Pattern | ||
| import groovy.cli.picocli.CliBuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this class part of groovy?
How system understand what jar to download to use this class?
We used Grape .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use @grab, still testing some things, let me test and get back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid grape if possible.
Did you run this groovy on your local?
I just need some proof that this import is available.
Can we apply this patch to run on groovy 4? If yes, let's migrate off Ant sooner, before migration to groovy 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, If you remember from last time we were also in process of upgrading groovy, but we find out that below groovy 4 we have clibuilder() and antbuilder() included in groovy all jar but from 4+ these are excluded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In local they are available given If we use
@GrabConfig(systemClassLoader=true) @Grab('info.picocli:picocli:4.2.0')
so I researched how these grab mechanism work , It
automatically download and add a dependency (JAR) to the classpath when the script runs in real-time, without mentioning it in pom.xml or adding extra dependencies.
But I am interested in knowing why you want to avoid grab engine mechanism used in groovy? are we making tradeoffs here, Isn't is more resilient like BOM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
back in time this still required some config file. But if all works without extra file , it is good.
91c4881 to
76bdb63
Compare
|
@romani Plz review:
|
|
Can you describe a problem? |
|
Hey @romani It's because cli builder class is not found as the import is no longer part of groovy all-jar, so we have to use picoli import if we want to use clibuilder for more refernce : https://stackoverflow.com/questions/73862425/groovy-clibuilder-class-not-exists |
|
based on stackoverflow , such CLI class is already in groovy4 , so we can migrate to it wihout migration to groovy 5. as we already use groovy4 images in other places, lets migrate to "info.picocli:picocli" now, test that all works. Does it sounds good to you ? |
|
Hey @romani as far as I have remember we are using 3.0.21 groovy Image in the docker file which is being used in majority of the CI's, this was precisely the reason we went for 3.0.21 ( missing cli builder) and not 4.0. I understand your view, just all CLI changes and next we can go for only version bump. Yeah I can do that, but for this picoli changes must be backward compatible, let me check. |
|
Ok It's not , info.picoli will be compatible with 4+ only, anyways we have to go for upgrading to 4.0 why not5.0 directly, or do u want to go step by step, first 4 and then 5. Let me know. |
| # Install Java 21, Groovy, Maven via SDKMAN | ||
| RUN bash -c "source $SDKMAN_DIR/bin/sdkman-init.sh && \ | ||
| sdk install java 21.0.4-tem && \ | ||
| sdk install groovy 3.0.21 && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the version we are using
76bdb63 to
106cb0f
Compare
|
if smooth migration is not possible, ok, lets migrate to 5.0 directly. I just tried to make PRs smaller and easily to approve |
part of : #957