Add some javadoc / website doc to known maven properties #1597

gnodet · 2024-07-05T12:59:54Z

Follow-up to #1595

This PR adds an annotation, a class in the API to contain all config properties, and add a doc generation for those.

gnodet · 2024-07-05T13:21:50Z

Missing:

maven/maven-api-impl/src/main/java/org/apache/maven/internal/impl/resolver/RemoteSnapshotMetadataGenerator.java

Line 49 in d075fe7

Object bn = ConfigUtils.getObject(session, null, "maven.buildNumber");

gnodet · 2024-07-05T19:37:46Z

@michael-o Defining maven.user.home = ${user.home}/.maven should move the repository, toolchains and other config bits to a different place in the user home directory (maybe some are missing) .

michael-o · 2024-07-05T19:44:00Z

@michael-o Defining maven.user.home = ${user.home}/.maven should move the repository, toolchains and other config bits to a different place in the user home directory (maybe some are missing) .

maven.user.home implies that there might be other properties with maven.user. prefix. I doubt that we should do that. I think that maven.userdir should be better.

Both refer to it as config dir:

michael-o · 2024-07-05T19:45:32Z

apache-maven/src/assembly/maven/conf/maven.properties

+# Maven user properties
+#
+
+maven.user.home = ${user.home}/.m2


Based on my previous comment: maven.user.conf? Akin to maven.conf?

for ref (and ideas): https://github.com/maveniverse/mima/blob/main/context/src/main/java/eu/maveniverse/maven/mima/context/MavenUserHome.java

Meaning, I am 100% with value key name.

That wouldn't be consistent with the global props. All of those XML files are config files, at the end.

I disagree, as that IS "maven user home", so "user home" plus something, local repo is not config, is it?

Plus, that is where mvnd daemons logs, where indexer put some files, resolver put file locks, and so on... it is IMHO really "maven user home".

I disagree, as that IS "maven user home", so "user home" plus something, local repo is not config, is it?

It is not, and this is the reason why it has its own property to be relocated. For technical reasons we can only assume the user home to be writable, but that can be a problem if it is NFS mounted or similar.

Plus, that is where mvnd daemons logs, where indexer put some files, resolver put file locks, and so on... it is IMHO really "maven user home".

Then we need maven.user.workdir or similar.

@cstamas The problem you describe has been solved by Windows with APPDATA\{Roaming,Local,LocalLow} the the conf files will roam, but the generated stuff will be in Local, if not even LocalLow, thus never be network mounted.

gnodet · 2024-07-05T19:55:15Z

@michael-o I don't have a strong opinion on the naming, but I'd really like them to be consistent.
The properties file does not contain all of them, so maybe looking at the new Constants class would be better.
And I agree we don't always have the 3 conf levels clearly explicited. The naming is not always homogeneous (I've renamed a few ones that were introduced in 4.x, so no real breakage).
To rename old ones (such as maven.conf -> maven.global.conf), we need to keep the previous one and make it an alias somehow. This is doable.

So if we can come up with clear rules, that would be the best imho.

michael-o · 2024-07-05T19:56:49Z

@michael-o I don't have a strong opinion on the naming, but I'd really like them to be consistent. The properties file does not contain all of them, so maybe looking at the new Constants class would be better. And I agree we don't always have the 3 conf levels clearly explicited. The naming is not always homogeneous (I've renamed a few ones that were introduced in 4.x, so no real breakage). To rename old ones (such as maven.conf -> maven.global.conf), we need to keep the previous one and make it an alias somehow. This is doable.

So if we can come up with clear rules, that would be the best imho.

We should strive for repeatable consistency.

cstamas · 2024-07-05T21:16:55Z

I would perform these renames... these "options" were hidden (like maven.buildNumber, am sure, was used by me only, as it was nor even documented). So no harm, but we have 3.x to 4.x change as well, so I'd do all to make those "pretty clear", and many of those do need rename as were mostly ad-hoc named as 4.x was done....

