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

gnodet · 2024-07-04T21:48:12Z

No description provided.

michael-o

There is a conceptual problem here. There are only Maven user properties and Java system properties, but not Maven system properties. We are trying to get rid of them. That should clear in a new solution. I wouldn't even try to make system properties right. @slawekjaranowski

gnodet · 2024-07-05T06:43:41Z

There is a conceptual problem here. There are only Maven user properties and Java system properties, but not Maven system properties. We are trying to get rid of them. That should clear in a new solution. I wouldn't even try to make system properties right. @slawekjaranowski

Yes, it makes sense. I've removed the system properties support to only keep maven.properties which are user properties.

michael-o · 2024-07-05T07:46:41Z

There is one conceptual problem I have here: How does this relate to maven.config doesn't it cause confusion? Add even more complexity?

gnodet · 2024-07-05T08:02:08Z

There is one conceptual problem I have here: How does this relate to maven.config doesn't it cause confusion? Add even more complexity?

It's imho, a much better way to provide configuration properties. maven.config can be used to provide command line arguments, but it has a few problems: it does not have the usual multi-level that we have for settings or toolchains (global, user, project). In addition, the syntax is not user friendly, does not support inclusions of other files and does not support substitutions.
Imho, we should keep maven.config for actual CLI arguments and users should use maven.properties to define properties such as https://maven.apache.org/resolver/configuration.html (we should have a similar one for maven).

michael-o · 2024-07-05T09:04:03Z

There is one conceptual problem I have here: How does this relate to maven.config doesn't it cause confusion? Add even more complexity?

It's imho, a much better way to provide configuration properties. maven.config can be used to provide command line arguments, but it has a few problems: it does not have the usual multi-level that we have for settings or toolchains (global, user, project). In addition, the syntax is not user friendly, does not support inclusions of other files and does not support substitutions. Imho, we should keep maven.config for actual CLI arguments and users should use maven.properties to define properties such as https://maven.apache.org/resolver/configuration.html (we should have a similar one for maven).

I see, then we also should educate users when to use what.

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

gnodet · 2024-07-05T09:05:38Z

There is one conceptual problem I have here: How does this relate to maven.config doesn't it cause confusion? Add even more complexity?

It's imho, a much better way to provide configuration properties. maven.config can be used to provide command line arguments, but it has a few problems: it does not have the usual multi-level that we have for settings or toolchains (global, user, project). In addition, the syntax is not user friendly, does not support inclusions of other files and does not support substitutions. Imho, we should keep maven.config for actual CLI arguments and users should use maven.properties to define properties such as https://maven.apache.org/resolver/configuration.html (we should have a similar one for maven).

I see, then we also should educate users when to use what.

Agreed. I suppose enhancing https://maven.apache.org/configure.html would be a first step.

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

michael-o · 2024-07-06T16:19:46Z

I will get back to this tomorrow. This is a crucial change which needs careful design.

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

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

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

maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/MavenBuildTimestamp.java

maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java

michael-o · 2024-07-10T14:38:56Z

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

+# Comma-separated list of files to include.
+# Each item may be enclosed in quotes to gracefully include spaces. Items are trimmed before being loaded.
+# If the first character of an item is a question mark, the load will silently fail if the file does not exist.
+${includes} = "?${maven.user.conf}/maven.properties", \


Since you are quoting now the value, wouldn't it be more natural to put the question mark at the end? Unless you want to use "¿" as well. Maybe qoutes should be mandatory. Worth looking into Tomcat code for ths. I do not remember the motiviation.

I'm not sure quotes are mandatory, but they certainly avoid any problem with spaces inside path.
About the location of the '?', I fear that putting it at end will be less prominent.
It's also the same syntax than for optional profiles, see https://maven.apache.org/guides/introduction/introduction-to-profiles.html#explicit-profile-activation

I see your point. Shouldn't then the question mark be outside of the quotation marks to make the entire space-containing value optional?

