[MNG-7038] Introduce public properties to point to to the root and top directory of (multi-module) project #1061

gnodet · 2023-03-16T15:04:43Z

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 .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.

maven-embedder/src/main/java/org/apache/maven/cli/CLIReportingUtils.java

michael-o · 2023-03-17T14:53:41Z

Do these three properties represent the same value?

gnodet · 2023-03-17T16:35:27Z

Do these three properties represent the same value?

I suppose you're talking about topdir, project.topdir and session.topdir ? They are aliases, but I don't think the project.topdir is a good idea, since it's not really a project attribute.

I'm still thinking about renaming the new property to rootdir, and introduce a topdir for the execution directory in a consistent way. And deprecate MavenSession.getExecutionRootDirectory(), MavenExecutionRequest.getBasedir(), MavenExecutionRequest.getMultiModuleProjectDirectory() accordingly. I'm open to any naming, but things are a bit inconsistent, so I'd rather fix it.

michael-o · 2023-03-17T20:17:35Z

Before we agree on new names, we need to agree what these properties should contain and the expectations/requirements from users.

gnodet · 2023-03-18T06:55:50Z

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:

topdir : the directory of the top-most project being built, this is usually the current directory, or the one pointed by the -f/--file option
rootdir : the parent directory containing a child .mvn directory. If there's no such directory, print a warning and use a topdir

This needs to be clarified when using -pl/-am/-amd options.

rmannibucau · 2023-03-18T07:29:00Z

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 .

gnodet · 2023-03-18T19:52:22Z

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.

The main use case for the rootdir is for build related files location, for example, locate files shared throughout the build. This is currently a bit difficult to use. I have not seen any real use case for the parent/child relationship.

For me topdir is useless since we already have it somehow .

We do, it's called executionRootDir.

rmannibucau · 2023-03-18T20:01:13Z

@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.

gnodet · 2023-03-18T20:10:37Z

@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.

rmannibucau · 2023-03-18T20:18:10Z

@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).

gnodet · 2023-03-18T20:38:31Z

@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 multiModuleProjectDirectory which is hidden. What's the point of having it hidden ?

rmannibucau · 2023-03-18T23:59:04Z

I'd be to make it public but also not controlled by scripts to always be accurate.

gnodet · 2023-03-19T00:51:52Z

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 .mvn directory gives a clear and simple definition and computation. Not having it may lead to go outside the source tree (if the project is part of a bigger project) or unable to be detected (in which case the multiModuleProjectDirectory defaults to the current dir).

gnodet · 2023-03-19T20:44:43Z

I'd be to make it public but also not controlled by scripts to always be accurate.

Fwiw, that will mean that the computation needs to be duplicated somehow, so that the maven launch script can locate the .mvn/jvm.config file and read it before starting the JVM...

rmannibucau · 2023-03-19T21:01:08Z

Fwiw, that will mean that the computation needs to be duplicated somehow, so that the maven launch script can locate the .mvn/jvm.config file and read it before starting the JVM...

Well, we still have issues with jvm.config (spaces, quotes, EOL, ...) in terms of parsing so wonder if we shouldn't revisit it anyway. I had in mind a small native binary (either using graalvm without http and all the options making it fat or whatever is fast to launch) which could handle it to actually fork (unix)/trigger the real execution but, IMHO, it must belong to maven and not any enclosing tooling otherwise it is not part of maven and lost easily in any integration which must mimic our fragile script.

gnodet · 2023-03-20T06:35:28Z

Fwiw, that will mean that the computation needs to be duplicated somehow, so that the maven launch script can locate the .mvn/jvm.config file and read it before starting the JVM...

Well, we still have issues with jvm.config (spaces, quotes, EOL, ...) in terms of parsing so wonder if we shouldn't revisit it anyway. I had in mind a small native binary (either using graalvm without http and all the options making it fat or whatever is fast to launch) which could handle it to actually fork (unix)/trigger the real execution but, IMHO, it must belong to maven and not any enclosing tooling otherwise it is not part of maven and lost easily in any integration which must mimic our fragile script.

That looks like mvnd but without keeping daemons alive...

rmannibucau · 2023-03-20T09:41:03Z

That looks like mvnd but without keeping daemons alive...

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.

gnodet · 2023-03-20T09:45:17Z

That looks like mvnd but without keeping daemons alive...

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.

@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 .mvn/jvm.config file.
See https://github.com/apache/maven-mvnd/blob/master/client/src/main/java-mvnd/org/mvndaemon/mvnd/client/DefaultClient.java#L143-L148

rmannibucau · 2023-03-20T10:07:33Z

@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.

michael-o · 2023-03-20T10:14:08Z

@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.

maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java

maven-core/src/main/java/org/apache/maven/execution/MavenSession.java

michael-o · 2023-03-20T13:00:49Z

maven-model-builder/src/site/apt/index.apt

+| <<<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\}>>> |
+*----+------+------+


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.

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.

Make sense, but for that case maybe they should be added manually to the interpolation instead of going through system properties route?

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.

? topdir should be available in interpolation, it is its main usage to reference absolutely configurations otherwise not sure its interest for end users.

? topdir should 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).

what's the point of topdir then? it does not have to match cwd nor the rootdir so sounds like a randomdir at usage

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.

gnodet · 2023-04-14T09:22:19Z

I'm done with the changes and added an IT.

michael-o · 2023-04-14T09:43:14Z

I'll try to review this weekend.

michael-o · 2023-04-17T18:38:36Z

I think as far as @elharo's comments are concerned, he's right. Names should be written out, i.e., getRootDirectory(), getTopDirectory(), etc.

gnodet · 2023-04-17T19:08:20Z

I think as far as @elharo's comments are concerned, he's right. Names should be written out, i.e., getRootDirectory(), getTopDirectory(), etc.

Should that imply that the properties become project.rootDirectory, session.rootDirectory and session.topDirectory ?
I think people are used to basedir so I wanted to align with it and make things coherent. If so, I'd rename that one to project.baseDirectory too.

michael-o · 2023-04-17T19:11:16Z

I think as far as @elharo's comments are concerned, he's right. Names should be written out, i.e., getRootDirectory(), getTopDirectory(), etc.

Should that imply that the properties become project.rootDirectory, session.rootDirectory and session.topDirectory ? I think people are used to basedir so I wanted to align with it and make things coherent. If so, I'd rename that one to project.baseDirectory too.

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?

michael-o · 2023-04-18T07:44:27Z

I think as far as @elharo's comments are concerned, he's right. Names should be written out, i.e., getRootDirectory(), getTopDirectory(), etc.

Should that imply that the properties become project.rootDirectory, session.rootDirectory and session.topDirectory ? I think people are used to basedir so I wanted to align with it and make things coherent. If so, I'd rename that one to project.baseDirectory too.

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

elharo · 2023-04-18T11:37:51Z

Should that imply that the properties become project.rootDirectory, session.rootDirectory and session.topDirectory ?
I think people are used to basedir so I wanted to align with it and make things coherent. If so, I'd rename that one to project.baseDirectory too.

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.

michael-o

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}

maven-model-builder/src/site/apt/index.apt

michael-o

A few misses

api/maven-api-core/src/main/java/org/apache/maven/api/Project.java

maven-core/src/main/java/org/apache/maven/execution/MavenExecutionRequest.java

maven-core/src/main/java/org/apache/maven/execution/MavenSession.java

michael-o · 2023-04-19T13:32:48Z

maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java

+                }
+                break;
+            } else {
+                isAltFile = arg.equals(String.valueOf(CLIManager.ALTERNATE_POM_FILE)) || arg.equals("file");


This looks pretty low level, can't we use high level CLI to get this value?

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.

Aha, then a comment should accompany that code,

Added some comments.

maven-model-builder/src/main/java/org/apache/maven/model/root/RootLocator.java

maven-model-builder/src/site/apt/index.apt

michael-o · 2023-04-19T15:30:28Z

maven-model-builder/src/main/java/org/apache/maven/model/root/DefaultRootLocator.java

+                }
+            }
+        } catch (Exception e) {
+            // Ignore


Can we really swallow this?

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 ?

I can add a comment :-)

Yes, depicting that a later stage will handle this better

Comment added.

michael-o

Looks much better now. Will try this with the IT you have written.

gnodet · 2023-04-19T15:45:21Z

Looks much better now. Will try this with the IT you have written.

I can also deprecate basedir and add a baseDirectory, but this may be better located in a subsequent PR.

michael-o · 2023-04-19T16:48:08Z

Looks much better now. Will try this with the IT you have written.

I can also deprecate basedir and add a baseDirectory, but this may be better located in a subsequent PR.

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

maven-model-builder/src/main/java/org/apache/maven/model/root/DefaultRootLocator.java

michael-o · 2023-04-19T20:03:37Z

@gnodet Please don't forget to update the commit summary and JIRA summary since the property names have changed.

michael-o

Looks good to me now except the summary and the commit description.

…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.

michael-o reviewed Mar 17, 2023

View reviewed changes

maven-embedder/src/main/java/org/apache/maven/cli/CLIReportingUtils.java Show resolved Hide resolved

michael-o self-requested a review March 17, 2023 14:52

apache deleted a comment from w6et Mar 17, 2023

gnodet force-pushed the MNG-7038-toplevel branch from 52cc643 to 1fcefd6 Compare March 20, 2023 10:03

