Closed cstamas closed 1 year ago
lefou
commented
1 year ago 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.
cstamas
commented
1 year ago @lefou project used 3.6.1, I upped that to 3.6.3, and I was unaware it is intended to go even lower...
lefou
commented
1 year ago @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: Maven 3.3.1+ and in the Scala dialect pom.xml, there is this note: https://github.com/takari/polyglot-maven/blob/5a92df156dc221792cc06443cab1f4b4f93f1b12/polyglot-scala/pom.xml#L23
We should update the Readme to meet the current minimal requirements.
cstamas
commented
1 year ago Can you please split up your changes (into separate commits) so that changes which only reformat or reorder imports do not clutter the essential changes.
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?
lefou
commented
1 year ago Can you please split up your changes (into separate commits) so that changes which only reformat or reorder imports do not clutter the essential changes.
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:
* plexus components -> JSR330 * plexus Logger -> slf4jin the majority of sources...
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.
lefou
commented
1 year ago As Maven 3.9 and even Alpha Maven 4.x is required to support Java 8, we should also run test against that runtime.
laeubi
commented
1 year ago 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.
lefou
commented
1 year ago I'd like to see an update to the top-level Readme, with the minimal supported versions of Maven 3.9 3.6.3 and Java 8. How hard is it to add Java 8 to the CI setup?
cstamas
commented
1 year ago 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.
mosabua
commented
1 year ago 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.
lefou
commented
1 year ago 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.
laeubi
commented
1 year ago 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.
lefou
commented
1 year ago 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.
laeubi
commented
1 year ago I think the code base is Java 8 (maybe even Java 1.7), so probably one can just enable target=1.8 if you just care about class-file support.
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 :-)
lefou
commented
1 year ago 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.
laeubi
commented
1 year ago That's only something @cstamas can answer, as he has choose to
move to Takari shared GH action (uses Java 11 and 17)
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 + ...
cstamas
commented
1 year ago Will get back to here once resolver and maven 3.9.1 is out (week or two)
mosabua
commented
1 year ago 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.
cstamas
commented
1 year ago 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.
lefou
commented
1 year ago Thank you @cstamas for your comments. I agree, we should try to only raise the requirement per dialect, if needed.
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.
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:
So, 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.
cstamas
commented
1 year ago That was me and cool you point that out!
In short, what happened: Takari parent POM 50 lifted baseline to Java 11: https://github.com/takari/takari-pom/blob/master/pom.xml#L84 I updated parent POM in takari lifecycle and released 2.1.0 with it: https://github.com/takari/takari-lifecycle/commit/6b3634dbbb4d1d21f9dc3f9e58cd1fa7330922c1#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R18
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.
laeubi
commented
1 year ago 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.
lefou
commented
1 year ago @cstamas Great to hear, it is fixable. I hope, this also points out the importance of having a CI workflow using the minimal supported platform, which would have made that issue visible much earlier.
@laeubi I agree that Java 8 is definitely enough. It's the oldest still supported JDK.
cstamas
commented
1 year ago Created PR https://github.com/takari/takari-lifecycle/pull/113 and let's see what @jvanzyl (or @mosabua ) has to add.
laeubi
commented
1 year ago @lefou I think this is just pointing out you can't build the polyglot with Java 8, but the resulting jar can still be used with Java 8.
e.g. we use this really extensively in Tycho, while Tycho itself requires Java 17 (!) to run the build, we still compile code there for Java target 1.1 (!) (thanks to Eclipse compiler).
Also user of Tycho need Java 17 to run their maven build, but thanks to the advanced Toolchainsupport (and the eclipse compiler support), they can compile their code with whatever JVM they want even in the same build! Also different modules can then run their tests with the desired "real" JVM if needed, but there is no need to have different JVM to run the build itself.
cstamas
commented
1 year ago There is another thing I wanted to bring up: we can raise build time java requirement while keep runtime java requirement lower, meaning, if we state "polyglot minimally requires Java 8" does not mean you CAN build it with Java 8.
Example for this (will be) resolver 1.10.x, where we plan to introduce HTTP/2 transports (https://github.com/apache/maven-resolver/pull/231), and the plan to achieve this is following:
The result is, that resolver existing modules remain Java 8 (so adheres to Maven 3.9.x "minimally supported Java"), while users are able to utilize new HTTP/2 transports IF they run Maven using Java 11 or above. And these days, many of our users run Maven on 11+ Java versions, while again, they also can use release=X to target something lower at runtime.
And if paranoid, you can still setup some IT related things that verifies that at runtime all works as expected on Java8 as well.
lefou
commented
1 year ago Yeah, right. Thanks for completing the picture of options! Yet, that means, there is more work to do, before we re-establish the confidence to the same level as the status quo.
For me, as the maintainer of the Scala dialect, having some integration tests to reproduce issues is essential to keep the tool usable. Loosing this capability (at least without additional further setup) is essentially making my work harder or impossible, due to the overhead.
Being able to build the project with Java 8 is not necessary. But it's the low hanging fruit so to speak, in contrast to setup toolchains support for integration tests. Everything under the assumption, that other dialects have integration tests. As long as these work, I'm happy with any solution. And frankly, I have no idea how hard it is to setup a toolchain matrix for integration tests with Maven. But it feels very complicated.
laeubi
commented
1 year ago So what do you think about the following:
The CI integration jobs verify everything works fine, and one can simply release with a higher java (11) later on.
lefou
commented
1 year ago So what do you think about the following:
* We just use a matrix with Java 8, 11, 17 (@cstamas do you see any problem with that?) * The pom includes by default all modules that can compile/run java 8 * any module require a higher java version simply is added to the modules with a profile with jdk activation * each module additionally defines the target levelThe CI integration jobs verify everything works fine, and one can simply release with a higher java (11) later on.
That's a good plan!
laeubi
commented
1 year ago For me, as the maintainer of the Scala dialect
By the way one thing that came into my mind (I'm not familiar with Scala so maybe its a bad idea):
It seems there is a scala maven plugin, maybe it would make sense to maintain support of the polyglot-scala there and not as part of this repository? For example "Tycho-Polyglot" extension is also part of the Tycho project and only consuming the polyglot-commons and maybe that would be an option for scala as well, then changes in the scala parts do not require a polyglot release and one is more free in regards of reusing/sharing codes, used scala / java versions and the code is there where the scala experts are.
One could then just maintain a list of polyglot langs and where they are implemented / maintained and probably over time then only polyglot-commons would be required here (even though I wish it actually would be part of maven directly).
lefou
commented
1 year ago If outsourced, the Scala should become a standalone extension, IMHO. It's completely independent, can even be used without the scala-maven-plugin, or could use different versions of it. I know of pure Java projects which use the Scala polyglot extension for pure maintainability reasons. A downside of outsourcing: the whole polyglot universe would become more cluttered.
cstamas
commented
1 year ago @asomov @headius ping... please chime in
headius
commented
1 year ago Missed this one! I will catch up.
headius
commented
1 year ago 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!
cstamas
commented
1 year ago Thanks, so Java 8 as minimum is not a problem for JRuby.
cstamas
commented
1 year ago @lefou @laeubi @headius @asomov @mosabua pls add some green to this PR.
CI now builds with 8, 11, 17.
PR description updated to reflect real changes.
cstamas
commented
1 year ago Commit -2 just updates some commons deps. Commit -1 updates to Maven 3.9.5 to get rid of MANY sec issues this project has, but there is a catch: in Maven model 3.6 exclusions were AG ordered, while from 3.8 order changed to "natural" GA, so adjusted all pom.xmls used for comparisons as needed.
Compiling against 3.9.x retains backward compat, while at the same time gets deprecations visible as well...
High level changes: