-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Introduction of maven.top-level-basedir property and updated docs #17351
Conversation
f4a7b84
to
3997354
Compare
This workflow status is outdated as a new workflow run has been triggered. |
3997354
to
eba0361
Compare
This workflow status is outdated as a new workflow run has been triggered. |
I will have to take a close look later, but have you considered |
We actually had something similar by default https://github.com/quarkusio/quarkus/blob/main/independent-projects/bootstrap/maven-resolver/src/main/java/io/quarkus/bootstrap/resolver/maven/BootstrapMavenContext.java#L817 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spotted a couple of minor things.
if (config.rootProjectDir == null) { | ||
final String topLevelBaseDirStr = PropertyUtils.getProperty(MAVEN_TOP_LEVEL_PROJECT_BASEDIR); | ||
final Path tmp = topLevelBaseDirStr == null ? null : Paths.get(topLevelBaseDirStr); | ||
this.rootProjectDir = tmp == null || !Files.exists(tmp) ? null : tmp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite convoluted.
Shouldn't we just check if it's defined and if so throw an error if it doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Done.
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building 90fa969
|
cb25232
to
2d58577
Compare
This workflow status is outdated as a new workflow run has been triggered. |
<maven.home>${maven.hom}</maven.home> <1> | ||
<maven.repo.local>${settings.localRepository}</maven.repo.local> <2> | ||
<maven.settings>${session.request.userSettingsFile.path}</maven.settings> <3> | ||
<maven.top-level-basedir>${session.topLevelProject.basedir.absolutePath}</maven.top-level-basedir> <4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem to provide the correct dir when used with -f
:
$ mvn -f integration-tests/flyway/ help:evaluate -Dexpression=session.topLevelProject.basedir.absolutePath -q -DforceStdout
/home/famod/proj/quarkus/integration-tests/flyway
$ mvn -f integration-tests/flyway/ help:evaluate -Dexpression=maven.multiModuleProjectDirectory -q -DforceStdout
/home/famod/proj/quarkus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's what Maven reports as the top-level project. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you use -f
that's what you are asking for, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I also see your point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use the reproducer from the original report, either property works. So, I'm wondering whether maven.multiModuleProjectDirectory
is a better choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, maven.multiModuleProjectDirectory
is not really documented. And it seems some Maven maintainers don't like users taking advantage of it:
https://lists.apache.org/thread.html/re9419b4792bd324eac4bbe5a5d21c11800b33c8d8fb2dfdf3eb55b28%40%3Cusers.maven.apache.org%3E
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: Link fixed ⬆️.
PPS: I use that property in my projects regardless of that controversy. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note that it's not an officially supported property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah ok. I mean it's out there, people are using it, for good reasons!
But to be on the safe side for a big public framework like Quarkus, this defensive approach makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @famod
561b2d9
to
db9dbac
Compare
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building 561b2d9
|
db9dbac
to
fcbd71a
Compare
This workflow status is outdated as a new workflow run has been triggered. |
My opinion on:
I wouldn't do it because:
So, IMO, if someone with a more "exotic" setup runs into problems he or she can have a look at the docs you've just extended and pick one of the properties. If I were that person I would go for |
fcbd71a
to
03c7e3c
Compare
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building fcbd71a
|
03c7e3c
to
4d22dd3
Compare
This workflow status is outdated as a new workflow run has been triggered. |
@gsmet is this looking ok to you? |
I'm happy @famod had a closer look. I'll merge it to get in into CR1. |
Fixes #17231
I am not sure whether I should add this property to the project generation templates.