michael-o · 2024-07-05T21:19:02Z

I would perform these renames... these "options" were hidden (like maven.buildNumber, am sure, was used by me only, as it was nor even documented). So no harm, but we have 3.x to 4.x change as well, so I'd do all to make those "pretty clear", and many of those do need rename as were mostly ad-hoc named as 4.x was done....

Which exactly do you mean?

gnodet · 2024-07-06T05:35:42Z

What about:

maven.home
maven.global.conf = ${maven.home}/conf (we deprecate maven.conf and use it as an alias to maven.global.conf)
maven.user.conf = ${user.home}/.m2
maven.project.conf = ${session.rootDirectory}/.mvn
maven.global.toolchains = ${maven.global.conf}/toolchains.xml
maven.user.toolchains = ${maven.user.conf}/toolchains.xml (we don't have project toolchains)
maven.global.extensions = ${maven.global.conf}/extensions.xml
maven.user.extensions = ${maven.user.conf}/extensions.xml
maven.project.extensions = ${maven.project.conf}/extensions

cstamas · 2024-07-09T08:39:10Z

One question: before it was "maven system home" and "maven user home"... why "global" now? IMHO is not global for sure, especially as you can have multiple maven versions in multiple terminals (like with sdkman) and as single user. So "global" is kinda misleading to me. I like more "maven system..."

cstamas · 2024-07-09T08:39:45Z

But overall, this PR is something that was missing a LOT, this is very cool!

cstamas · 2024-07-09T08:45:06Z

Also one nit: @since is missing from output, but as I see constants are lacking them (hence generated doco is missing them as well).

michael-o · 2024-07-09T14:35:40Z

One question: before it was "maven system home" and "maven user home"... why "global" now? IMHO is not global for sure, especially as you can have multiple maven versions in multiple terminals (like with sdkman) and as single user. So "global" is kinda misleading to me. I like more "maven system..."

Regarding this: we have discussed this notion last year with @rfscholte and came up with https://cwiki.apache.org/confluence/display/MAVEN/Commandline+inheritance. Globals to the installation. We don't have inter-Maven config like other applications have.

cstamas · 2024-07-09T14:51:11Z

This is all about "precedence". Take for example settings:

installation provides one (default system settings)
user may provide one (default user settings)

but both can be overridden, so you could have "inter-maven" settings like "workstation specific" (for example all uers of given workstation access network in same way, so HTTP proxy as example) + "user specific" (users use user specific credentials).

IMHO, this is more about specificity and/or precedence... so "user" MAY override things from "system", the final settings is built by "layering" in proper order.

michael-o

Yet another stupid comment to pick up @cstamas' concerns: Since the user home contains handwritten conf and generated stuff, wouldn't it rather wise to introduce a new property, say maven.user.work which will point by default to maven.user.home, but can be remounted to host all the stuff like local repo, wrapper, indexes, etc? Those who do not want to have something like this on a network mount.

michael-o · 2024-07-09T14:37:20Z

apache-maven/src/assembly/maven/conf/settings.xml

@@ -29,14 +29,14 @@ under the License.
 |
 |                 -s /path/to/user/settings.xml
 |
- |  2. Global Level. This settings.xml file provides configuration for all Maven
+ |  2. System Level. This settings.xml file provides configuration for all Maven


I consider this a wrong as "global". See https://cwiki.apache.org/confluence/display/MAVEN/Commandline+inheritance. It should be "installation" level.

And what if I have defined on workstation MAVEN_ARGS=-gs=/etc/maven/settings.xml? Is that still "installation" level? Moreover. what if that env variable is defined in /etc/profile.d ?

Well, that is an override. It will then instruct the currently running Maven installation use that settings file. It could be in the profile or just in the current shell session, but there is no explicit system wide settings for all Maven installations by default. This is what I mean.

I see that differently. We have "system" and "user" settings, and the utter difference between the two is precedence. Whatever they come from, or are they "default" or "override", their precedence is all that matters.

This is very much alike how we build "effective POM/models", Maven does not care from where the bits come (is current POM in local repo, or in reactor, is parent POM resolved from remote...), what Maven cares is HOW the final result is assembled.

FQN is "Maven System Level". We discussed here in context of Maven. So "Maven User Level" and "Maven System Level".

As in 'Maven System' and not Maven in the entire system (OS, etc)?

@gnodet What I like is you are picking up the idea of https://github.com/apache/tomcat/blob/main/conf/catalina.properties#L33.

Honestly, I cannot decipher it: ${cli:it:-${cli:gt:-${maven.install.conf}/toolchains.xml}} Takes too much brain muscles from first glance.

I wonder why those XML files cannot be optional by default and Maven would simply iterate over

?${maven.install.extensions} \ ?${maven.project.extensions} \ ?${maven.user.extensions}

At the end we define the semantics how a property is interpreted. Git or Subversion do search for a user and global config and it is not a failure if both isn't there. Same here. We could log the absence at debug level. WDYT?
This would make the ? redudant.

I'm fine with removing the ? and assuming all those files are optional.

But that does not address CLI parameters. Right now, they are supported by dedicated code in MavenCli. The code would check if -t is set, and set the user toolchain file accordingly, else compute the default. It's one OR is the other, not both. So we need a way to have default values in the syntax, that's what the following would do:

maven.user.toolchains.default = ${maven.user.conf}/toolchains.xml} maven.user.toolchains = ${cli:t:-${maven.user.toolchains.default}} maven.toolchains = ${maven.install.toolchains} \ ${maven.project.toolchains} \ ${maven.user.toolchains}

