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

[MCOMPILER-579] allow module-version configuration #273

Conversation

mguillem
Copy link

Allow module-version configuration (like in master branch).

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MCOMPILER-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MCOMPILER-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@mguillem
Copy link
Author

mguillem commented Jan 8, 2025

Is there a chance to see this merged and released? Otherwise we'll have to start working with our own private version of the plugin what is never a pleasure to maintain.

@slachiewicz slachiewicz added the enhancement New feature or request label Jan 8, 2025
@slawekjaranowski slawekjaranowski self-assigned this Feb 5, 2025
@slawekjaranowski
Copy link
Member

@mguillem I will take care about it.
Thanks for patient.

@slawekjaranowski slawekjaranowski added this to the 3.13.1 milestone Feb 5, 2025
@slawekjaranowski slawekjaranowski changed the title MCOMPILER-579 allow module-version configuration [MCOMPILER-579] allow module-version configuration Feb 5, 2025
Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are missing tests which reproduce a problem ...

@slawekjaranowski slawekjaranowski removed this from the 3.14.0 milestone Feb 5, 2025
@slawekjaranowski slawekjaranowski removed their assignment Feb 5, 2025
@mguillem
Copy link
Author

@slachiewicz the changes have been picked up from master branch. The master branch contains unit tests for that but the setup for these tests is not available in the 3.x branch and would be far more complex than the 3 trivial lines of code changed in this PR. Backporting the tests make only sense to me if I can be sure that there is an interest for them from the project maintainers.

@slawekjaranowski
Copy link
Member

ok, I see -

@Parameter(property = "moduleVersion", defaultValue = "${project.version}")

I would like to change a property moduleVersion to maven.compiler.moduleVersion in both branches.

@slawekjaranowski
Copy link
Member

property in master #302

@slawekjaranowski
Copy link
Member

slawekjaranowski commented Feb 13, 2025

One more question moduleVersion is added to AbstractCompilerMojo but it is only used in CompilerMojo it is not used in TestCompilerMojo ... so should be moved to CompilerMojo to be clear.

In other way will be avaliable as parameter for TestCompilerMojo but will be not used ...

@mguillem @slachiewicz @desruisseaux

@desruisseaux
Copy link
Contributor

Right. Would you like to do it in this pull request?

@slawekjaranowski
Copy link
Member

Right. Would you like to do it in this pull request?

yes I would like .. but look like --module-version is not used in TestCompilerMojo but it can be another issue to clarify

@desruisseaux
Copy link
Contributor

I think that we should not provide --module-version parameter for the test classes. We are restricted to only one module version per module-info.class file, and that file is in the main code. I realize that we have documentation in the Maven web site suggesting to provide a module-info in the tests also, but I think that we should deprecate this practice because of its drawback. For example, this approach (overwriting the main/module-info.java with test/module-info.java) requires that Maven plugins put the project upside-down, with the test classes declared as the main project and the real main classes declared as patches applied on the tests. There is better alternatives, such as generating the --patch-module parameters automatically.

Therefore, if you agree to move moduleVersion into CompilerMojo, I suggest that you do it in this pull request.

@heliodoro198205
Copy link

heliodoro198205 commented Feb 13, 2025 via email

@slawekjaranowski slawekjaranowski merged commit 09dce4e into apache:maven-compiler-plugin-3.x Feb 15, 2025
88 checks passed
@github-actions github-actions bot added this to the 3.14.0 milestone Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants