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

Make mvnd 1.X work and require Maven 3.9.6 to build Quarkus #41648

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

manofthepeace
Copy link
Contributor

@manofthepeace manofthepeace commented Jul 2, 2024

Fixes #41323

Supersedes #41628

This is a bit of a hack. But it works.

The codebase has a lot of references to maven.multiModuleProjectDirectory not sure if everything does work. But for futureproofness this should be changed, I think.

Occurences;

  • core/processor/src/main/java/io/quarkus/annotation/processor/Constants.java
  • docs/src/main/asciidoc/maven-tooling.adoc
  • docs/src/main/asciidoc/tests-with-coverage.adoc
  • independent-projects/parent/pom.xml
  • independent-projects/resteasy-reactive/pom.xml
  • tcks/resteasy-reactive/pom.xml
  • Some places in the maven wrapper.

cc @gsmet

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Jul 3, 2024

@manofthepeace did you test with -f and also when building inside a nested directory that contains a reference to this property?

@manofthepeace
Copy link
Contributor Author

@manofthepeace did you test with -f and also when building inside a nested directory that contains a reference to this property?

What I tried is the following;

  • Full build via mvn and mvnd
  • from project root; mvnd/mvn -f tcks/resteasy-reactive clean install -Dquickly
  • from tcks/resteasy-reactive mvnd/mvn clean install -Dquickly
@manofthepeace
Copy link
Contributor Author

If you want precises tests done please let me know. But I assume we will hit this somehow; apache/maven#1589 . I think its still what can be done atm without downgrading.

but I think the fix proposed is still needed nevertheless as maven.multiModuleProjectDirectory is not coming back

@gsmet
Copy link
Member

gsmet commented Jul 4, 2024

Do you happen to know which versions support session.rootDirectory?

I stumbled upon https://issues.apache.org/jira/browse/MNG-7038 but it's only marked for Maven 4.

@manofthepeace
Copy link
Contributor Author

This PR actually uses session.rootDirectory which is puts in to maven.multiModuleProjectDirectory for things to just "work".

From what I understood from apache/maven-mvnd#1031 (comment) . Is that the value is only available in settings this needs to be used that way. In Maven 4 I believe this can be removed as it can be used as property, so we can change it in the poms and other places.

@gsmet
Copy link
Member

gsmet commented Jul 4, 2024

Yeah, my question was about when session.rootDirectory was introduced as to know if we would need to require a more recent version of Maven to build the project.

@manofthepeace
Copy link
Contributor Author

https://maven.apache.org/docs/3.9.2/release-notes.html

Task MNG-7774 implemented interpolation for configuration and command line, but also provides two new properties usable in configuration interpolation: session.topDirectory (reactor top directory) and session.rootDirectory (project root directory, usually where .mvn directory reside). It is recommended to create .mvn directory in project root directory, as presence of this directory is used to detect root directory location. If .mvn directory does not exists, root directory will not be detected, and in such case attempted use of expression session.rootDirectory in interpolation will make Maven refuse to start (will report error). It is important to mention, that those two new properties, are only working within the maven.config file and via the command line. They are NOT usable as configuration properties within your plugin configuration in your pom.xml file.

does that answer?

@gsmet
Copy link
Member

gsmet commented Jul 8, 2024

@manofthepeace yes, perfect thanks.

I will require Maven 3.9.6+ to build Quarkus.

@gsmet gsmet changed the title Make mvnd 1.X work Jul 8, 2024
@quarkus-bot

This comment has been minimized.

@quarkus-bot quarkus-bot bot added the area/gradle Gradle label Jul 9, 2024
@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Jul 9, 2024

I ran some more tests and something is odd.

When running a mvnd build with this PR, I end up with a ${session.rootDirectory} directory in my Quarkus directory with the following content:

${session.rootDirectory}/
└── target
    └── asciidoc
        └── generated

(and all the doc content is there).

So there's something that needs fixing in the tree.

@manofthepeace
Copy link
Contributor Author

Interesting. Could it be something with asciidoctor itself, or the asciidoctor maven plugin? In anycase do you think this is major, is mvnd used in general internally/ to build the asciidoc?

I'll try to have a look eventually. If you have any pointers let me know.

@gsmet
Copy link
Member

gsmet commented Jul 10, 2024

It's essential to have it working. I'm working on a patch.

@gsmet
Copy link
Member

gsmet commented Jul 10, 2024

So the issue is that we end up with the following system property:

maven.multiModuleProjectDirectory=${session.rootDirectory}

And well, when we try to get the maven.multiModuleProjectDirectory property in the annotation processor, we get the ${session.rootDirectory} string.

https://github.com/quarkusio/quarkus/blob/main/core/processor/src/main/java/io/quarkus/annotation/processor/Constants.java#L88-L89

Unfortunately, session.rootDirectory is not a system property so we cannot get the path from the system properties anymore.

Note that the current behavior is very hackyish and we really need to do better and I probably need to revive my rewrite of the annotation processor. But ideally I would like to have a quick fix.

The only thing that makes sense would be to pass the path as an argument to the annotation processor so that it's fully resolved by Maven first (you can't pass additional system properties to the compiler plugin).

@manofthepeace
Copy link
Contributor Author

Ok, I figured it could be something like this, where the value was taken but not expanded by something out of maven.. This is unfortunate, I do not see a quick win at first glance. Not sure how far maven 4 is, but I would assume it would solve all of this.. and probably create a bunch of other things though..

Note that the current behavior is very hackyish and we really need to do better

well.. I feel like removing a property like this in a minor version was not the best, so recourse to hacks to bring it back, as maven themselves dont have a way around this makes it somewhat necessary, to reinstate the old behaviour.. I was hoping the maven.config woult actually set an env, and not something just in maven, apparently it is not the case.

I'll try to take a look. But I am short on ideas.

@manofthepeace
Copy link
Contributor Author

manofthepeace commented Jul 10, 2024

@gsmet do you think this could potentially work? https://stackoverflow.com/questions/45723834/pass-argument-to-annotation-processor. i.e passing the arg to the annotation processor.

EDIT: nevermind I see it is used externally.. Only way I see is again super hacky, but it would be to go from the CWD and recurse to the root via the existence of a pom file.

@gsmet
Copy link
Member

gsmet commented Jul 10, 2024

Yeah that's what I had in mind.
I don't think we will have a lot of other options unfortunately.

@gsmet
Copy link
Member

gsmet commented Jul 10, 2024

But long term, we actually need the extension processor to generate content in the module itself so that should probably be easier (we want to avoid writing things outside of the scope of the module, to make caching with Develocity possible).

I started working on that a while ago. I'll try to revive the PR and see if I can come up with something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment