[MNG-7038] Introduce public properties to point to to the root and top directory of (multi-module) project#1061
Conversation
maven-embedder/src/main/java/org/apache/maven/cli/CLIReportingUtils.java
Show resolved
Hide resolved
|
Do these three properties represent the same value? |
I suppose you're talking about I'm still thinking about renaming the new property to |
|
Before we agree on new names, we need to agree what these properties should contain and the expectations/requirements from users. |
@michael-o so I propose the following:
This needs to be clarified when using |
|
Think rootdir must be the first module of the hierarchy (child to parent order) without a parent in the same relativepath (not null) or parent dir and listing the child. Would be bad to require .mvn presence imho. For me topdir is useless since we already have it somehow . |
The main use case for the
We do, it's called |
|
@gnodet sharing the use case, just adding that rootdir must not require .mvn dir presence as a marker so must use the pom hierarchy and take the highest parent, was just detailling a bit the impl idea. |
I think that would require another property, I'm all for defining additional properties. But a property based on parent/child relationship would be made available after projects are built, so it would not be able to be used during interpolation imho. |
|
@gnodet guess worse case we can do 2 passes since parent/child relationship does not support interpolation properly anyway by design. Point is that the .mvn requirement is high for a so often needed prop so 1. I'd live to not have it (technically it is cleaner too even if .mvn does not prevent to "stop bubbling") and 2. if kept like that it must be named mvn.mvn.rootdir and be made it obvious it must not be used in parent poms, boms and plugins probably (indeed i hope we avoid that since tech we dont need .mvn presence in 99% of the cases). |
The goal is to make have a public property that can be used, not to hide it. We already have |
|
I'd be to make it public but also not controlled by scripts to always be accurate. |
I have no problem with that. The point is to have a proper definition and thus computation for it. Having a |
Fwiw, that will mean that the computation needs to be duplicated somehow, so that the maven launch script can locate the |
Well, we still have issues with |
That looks like |
Not really since mvnd is a full mvn process whereas here it would just be a launcher responsible to handle the actual java command line to respect properly jvm.config. Agree the resolution can bring some resolver deps but shouldn't be as complete as mvnd. Even if I agree with the overall statement, anything in between looks wrong to me since it means topdir will not be usable until you have a .mvn (which is not the most common thing), we'll keep broken properties for this very common need and we keep a broken jvm.config so from my window everything converges to get a launcher to maven if we want these features. |
@rmannibucau That's a wrong assumption. Mvnd has a client which connects or boots a JVM (the daemon) and which is compiled to native with graalvm (or using java on unsupported platforms). The client itself does not load maven at all, but it does load the |
52cc643 to
1fcefd6
Compare
|
@gnodet right but it has the network stack which makes the binary "fat" compared to a vanilla main compiled with graalvm but let's put graal aside and agree on the fact we need a light (not java sadly) launcher to run properly and portably maven. Then topdir feature comes into this module and is well defined compared to all alternatives we have which are all partial. |
I have been thinking for years to write a C based launcher for Maven similar to Eclipse, but never found the time. |
1fcefd6 to
6d73e3a
Compare
maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/execution/MavenSession.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/execution/MavenSession.java
Outdated
Show resolved
Hide resolved
| | <<<maven.rootdir>>> | the directory containing the root <<<pom.xml>>> file of a multi module project, in a single module project this is the same as <<<project.basedir>>> | <<<$\{maven.rootdir\}>>> | | ||
| *----+------+------+ | ||
| | <<<maven.topdir>>> | the directory containing the top-level <<<pom.xml>>> in this reactor build | <<<$\{maven.topdir\}>>> | | ||
| *----+------+------+ |
There was a problem hiding this comment.
I believe that these properties should not propagate into system properties permanently, but should be private _maven. because maven. refers to the Maven installation here, everything else is project. or session.. Let's not repeat that mistake.
There was a problem hiding this comment.
I agree, I'll remove all references to maven.rootdir and maven.topdir. It makes sense to keep session.rootdir and session.topdir because they are available from those objects.
I'd rather set them as session.rootdir and session.topdir in the request system properties. That would allow using them in the context of arguments interpolation / MNG-6303.
There was a problem hiding this comment.
Make sense, but for that case maybe they should be added manually to the interpolation instead of going through system properties route?
There was a problem hiding this comment.
Actually, topdir should not be part of any model interpolation, because it's mainly the current directory and should definitely not have any effect. This will require an additional parameter to ModelInterpolator.interpolate() or to add it to the ModelBuildingRequest.
However, rootdir should be part of profile activation along basedir imho.
There was a problem hiding this comment.
? topdir should be available in interpolation, it is its main usage to reference absolutely configurations otherwise not sure its interest for end users.
There was a problem hiding this comment.
?
topdirshould be available in interpolation, it is its main usage to reference absolutely configurations otherwise not sure its interest for end users.
No, that's would be the rootdir. The topdir would be defined as the the top-most directory of executed projects in the reactor (basically, the current directory or the one pointed to when using -f/--file).
There was a problem hiding this comment.
what's the point of topdir then? it does not have to match cwd nor the rootdir so sounds like a randomdir at usage
There was a problem hiding this comment.
topdir is not really new, it's mostly a rename of the executionRootDir property to make it more coherent with basedir and rootdir, else we end up with root directory and execution root directory for different things, and that will definitely lead to problems imho.
6d73e3a to
5287c77
Compare
|
I think as far as @elharo's comments are concerned, he's right. Names should be written out, i.e., |
Should that imply that the properties become |
I wouldn't touch the properties, only Java code. But if the properties map to Java code, then this is a problem, of course. Is this the case? |
@elharo, please tell us what you think about this inconsistency. |
maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java
Outdated
Show resolved
Hide resolved
I do not think consistency is the most important virtue here. If these are new properties in 4.0, then by all means name the properties rootDirectory, topDirectory, etc. or perhaps some case variant of that like root_directory. I think basedir already exists so I wouldn't rename it. My general train of thought is to make the API and properties as clean and readable as possible, even if that introduces some inconsistencies with older API that does not follow best practices. |
There was a problem hiding this comment.
Rereading the discussions and looking into our source code, I believe that @elharo is right. Our code base from MavenProject consistently uses the term directory, everything is spelled out: directories, dependencies, repositories. Nothing abbreviated. Therefore, it seems logical that we should have ${rootDirectory}, ${project.rootDirectory}, ${session.rootDirectory}, ${session.topDirectory}
api/maven-api-core/src/main/java/org/apache/maven/api/Project.java
Outdated
Show resolved
Hide resolved
api/maven-api-core/src/main/java/org/apache/maven/api/Project.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/execution/MavenSession.java
Outdated
Show resolved
Hide resolved
maven-core/src/main/java/org/apache/maven/execution/MavenSession.java
Outdated
Show resolved
Hide resolved
| } | ||
| break; | ||
| } else { | ||
| isAltFile = arg.equals(String.valueOf(CLIManager.ALTERNATE_POM_FILE)) || arg.equals("file"); |
There was a problem hiding this comment.
This looks pretty low level, can't we use high level CLI to get this value?
There was a problem hiding this comment.
Well, the CLI isn't built yet, this is really one of the first things done. Also the point is to be able to leverage those properties during argument interpolation (see #1062), so this really has to be done very early.
There was a problem hiding this comment.
Aha, then a comment should accompany that code,
There was a problem hiding this comment.
Added some comments.
maven-model-builder/src/main/java/org/apache/maven/model/root/RootLocator.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| } catch (Exception e) { | ||
| // Ignore |
There was a problem hiding this comment.
Not sure how to handle that. If there's really pom.xml file which is not readable / parseable, the build should fail a bit later with a cleaner exception that we can display here.
That's why I decided to go this way. The problem is that doing it in a cleaner way will involve differentiating what can be ignored and what errors should be displayed to the user, and how to display them, and whether this should fail the build or not. Also, even the check is minimalist as we don't check the namespace.
What would you suggest ?
There was a problem hiding this comment.
I can add a comment :-)
There was a problem hiding this comment.
Yes, depicting that a later stage will handle this better
michael-o
left a comment
There was a problem hiding this comment.
Looks much better now. Will try this with the IT you have written.
I can also deprecate |
Let's move this to new PR where a separate ticket will make it visible. |
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
Outdated
Show resolved
Hide resolved
maven-model-builder/src/main/java/org/apache/maven/model/root/DefaultRootLocator.java
Outdated
Show resolved
Hide resolved
|
@gnodet Please don't forget to update the commit summary and JIRA summary since the property names have changed. |
…irectories of (multi-module) project This commit introduces three properties: * project.rootDirectory: the project's directory or parent directory containing a .mvn subdirectory or a pom.xml flagged with the root="true" attribute. If no such directory can be found, accessing the rootDirectory property will throw an IllegalStateException. * session.topDirectory : the directory of the topmost project being built, usually the current directory or the directory pointed at by the -f/--file command line argument. The topDirectory is similar to the executionRootDirectory property available on the session, but renamed to make it coherent with the new rootDirectory and to avoid using root in its name. The topDirectory property is computed by the CLI as the directory pointed at by the -f/--file command line argument, or the current directory if there's no such argument. * session.rootDirectory : the rootDirectory for the topDirectory project. The topDirectory and rootDirectory properties are made available on the MavenSession / Session and deprecate the executionRootDirectory and multiModuleProjectDirectory properties. The rootDirectory should never change for a given project and is thus made available for profile activation and model interpolation (without the project. prefix, similar to basedir). The goal is also to make the rootDirectory property also available during command line arguments interpolation. A root boolean attribute is also added to the model to indicate that the project is the root project. This attribute is only supported if the buildconsumer feature is active and removed before the pom is installed or deployed. It can be used as an alternative mechanism to the .mvn directory.
d959146 to
ce50ee7
Compare
|
Resolve #8000 |
1 similar comment
|
Resolve #8000 |
|
Is there a Maven 3 equivalent? Line 1 in 29b3efb |
|
There are
|
JIRA issue: [MNG-7038] Introduce public property to point to a root directory of (multi-module) project
This PR introduces three properties:
project.rootDirectory: the project's directory or parent directory containing a.mvnsubdirectory or apom.xmlflagged with theroot="true"attribute. If no such directory can be found, accessing the rootDirectory property will throw anIllegalStateException.session.topDirectory: the directory of the topmost project being built, usually the current directory or the directory pointed at by the-f/--filecommand line argument. ThetopDirectoryis similar to theexecutionRootDirectoryproperty available on the session, but renamed to make it coherent with the newrootDirectoryand to avoid using root in its name. ThetopDirectoryproperty is computed by the CLI as the directory pointed at by the-f/--filecommand line argument, or the current directory if there's no such argument.session.rootDirectory: the rootDirectory for the topDirectory project.The
topDirectoryandrootDirectoryproperties are made available on theMavenSession/Sessionand deprecate theexecutionRootDirectoryandmultiModuleProjectDirectoryproperties. TherootDirectoryshould never change for a given project and is thus made available for profile activation and model interpolation (without theproject.prefix, similar tobasedir). The goal is also to make therootDirectoryproperty also available during command line arguments interpolation.A
rootboolean attribute is also added to the model to indicate that the project is the root project. This attribute is only supported if the buildconsumer feature is active and removed before the pom is installed or deployed. It can be used as an alternative mechanism to the.mvndirectory.