Yes, makes sense.

I've added support for moving the '?' before the quotes (both before and after are supported), the default config file moves them before.

maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/MavenBuildTimestamp.java

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

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

slawekjaranowski · 2024-07-10T20:10:10Z

my test in project .mvn/maven.properties with content:

${includes}=default.properties,?env-${env:envName}.properties

looks like ${env...} is not interpolated in project properties

executed:

envName=test mvn ...

It will be great to support user properties in such case to have a possibility:

${includes}=default.properties,?env-${envName}.properties

and

mvn -DenvName=test ...

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

gnodet · 2024-07-11T06:43:03Z

my test in project .mvn/maven.properties with content:
${includes}=default.properties,?env-${env:envName}.properties
looks like ${env...} is not interpolated in project properties

executed:
envName=test mvn ...
It will be great to support user properties in such case to have a possibility:
${includes}=default.properties,?env-${envName}.properties
and
mvn -DenvName=test ...

I've added a unit test showing this now works.

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

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

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

gnodet · 2024-08-17T08:16:47Z

@michael-o @slawekjaranowski @cstamas any more comments on that one ?

michael-o · 2024-08-17T08:24:52Z

@michael-o @slawekjaranowski @cstamas any more comments on that one ?

Will give it a spin again today

michael-o

What is lacking completely is documentation how feature rich this approach is with inclusiion and optional files w/o reading source code.

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

michael-o · 2024-08-17T16:08:45Z

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

+        userProperties.stringPropertyNames().stream()
+                .filter(k -> !sys.contains(k))
+                .forEach(k -> System.setProperty(k, userProperties.getProperty(k)));
+    }


We must seriously reconsider this promotion in the future before GA.

The best we should not change a System property at all

I've raised #1661 to assess if there are any failing ITs as a first step.

Invoker and friends are our worst enemies here.

slawekjaranowski

evaluate properties from cmd

gnodet · 2024-08-17T18:06:21Z

evaluate properties from cmd

Could you expand a bit more ?

slawekjaranowski · 2024-08-17T18:10:26Z

evaluate properties from cmd

Could you expand a bit more ?

more in comments: #1595 (review)

#1595 (comment)

here only as reason why I marek PR as require change

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

gnodet · 2024-08-17T20:50:58Z

evaluate properties from cmd

Could you expand a bit more ?

more in comments: #1595 (review)

#1595 (comment)

here only as reason why I marek PR as require change

Thx. I modified the UT and fixed the use case.

slawekjaranowski

Looks ok for me.

We need to review documentation for it at all, generated and manually written.

eolivelli

Great

# Conflicts: # maven-embedder/src/main/java/org/apache/maven/cli/props/MavenProperties.java

michael-o requested changes Jul 5, 2024

View reviewed changes

michael-o reviewed Jul 5, 2024

View reviewed changes

apache-maven/src/assembly/maven/conf/maven.properties Outdated Show resolved Hide resolved

michael-o requested changes Jul 5, 2024

View reviewed changes

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

gnodet force-pushed the MNG-7914-config-properties branch from c7c6455 to f658846 Compare July 5, 2024 13:15

gnodet mentioned this pull request Jul 5, 2024

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

Closed

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

slawekjaranowski self-requested a review July 6, 2024 11:05

gnodet force-pushed the MNG-7914-config-properties branch from f658846 to 0cb4067 Compare July 10, 2024 11:11

michael-o reviewed Jul 10, 2024

View reviewed changes

apache-maven/src/assembly/maven/conf/maven.properties Outdated Show resolved Hide resolved

michael-o requested changes Jul 10, 2024

View reviewed changes

gnodet force-pushed the MNG-7914-config-properties branch 3 times, most recently from 4481525 to 72e905d Compare July 10, 2024 14:00

michael-o requested changes Jul 10, 2024

View reviewed changes

slawekjaranowski reviewed Jul 10, 2024

View reviewed changes

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