Actually, we may still need the ?. Currently, if you specify -t foo.xml, maven will fail if foo.xml file does not exist. Whereas default files can be safely ignored if they don't exist. So we need to be able to know if a file was explicitely specified or just defaulted imho. So maybe:

maven.user.toolchains = ${cli:t:-?${maven.user.toolchains.default}}

Note that we could use cli.t instead of cli:t, this may be less confusing:

maven.user.toolchains = ${cli.t:-?${maven.user.toolchains.default}}

One problem is that project settings are validated differently than user or install settings... If we have a single list, it will be hard to keep that information. I think this is the only case where those xml files (toolchains, settings, extensions) are loaded differently.

Another problem which is not currently addressed, is the merge of extensions.xml, but that's a different topic...

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

michael-o · 2024-07-09T14:39:39Z

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

+     * @since 3.0.0
+     */
+    @Config(source = Config.Source.MODEL, defaultValue = "yyyy-MM-dd'T'HH:mm:ss'Z'")
+    public static final String MAVEN_BUILD_TIMESTAMP_FORMAT = "maven.build.timestamp.format";


One of the problems with this is that the timezone (offset) is missing here. It should be either mentioned or we should switch to yyyy-MM-dd'T'HH:mm:ssXXX which will produce the same output as with Etc/UTC.

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

michael-o · 2024-07-09T14:43:30Z

maven-docgen/src/main/java/org/apache/maven/tools/CollectConfiguration.java

+                type = type.substring(javaLangPackage.length());
+            }
+        }
+        return nvl(type, "n/a");


michael-o · 2024-07-09T14:47:07Z

maven-embedder/src/main/java/org/apache/maven/cli/props/Properties.java