gnodet force-pushed the MNG-7038-toplevel branch from 1fcefd6 to 6d73e3a Compare March 20, 2023 12:50

michael-o requested changes Mar 20, 2023

View reviewed changes

gnodet force-pushed the MNG-7038-toplevel branch from 6d73e3a to 5287c77 Compare March 20, 2023 13:01

gnodet changed the title ~~[MNG-7038] Introducing project.topdir~~ [MNG-7038] Introducing project.rootdir Mar 20, 2023

gnodet changed the title ~~[MNG-7038] Introducing project.rootdir~~ [MNG-7038] Introduce public property to point to a root directory of (multi-module) project Mar 23, 2023

gnodet marked this pull request as ready for review March 23, 2023 05:56

michael-o self-requested a review April 14, 2023 07:57

gnodet force-pushed the MNG-7038-toplevel branch 2 times, most recently from a943eac to 0b042e0 Compare April 14, 2023 09:21

This was referenced Apr 14, 2023

[MNG-6303] Interpolate user supplied properties and command line arguments #1062

Merged

[MNG-5659] Provide project-specific settings located in ${session.rootdir}/.mvn/settings.xml by default #1059

Merged

elharo reviewed Apr 18, 2023

View reviewed changes

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

michael-o requested changes Apr 19, 2023

View reviewed changes

maven-model-builder/src/site/apt/index.apt Outdated Show resolved Hide resolved

michael-o requested changes Apr 19, 2023

View reviewed changes

michael-o reviewed Apr 19, 2023

View reviewed changes

maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java Outdated Show resolved Hide resolved

elharo reviewed Apr 19, 2023

View reviewed changes

maven-model-builder/src/main/java/org/apache/maven/model/root/DefaultRootLocator.java Outdated Show resolved Hide resolved

michael-o self-requested a review April 20, 2023 06:55

michael-o approved these changes Apr 20, 2023

View reviewed changes

gnodet force-pushed the MNG-7038-toplevel branch from d959146 to ce50ee7 Compare April 20, 2023 08:59

gnodet merged commit 2db7c85 into apache:master Apr 20, 2023

gnodet changed the title ~~[MNG-7038] Introduce public property to point to a root directory of (multi-module) project~~ [MNG-7038] Introduce public properties to point to to the root and top directory of (multi-module) project Apr 20, 2023

gnodet added this to the 4.0.0-alpha-6 milestone May 17, 2023

gnodet deleted the MNG-7038-toplevel branch November 18, 2023 21:48

[MNG-7038] Introduce public properties to point to to the root and top directory of (multi-module) project #1061

[MNG-7038] Introduce public properties to point to to the root and top directory of (multi-module) project #1061

Conversation

gnodet commented Mar 16, 2023 • edited Loading

michael-o commented Mar 17, 2023

gnodet commented Mar 17, 2023

michael-o commented Mar 17, 2023

gnodet commented Mar 18, 2023

rmannibucau commented Mar 18, 2023

gnodet commented Mar 18, 2023

rmannibucau commented Mar 18, 2023 • edited Loading

gnodet commented Mar 18, 2023

rmannibucau commented Mar 18, 2023

gnodet commented Mar 18, 2023 • edited Loading

rmannibucau commented Mar 18, 2023

gnodet commented Mar 19, 2023

gnodet commented Mar 19, 2023

rmannibucau commented Mar 19, 2023

gnodet commented Mar 20, 2023

rmannibucau commented Mar 20, 2023

gnodet commented Mar 20, 2023 • edited Loading

rmannibucau commented Mar 20, 2023

michael-o commented Mar 20, 2023

Choose a reason for hiding this comment

gnodet Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnodet commented Apr 14, 2023

michael-o commented Apr 14, 2023

michael-o commented Apr 17, 2023

gnodet commented Apr 17, 2023

michael-o commented Apr 17, 2023

michael-o commented Apr 18, 2023

elharo commented Apr 18, 2023

michael-o left a comment • edited Loading

Choose a reason for hiding this comment

michael-o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-o left a comment

Choose a reason for hiding this comment

gnodet commented Apr 19, 2023

michael-o commented Apr 19, 2023

michael-o commented Apr 19, 2023

michael-o left a comment • edited Loading

Choose a reason for hiding this comment

gnodet commented Mar 16, 2023 •

edited

Loading

rmannibucau commented Mar 18, 2023 •

edited

Loading

gnodet commented Mar 18, 2023 •

edited

Loading

gnodet commented Mar 20, 2023 •

edited

Loading

gnodet Mar 20, 2023 •

edited

Loading

michael-o left a comment •

edited

Loading

michael-o left a comment •

edited

Loading