slawekjaranowski reviewed Jul 10, 2024

View reviewed changes

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

slawekjaranowski reviewed Jul 10, 2024

View reviewed changes

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

michael-o reviewed Jul 11, 2024

View reviewed changes

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

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

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

slawekjaranowski reviewed Jul 11, 2024

View reviewed changes

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

slawekjaranowski reviewed Jul 11, 2024

View reviewed changes

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

gnodet mentioned this pull request Jul 12, 2024

[MNG-8181] Provide a variable for maven central repo url, backed by an environment variable #1615

Merged

slawekjaranowski requested review from michael-o and slawekjaranowski August 17, 2024 08:46

michael-o requested changes Aug 17, 2024

View reviewed changes

slawekjaranowski requested changes Aug 17, 2024

View reviewed changes

michael-o requested changes Aug 17, 2024

View reviewed changes

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

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

slawekjaranowski approved these changes Aug 18, 2024

View reviewed changes

michael-o approved these changes Aug 18, 2024

View reviewed changes

eolivelli approved these changes Aug 18, 2024

View reviewed changes

gnodet added 4 commits August 22, 2024 15:22

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

6df179d

[MNG-7914] Rename global -> installation

187aab6

[MNG-7914] Include time zone in Maven build timestamp

6a9f682

[MNG-7914] Use a single place to document all maven properties

d5946f0

# Conflicts: # maven-embedder/src/main/java/org/apache/maven/cli/props/MavenProperties.java

gnodet force-pushed the MNG-7914-config-properties branch from 8ef8f9e to d5946f0 Compare August 22, 2024 13:22

gnodet merged commit 6ac914d into apache:master Aug 22, 2024
13 checks passed

gnodet mentioned this pull request Aug 24, 2024

[MNG-7914] Add doc for the new maven.properties file #1674

Merged

gnodet deleted the MNG-7914-config-properties branch August 30, 2024 14:28

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gnodet commented Jul 4, 2024

michael-o left a comment •

edited

Loading

gnodet commented Jul 5, 2024

michael-o commented Jul 5, 2024

gnodet commented Jul 5, 2024

michael-o commented Jul 5, 2024

gnodet commented Jul 5, 2024

michael-o commented Jul 6, 2024

michael-o Jul 10, 2024

gnodet Jul 10, 2024

michael-o Jul 10, 2024

gnodet Jul 10, 2024

gnodet Jul 10, 2024

slawekjaranowski commented Jul 10, 2024

gnodet commented Jul 11, 2024

gnodet commented Aug 17, 2024

michael-o commented Aug 17, 2024

michael-o left a comment

michael-o Aug 17, 2024

slawekjaranowski Aug 17, 2024

gnodet Aug 17, 2024

michael-o Aug 17, 2024

slawekjaranowski left a comment

gnodet commented Aug 17, 2024

slawekjaranowski commented Aug 17, 2024 •

edited

Loading

gnodet commented Aug 17, 2024

slawekjaranowski left a comment

eolivelli left a comment

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

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

Conversation

gnodet commented Jul 4, 2024

michael-o left a comment • edited Loading

Choose a reason for hiding this comment

gnodet commented Jul 5, 2024

michael-o commented Jul 5, 2024

gnodet commented Jul 5, 2024

michael-o commented Jul 5, 2024

gnodet commented Jul 5, 2024

michael-o commented Jul 6, 2024

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

slawekjaranowski commented Jul 10, 2024

gnodet commented Jul 11, 2024

gnodet commented Aug 17, 2024

michael-o commented Aug 17, 2024

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

slawekjaranowski left a comment

Choose a reason for hiding this comment

gnodet commented Aug 17, 2024

slawekjaranowski commented Aug 17, 2024 • edited Loading

gnodet commented Aug 17, 2024

slawekjaranowski left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

michael-o left a comment •

edited

Loading

slawekjaranowski commented Aug 17, 2024 •

edited

Loading