+ * Enhancement of the standard <code>Properties</code>
+ * managing the maintain of comments, etc.
+ */
+public class Properties extends AbstractMap<String, String> {


Maven this class should be called MavenProperties to avoid confusion?

michael-o · 2024-07-09T14:48:41Z

maven-embedder/src/main/java/org/apache/maven/cli/props/Properties.java

+     *
+     * @param str  the <code>String</code> to unescape, may be null
+     * @return the processed string
+     * @throws IllegalArgumentException if the Writer is <code>null</code>


Which writer?

michael-o · 2024-07-09T14:48:50Z

maven-embedder/src/main/java/org/apache/maven/cli/props/Properties.java

+                        inUnicode = false;
+                        hadSlash = false;
+                    } catch (NumberFormatException nfe) {
+                        throw new IllegalArgumentException("Unable to parse unicode value: " + unicode, nfe);


Does this require documentation?

michael-o · 2024-07-09T14:52:49Z

maven-embedder/src/main/java/org/apache/maven/cli/props/PropertiesLoader.java

+
+    public static final String INCLUDES_PROPERTY = "${includes}"; // mandatory includes
+
+    public static final String OPTIONALS_PROPERTY = "${optionals}"; // optionals include


Maybe we can make optionals more succint with ${includes?} like we have in here: https://maven.apache.org/plugins/maven-assembly-plugin/faq.html#dashClassifier

This is the property key, you want to rename and use ${includes?} = ${user.home}/.m2/maven.properties ?

Yes, as an idea. Maybe it is a bad one. I am open to ideas. Comments should read "mandatory includes"/"optional includes", but the property names aren't consistent in this regard.

Another possibility is to have a single list, and if the first character is a ?, then make it optional.

${includes} = ?${user.home}/.m2/maven.properties, \ ?${session.rootDirectory}/.mvn/maven.properties

We have this syntax for profiles.

michael-o · 2024-07-09T14:53:59Z

src/site/markdown/configuration.md

+
+| No | Key | Type | Description | Default Value | Since | Source |
+| --- | --- | --- | --- | --- | --- | --- |
+| 1. | `maven.build.timestamp.format` | `String` | Build timestamp format. |  `yyyy-MM-dd'T'HH:mm:ss'Z'`  | 3.0.0 | Model properties |


I think that the desc should contain the time zone information.

Yes, this is generated from the defaultValue above.

cstamas · 2024-07-09T14:57:24Z

Which exactly do you mean?

I meant this one

maven/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/RemoteSnapshotMetadataGenerator.java

Line 49 in fd8f99e

Object bn = ConfigUtils.getObject(session, null, "maven.buildNumber");

But is not present in this PR yet. Essentially, I find this property badly named, that's all. As many other, but renaming these is IMHO out of scope for this PR.

michael-o · 2024-07-09T15:04:27Z

This is all about "precedence". Take for example settings:
* installation provides one (default system settings)

* user may provide one (default user settings)
but both can be overridden, so you could have "inter-maven" settings like "workstation specific" (for example all uers of given workstation access network in same way, so HTTP proxy as example) + "user specific" (users use user specific credentials).

IMHO, this is more about specificity and/or precedence... so "user" MAY override things from "system", the final settings is built by "layering" in proper order.

This I don't understand. While the user settings can be shared between Maven versions, there is no global settings which is sourced by every Maven installation like mavenrc.

michael-o · 2024-07-09T15:05:19Z

Which exactly do you mean?

I meant this one

maven/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/RemoteSnapshotMetadataGenerator.java

Line 49 in fd8f99e

Object bn = ConfigUtils.getObject(session, null, "maven.buildNumber");

But is not present in this PR yet. Essentially, I find this property badly named, that's all. As many other, but renaming these is IMHO out of scope for this PR.

Is there even proper docs for this property?

cstamas · 2024-07-09T15:09:01Z

Is there even proper docs for this property?

See https://issues.apache.org/jira/browse/MNG-8054

…ame system->install

michael-o · 2024-07-10T06:53:17Z

apache-maven/src/assembly/maven/conf/maven.properties

+# The '-is' flag will override the 'maven.install.settings' property.
+# The '-ps' flag will override the 'maven.project.settings' property.
+# The '-s' flag will override the 'maven.user.settings' property.
+maven.install.settings = ${maven.install.conf}/settings.xml


Attention: This must not be confused with the Maven Install Plugin, hence: maven.installation.<prop>

gnodet · 2024-07-10T11:13:49Z

Closing this PR in favour of #1595

[MNG-7914] Provide a single entry point for configuration

f658846

gnodet force-pushed the docgen branch from 73fc2b2 to e204221 Compare July 5, 2024 19:31

gnodet requested review from cstamas, michael-o and slawekjaranowski July 5, 2024 19:39

michael-o reviewed Jul 5, 2024

View reviewed changes

gnodet force-pushed the docgen branch from e204221 to 91a6149 Compare July 5, 2024 19:46

Refactor constants and generate doc for those

efb32c2

gnodet force-pushed the docgen branch from 91a6149 to efb32c2 Compare July 5, 2024 19:59

Revert change

044270b

gnodet added this to the 4.0.0-beta-4 milestone Jul 6, 2024

gnodet modified the milestones: 4.0.0-beta-4, 4.0.0 Jul 6, 2024

gnodet added 2 commits July 9, 2024 13:35

Rename global -> system in the API

f15f0dd

Add since tags

4163750

gnodet force-pushed the docgen branch from fa8d862 to 4163750 Compare July 9, 2024 11:57

Also support maven.conf system property not being set

d156c54

michael-o requested changes Jul 9, 2024

View reviewed changes

cstamas approved these changes Jul 9, 2024

View reviewed changes

Keep the 3 levels with individual properties, fixes after review, ren…

d74513d

…ame system->install

michael-o requested changes Jul 10, 2024

View reviewed changes

gnodet added 5 commits July 10, 2024 09:23

Rename install->installation

6fcdd82

Fixes

268009b

Fix xml

e1ee17a

Fix xml

f2d7922

Improvements

f9c5514

gnodet closed this Jul 10, 2024


		public static final String INCLUDES_PROPERTY = "${includes}"; // mandatory includes

		public static final String OPTIONALS_PROPERTY = "${optionals}"; // optionals include

Add some javadoc / website doc to known maven properties #1597

Add some javadoc / website doc to known maven properties #1597

Conversation

gnodet commented Jul 5, 2024 • edited Loading

gnodet commented Jul 5, 2024

gnodet commented Jul 5, 2024

michael-o commented Jul 5, 2024

michael-o Jul 5, 2024 • 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

cstamas Jul 5, 2024 • 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

gnodet commented Jul 5, 2024

michael-o commented Jul 5, 2024

cstamas commented Jul 5, 2024

michael-o commented Jul 5, 2024

gnodet commented Jul 6, 2024

cstamas commented Jul 9, 2024

cstamas commented Jul 9, 2024

cstamas commented Jul 9, 2024

michael-o commented Jul 9, 2024

cstamas commented Jul 9, 2024 • edited Loading

michael-o left a comment

Choose a reason for hiding this comment

michael-o Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

cstamas Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

michael-o Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

cstamas Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

cstamas Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-o Jul 9, 2024 • 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

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 Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas commented Jul 9, 2024

michael-o commented Jul 9, 2024

michael-o commented Jul 9, 2024

cstamas commented Jul 9, 2024

Choose a reason for hiding this comment

gnodet commented Jul 10, 2024

gnodet commented Jul 5, 2024 •

edited

Loading

michael-o Jul 5, 2024 •

edited

Loading

cstamas Jul 5, 2024 •

edited

Loading

cstamas commented Jul 9, 2024 •

edited

Loading

michael-o Jul 9, 2024 •

edited

Loading

cstamas Jul 9, 2024 •

edited

Loading

michael-o Jul 9, 2024 •

edited

Loading

cstamas Jul 9, 2024 •

edited

Loading

cstamas Jul 9, 2024 •

edited

Loading

michael-o Jul 9, 2024 •

edited

Loading

gnodet Jul 9, 2024 •

edited

Loading