Add javaModules and enablePreview options
#100
Conversation
In the dev.aspectj fork, in the process of making javadoc generation work on JDKs 8-16, instead of suppressing warnings, I rather made some minimal Javadoc changes in order to make the warnings disappear. I also changed a method parameter name at one point, making its intent clearer, and fixed a few typos in method names and group IDs. The PR also contains other documentation improvements, e.g. updated AspectJ usage page output. In .github/workflows/maven.yml, the faulty ASF licence was changed to the correct MIT licence. Probably, that was just a copy and paste error before. Finally, the main licence file was slightly updated, reflecting the transition from Codehaus to Mojohaus in 2015 in the copyright section.
Relates to mojohaus#42. TODO: write tests.
They correspond to AJC/ECJ/Javac parameters '--enable-preview' and '--module-path'. A few minor code clean-ups are also included.
So these are in the list of allowed version numbers now (AjcHelper.ACCEPTED_COMPLIANCE_LEVEL_VALUES).
| // and possibly runtime setting. As for AJDT, maybe we have to implement it ourselves, but actually I found no | ||
| // references to the Maven module there, so I guess the import is implemented somewhere else. | ||
| @Parameter( defaultValue = "false" ) | ||
| protected boolean enablePreview; |
There was a problem hiding this comment.
IMO we should not expose this option as a parameter. Artifacts created with this flag are useless with any other Java version. Most like a lot of people are not aware of this restriction. Hence if people need it, they should provide it as an additionalCompilerArgument.
There was a problem hiding this comment.
I disagree. Why would we want to patronise AspectJ developers, as if they were more stupid or less informed than any average Java developer who has this parameter at her disposal? As a separate parameter it is also easier to document, which I did. You probably read:
Activates compiler preview features (e.g. sealed classes in Java 16) when used with a suitable JDK version
If that is not enough, I can explain it similarly to how I did in the AspectJ 1.9.7 release notes:
If we could agree on making the caveat more explicit, I would be glad to improve the parameter's description. But I disagree with deliberately hiding this important compiler switch from our users. AspectJ supported JVM preview features before, and it is important that developers use them in order to gain experience and provide feedback to both OpenJDK and AspectJ.
Bottom line: Treat your fellow developers as adults, not as minors who need your protection.
There was a problem hiding this comment.
There has been one option that sounded acceptable for the maven-compiler-plugin: only allow it for SNAPSHOTs. But that proposal has been dropped, I can't remember the reason.
Not exposing this as a parameter should reduce the chance of mistakes. People that are aware of preview options can do so.
You defend the developer that wants easy access for using the preview option, whereas I defend the community that wants to consume proper artifacts. To me the impact of invalid artifacts is more important than ease of a parameter.
There was a problem hiding this comment.
Sorry to repeat myself, but again, this is not Maven Compiler Plugin. I am unaware of any policy here that we should mimic Maven Compiler Plugin in all respects. Until two days ago, this plugin did not even have an option to accept custom compiler parameters. This has just been committed, and in my AspectJ Maven fork these parameters existed before the new catch-all was added to this plugin. Now that the option exists, it is a great excuse to block all further API extensions by simply saying "we have an option to add custom compiler args". Then this plugin is no longer an abstraction of AJC, but simply a tool to pass through low level options. The user always has to check both, the compiler CLI documentation and the plugin documentation. I do not like that idea so much.
There was a problem hiding this comment.
I pushed 2cb9d40, trying to address your concerns by means of documentation instead of removing the parameter. Please also read my commit comment in order to better understand my reasoning.
There was a problem hiding this comment.
My -1, too for this parameter. If such an artefact gets into mvn central, it will just not work, unless the consumer (other dev) has the very same java version with preview features enabled as well.
Also, you can already enable it by using additionalCompilerArgument.
It was never a problem for users (devs) to use additionalCompilerArgument, why would it be now?
There was a problem hiding this comment.
It was a problem. I added the parameter on request by someone who needed it.
The existing release of this plugin (not any fork) supports only Java 8 and --enable-preview was introduced much later. As for the generic compiler argument parameter, it was committed here only 3 weeks ago: ebcda4b. So please let us not pretend that it existed for a long time while actually it is brandnew, shall we?
And how can you say it was never a problem before? This project was dormant for so long, who would have noticed? Since Javac, ECJ and AJC support the new parameter, there was no release of this plugin (and still there is none). But I did notice, because I am working on AspectJ and also was maintaining a fork of this plugin. AspectJ has tests for this compiler option, so it is only natural to also provide users here with the option. In case someone asks for support, we immediately see it in the POM and can resolve it quickly. I am just contributing this back.
If such an artefact gets into mvn central, it will just not work, unless the consumer (other dev) has the very same java version with preview features enabled as well.
Every developer who provides an artifact on Central should know this. Besides, if he would use additionalCompilerArgument, the effect would be the same. So if he wants to use it, he will. Let us not play worldwide developer police but treat fellow developers as adults, like I said before. You are implying they are kids who need to be protected from their own stupidity or immaturity. Where does this sense of superiority on you guys' part come from?
And why is my clear and comprehensive documentation not superior to security by obscurity? Is my Javadoc not clear enough?
| * @since 1.13 | ||
| */ | ||
| @Parameter | ||
| protected Module[] javaModules; |
There was a problem hiding this comment.
As you might have noticed, the maven-compiler-plugin doesn't need this to figure out the module path. Please look at that code combined with https://codehaus-plexus.github.io/plexus-languages/plexus-java/usage.html how to solve this without any extra parameters.
There was a problem hiding this comment.
I do not use modules, I added this parameter a couple of months ago because there was a use case in which modules simply did not work in AspectJ Maven for a user seeking support, or at least I could not figure out how. So I exposed this parameter to the user, and he could solve his problem. I do not remember the particulars, but I can certainly find the information somewhere in my inbox. If you want to review that case and explain to the user, how he could have done without it, feel free to give it a shot. I am certainly not the most competent person to do so. One thing I do know, though, because I had a discussion about it with Andy Clement, the AspectJ project lead: AspectJ is not particularly well prepared for JMS, because the concept of AOP and cross-cutting concerns is somewhat orthogonal to it. So whatever works, we are happy about it. Of course everything should work just like with AJC or ECJ, as long as AJC is used as a drop-in replacement for those. But as soon as aspects are involved, JMS is not so much fun anymore, because user expectations about how and what might work, might be different from reality. Sorry to digress, but it is a wider topic.
What I do remember is: A user used this parameter to solve a problem, which is why I added it. Again, we should not patronise users. To be able to put a dependency on the module path sounds like quite the reasonable feature in a JMS situation.
P.S.: Do you program a lot of AspectJ? Sorry to ask, I really don't know.
There was a problem hiding this comment.
I was a member of the Expert Group and solved the module path issues for the Maven plugins.
There was a problem hiding this comment.
I am aware of your senior status regarding Maven, but I was actually asking about AspectJ. Anyway, last time I checked, this was not Maven Compiler Plugin, i.e. it does not have the same capabilities and I am certainly not competent to port them, if that is what you are suggesting. So my idea for now is that we add this parameter, however imperfect the solution is from your perspective, until someone comes along and implements it here in the same way Maven Compiler does. I agree that would be better, but for now having this option is better than nothing. If you would volunteer to replace it with a better PR doing what you suggest, I would be glad to give up this option. But for now, I am not. WDYT? Would you volunteer to support us? That would be great and much better than just criticising me for exposing this compiler option to users in need of some JMS support.
Related question: I lately looked into the existing, but unfinished AspectJ component in Plexus Compiler and created codehaus-plexus/plexus-compiler#137 in order to at least fix basic functionality. Would I assume correctly that using Maven Compiler via Plexus in order to compile AspectJ would automatically address this issue?
There was a problem hiding this comment.
Hi Alex, is your offer to look for that mail in your inbox still a thing? I wonder if there is a better solution for the user you mentioned.
I wonder if codehaus-plexus/plexus-compiler#137 fixed this problemm too.
Have you tried it?
There was a problem hiding this comment.
is your offer to look for that mail in your inbox still a thing?
Actually, it was StackOverflow #65286736. In my inbox was simply the RSS feed notifiying me of new AspectJ questions there. Please read both the question and my answer, if you want to know in more detail. I had a longer private e-mail exchange about this and related topics with Andy Clement, but - it was private. In public, related discussion was in this thread on the AspectJ users mailing list.
I wonder if codehaus-plexus/plexus-compiler#137 fixed this problem too.
That commit was completely unrelated to this issue or JMS in general. It would not fix that issue, but rather profit from it to be a non-issue in Maven Compiler, because there according to Robert it is solved in another (better) way. That would not help us here, though, like I said - unless someone would port the same functionality to AspectJ Maven. Actually I did not try if this would automatically work in Plexus Compiler, because I am still waiting for Robert to answer this very question from my previous comment here, because he is the expert on the subject matter. @rfscholte, any insigths for Ben and me here? Sorry for repeatedly mentioning you directly. You can simply answer whenever it is the most convenient time for you, I know you must be quite busy.
There was a problem hiding this comment.
Have you read https://codehaus-plexus.github.io/plexus-languages/plexus-java/usage.html ? Basically: input is module-info + jars, output are the jars divided over modulepath and classpath.
Looking at the SO issue this should really be straight forward.
Code review feedback, thanks to @rfscholte. This parameter has its gotchas, so I think for AspectJ Maven users it is better to pull it into the API and document it better than actual CLI compilers do than to let users play with it via the generic 'additionalCompilerArgs' option and then answer lots of related StackOverflow and GitHub questions without even being able to point to the documentation ("we told you so").
They correspond to AJC/ECJ/Javac parameters
--enable-previewand--module-path.Also contained: ECJ (thus also AJC) accepts source/target like
5.0..16.0. So these are in the list of allowed version numbers now(
AjcHelper.ACCEPTED_COMPLIANCE_LEVEL_VALUES).Please note that this PR is branched off of #99. I.e. if you merge it afterwards, there will be no merge conflicts. If you prefer me to rather cherry-pick on master, I can do that too. But then maybe you have to merge, I did not check.
In addition to the 3 trivial PRs I 98-100, I also would like to contribute a few build-related things, but these require discussion first, so I want to wait for these 3 to be merged first, then we have a clean point to start from instead of juggling too many balls at once.