Revive the project #257
Revive the project #257
Conversation
- move to Takari shared GH action (uses Java 11 and 17) - move to latest released parent (but there is takari/takari-pom#12) - move off plexus component, convert all modules to takari-maven-component packaging (JSR300) - move off plexus logging, use Slf4j - plexus-utils is NOT auto-injected anymore starting with Maven 3.9.0, sort it out - fix compared POMs, locations are all now HTTPS (not HTTP) Problems: Kotlin module fails, but am not Kotlin magician to fully fix it (140 test passes OK, but there are 4 failures where KotlinModelWriter is about to inject MavenProject -> OutOfScopeEx All the rest of modules should be OK, tested with Maven 3.6.3, 3.8.7 and 3.9.0.
|
What we aim for the lowest supported Maven version? This also determines the lowest supported Java version. AFAIK, the claimed supported Maven 3.3.x still requires us to support Java 7, which itself is a blocker for the Scala dialect to support newer Scala versions than 2.11. |
|
@lefou project used 3.6.1, I upped that to 3.6.3, and I was unaware it is intended to go even lower... |
At least the project Readme still says: polyglot-maven/polyglot-scala/pom.xml Line 23 in 5a92df1 We should update the Readme to meet the current minimal requirements. |
Um, I did not (intend to) reformat anything - unless I inadvertently did that in IDE by reflex... can you point out some files were reformat happened? As those are for sure my mistakes, all I did was:
in the majority of sources... So the end result should be "some imports removed, some imports added". Or is the import ordering maybe off? |
I saw 184 changed files, but missed the fact, that you actually really changed them. It looks to me as some pure order change at first. Sorry for that. |
|
As Maven 3.9 and even Alpha Maven 4.x is required to support Java 8, we should also run test against that runtime. |
|
I think Java 7 is really not something this project should aim at. If there is a demand, one can still use an old release. Same for Maven < 3.8.x ... given that no one has felt to do anything new (except some bugfixes) I think the impact is really low. |
There was a problem hiding this comment.
If it works go for it!
If anyone care enough the disabled modules and/or test can be revived later on.
Actually it would be great if polyglot maven would be default distributed with maven, then the impls/test here can simply consume the maven provided artifact to implement the different languages...
|
I'd like to see an update to the top-level Readme, with the minimal supported versions of Maven |
|
Currently (master branch) has CI setup as:
This PR updates the CI setup to use shared Takari GH Actions that does:
We could switch to ASF Maven shared GH actions that is very versatile, there we could define a matrix of Maven versions and matrix of Java versions as well. |
|
So awesome. I totally agree with upping the requirement to Java 11, but don't know for use in terms of usage in other project that might need older Java version. And a newer Maven version as needed is also fine. Probably even more flexible to go to newer. We should get this merged and then work with the different specialists for the different dialect to get them working again as necessary. You can tell from the git history on the different ones who these contributors mostly are. |
|
I think, unless there are good technical arguments against keeping Java 8 support, we should keep it. Raising the bar for no reasons beside "Java 8 is old" isn't a good argument IMHO. Technically we currently actually do support Java 8 and latest stable Maven also supports Java 8. And it is also reasonable for Maven users to expect plugins to also support the same platforms, (again) unless there are good reasons against it. This PR essentially drops test coverage for Java 8 for no obvious reasons, which I don't like. |
The obvious reason is: No one feels eager enough to support Java 8. So if you think its important and like to support it, please open a PR to support Java 8. Otherwise this is just the usual demand to projects to take the burden of others what is not acceptable. |
|
I probably missed the point. Isn't the actual code base effectively supporting Java 8? All it needs is to keep a CI job that runs on Java 8. Maybe I overlooked something? Or I don't get what you mean with "support". Are there any reported issues specific to Java 8? If so, maybe we should start with raising the bar only for the offending dialects. |
|
I think the code base is Java 8 (maybe even Java 1.7), so probably one can just enable Beside that, any issue (either build or compile) has to be addressed, so for example you can checkout the PR and make necessary changes (maybe as easy as setting the target level) could be a first step. Also it is quite easy to setup toolchains with github actions so probably that could be another step. Still if these are maybe trivial in the end, someone must step up to do so :-) |
|
I still don't get it, sorry. The way you said it, makes me feel like it is obvious, but it isn't. I my eyes, this PR removes the CI job that runs on Java 8 (and that way made sure we support that platform). So when asking me to step up, isn't it exactly what I do, point out that this PR makes our Java 8 story weak? I don't want to offend anybody. I'm also glad to help if there is anything I can do. It's just not clear to me, what the issue is. The PR description does not tell it. |
|
That's only something @cstamas can answer, as he has choose to
meaning the build will run once for Java 11 and Java 17 (what do not mean we can't compile for lower targets). So if @cstamas has choose this because (for him) its to complicated to work with the older setup (that do not seem to work no more out of the box), unless someone else want to sort out the issues (for me it won't be an issue not to run on Java 8) I better have a version using Java 11 than one using Java 8 but fails on Maven 3.9.x + ... |
.github/workflows/ci.yml
Outdated
| name: Verify | ||
| uses: takari/takari-gh-actions/.github/workflows/ci.yml@v1 | ||
|
|
There was a problem hiding this comment.
This is almost identically to what we had before, except the newer version of the action/cehckout action. As we want to keep Java 8 coverage, I suggest to either drop that change or pass a changed java-version-matrix to that re-used job.
Maybe the following works. If not, the takari-gh-actions might need to be changed to support changing that value.
| name: Verify | |
| uses: takari/takari-gh-actions/.github/workflows/ci.yml@v1 | |
| strategy: | |
| matrix: | |
| java-version: ['8', '11', '17'] | |
| name: Verify | |
| uses: takari/takari-gh-actions/.github/workflows/ci.yml@v1 | |
| with: | |
| java-version: ${{ matrix.java-version }} |
See also this docuemntation: https://docs.github.com/en/actions/using-workflows/reusing-workflows#using-a-matrix-strategy-with-a-reusable-workflow
|
Will get back to here once resolver and maven 3.9.1 is out (week or two) |
|
I am fine with supporting Java 8 if its easy .. but essentially the latest Java LTS version is 17 and before that 11 .. that should be plenty of a support window. |
|
Just to explain the "Problems: Kotlin module fails, but am not Kotlin magician to fully fix it (140 test passes OK, but there are 4 failures where KotlinModelWriter is about to inject MavenProject -> OutOfScopeEx" bit: It is def UT issue (so more maven related issue than kotlin) as UT does not init the session scope, but am unsure how to express that in kotlin as I never used it. Will try my best a bit later, until then free feel to push to this PR (or propose any needed changes). @laeubi @lefou I think the question is rather which Java level we need? IMHO, the history (where Maven versions and corresponding Java versions are listed) is actually "minimal Java version supported by Maven", and that does NOT mean by any means we (polyglot) MUST support those Java versions. For example, many Maven plugins already require Java8, but they still run with Maven 3.8.x (Java 7), essentially meaning the two work if you use their "common denominator" java version that is 8. This PR does NOT raise the bar (just in CI setup!), but I agree, if we claim we are Java 8 CI should run with Java 8 as well. OTOH, our case is a bit more complicated than in case of "ordinary" Java plugin (or extension), as we should find the "common denominator" for all the languages we support (jruby, kotlin, scala, etc) where all they are usable, and that may be something higher than Java 8 (otherwise the language that requires higher Java than 8 would be "stuck" on some version that may introduce problem, or even simply become "abandoned", as whole language ecosystem moved away). Or even better, IMHO, polyglot should not "align" Java level as whole (with the Maven versions it supports), but rather with Java that are suitable/required for languages it support). Something like this (do it per module):
So to say, to align modules and state for each language which Java version and why is that version "minimally required". This would mean:
My 5 cents. |
|
Thank you @cstamas for your comments. I agree, we should try to only raise the requirement per dialect, if needed.
I just noticed, that this PR in fact does raise the bar, as the changes in the toolchain/lifecycle introduce classes compiled with Java 11. See this exception, for example: Output of this PR build with Java 8So, maybe that is what @laeubi already knew or assumed? Unless we find an older version still compatible to Java 8 but also fixing the Maven 3.9 injection problem, we might be out of options. I assume, we won't convice anybody to bring back support of Java 8 in that toolchain. |
|
That was me and cool you point that out! In short, what happened: And seems I missed this (takari parent 48 vs 50) change, that with parent, I inadvertently upped Java as well from 8 to 11. Will fix this, will do a lifecycle 2,1,1 release that fixes this, sorry. |
|
Yes the commons module is the one that actually can be good to keep at a lower level (but I think 8 is really enough). If I would have to design a matrix I maybe would just:
I think it would even be fair enough to simply disable all failing modules, if anyone cares about them they can be integrated later on. |
But keep build output at Java 8 level.
As if: * used as extension, p-u MUST NOT be present (as it is in core) * used as plugin, p-u MUST BE present (as 3.9 does not auto-inject it anymore)
|
Missed this one! I will catch up. |
|
JRuby 9.4, the current major release, still supports Java 8. The previous release 9.3, now in maintenance mode, also supports Java 8 minimum. JRuby "9.5", the next big version, will bump to Java 11 at least and possibly Java 17. We have no particular requirements for specific maven versions. If you need any other input from me here let me know! |
|
Thanks, so Java 8 as minimum is not a problem for JRuby. |
That is 2.0.8.
So copy paste and modify it here.
Compiling against it still retains compatibility with 3.6.x, but you get latest code with latest deprecations. One big change: serialization order changed for dependency/exclusion element, hence order is now ga (was ag). This is merely style, no semantic value.
|
Commit -2 just updates some commons deps. Compiling against 3.9.x retains backward compat, while at the same time gets deprecations visible as well... |
This is fixed with takari/polyglot-maven#257 and released with version 0.5.0
This is fixed with takari/polyglot-maven#257 and released with version 0.5.0
This is fixed with takari/polyglot-maven#257 and released with version 0.5.0 (cherry picked from commit b62e492)
This is fixed with takari/polyglot-maven#257 and released with version 0.5.0 (cherry picked from commit b62e492)
High level changes: