[MNG-8512] Fix org.slf4j.simpleLogger.defaultLogLevel Configuration conflict by CrazyHZM · Pull Request #2042 · apache/maven

CrazyHZM · 2025-01-12T07:58:16Z

JIRA issue: MNG-8512

elharo

I know it's tricky with system properties, but anyway this can be tested? Maybe with an IT?

Also, rerun the failing CI suite. It's known flaky.

elharo · 2025-01-12T12:50:30Z

impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java

+        String current = System.getProperty(defaultLogLevelKey);
+        if (current == null) {
+            System.setProperty(defaultLogLevelKey, value);
+        } else {


what if the levels are the same? Then there shouldn't be a warning.

When the value is the same, It doesn't cause -Dorg slf4j.SimpleLogger.DefaultLogLevel configuration affects the maven command line configuration failure.
@elharo

I'm not sure I follow. Can you rephrase?

For example, we run the command mvn clean install -Dorg slf4j.SimpleLogger.DefaultLogLevel -X, it will lead to effective -X, and -Dorgslf4j.SimpleLogger.DefaultLogLevel failure (I think I said against the above)

You're probably correct, but I do not understand what you are saying.

➜ maven git:(feat/MNG-8512) ✗ ~/work/git/maven/apache-maven/target/apache-maven-4.0.0-rc-3-SNAPSHOT/bin/mvn -X -Dorg.slf4j.simpleLogger.defaultLogLevel=debug [WARNING] System property 'org.slf4j.simpleLogger.defaultLogLevel' is already set to 'debug' - ignoring log level from -X/-e/-q options Apache Maven 4.0.0-rc-3-SNAPSHOT (cc9dc285f04d254e786b9e727ebea1d368bca400) ...

I think what @elharo means is that we don't want to print a warning if we're using -X and the system property is already set to the correct value.

Maybe we should compare and use the lowest level ? And simply ignore if the two values are the same?

Actually, I'm not sure we want user properties to take precedence over CLI options.
We use the same behavior for color for example:

maven/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java

Lines 209 to 212 in bebc3d4

String styleColor = mavenOptions

.color()

.orElse(userProperties.getOrDefault(

Constants.MAVEN_STYLE_COLOR_PROPERTY, userProperties.getOrDefault("style.color", "auto")));

We still need to tell the user that the two configurations conflict, but changing to the INFO level makes more sense. What do you think?
cc @gnodet @elharo

Yes, you're right. That could be done in a single call as System.setProperty returns the previous value. So we could check and if non null and different from the new value, log something at info level.

impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java

elharo · 2025-01-13T14:22:16Z

impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java

+        String current = System.getProperty(defaultLogLevelKey);
+        if (current == null) {
+            System.setProperty(defaultLogLevelKey, value);
+        } else {


I'm not sure I follow. Can you rephrase?

elharo · 2025-01-13T14:23:29Z

impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java

+            System.setProperty(defaultLogLevelKey, value);
+        } else {
+            LOGGER.warn(
+                    "System property '{}' is already set to '{}' - ignoring log level from -X/-e/-q options",


should we specify what the log level that's being ignored is? that is, add a third parameter?

Also, it's really easy to mix parameters up. Simple string concatenation is preferable.

No third parameter is added here, but now some Maven command-line parameters and the slf4j configuration conflict. I agree that the concatenation is better, but automatic formatting by IDEA is uncomfortable.

Use mvn spotless:apply and it should fix anything IntelliJ does wrong. And you can always edit in vi or whatever plain text editor you prefer.

impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java

gnodet · 2025-01-17T09:53:41Z

impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java

                };
-        System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", value);
+
+        String current = System.getProperty(MavenSimpleLogger.DEFAULT_LOG_LEVEL_KEY);


Suggested change

String current = System.getProperty(MavenSimpleLogger.DEFAULT_LOG_LEVEL_KEY);

String current = System.setProperty(MavenSimpleLogger.DEFAULT_LOG_LEVEL_KEY, value);

if (current != null && !value.equalsIgnoreCase(current)) {

Does this logically seem to be maven command options taking precedence over slf4j config?
@gnodet

Yes, a CLI option seems more explicit to me that a value from system properties. CLI options are usually set by the user for a given invocation, while system properties can come from various sources (also from CLI, that's right).
I think this would be in line with the example I pasted a few days ago:

String styleColor = mavenOptions .color() .orElse(userProperties.getOrDefault( Constants.MAVEN_STYLE_COLOR_PROPERTY, userProperties.getOrDefault("style.color", "auto")));

where the color option comes first from the CLI options, then user properties (in that case, we have the new, then the legacy property).

The log msg also needs to be adjusted.

…onflict Signed-off-by: crazyhzm <[email protected]>

gnodet · 2025-01-22T08:17:57Z

@CrazyHZM feel free to squash/merge this PR.

jira-importer · 2025-07-07T09:09:17Z

Resolve #9377

elharo reviewed Jan 12, 2025

View reviewed changes

gnodet reviewed Jan 13, 2025

View reviewed changes

impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java Outdated Show resolved Hide resolved

CrazyHZM force-pushed the feat/MNG-8512 branch from 41a3ff3 to cc9dc28 Compare January 13, 2025 14:02

elharo reviewed Jan 13, 2025

View reviewed changes

gnodet reviewed Jan 15, 2025

View reviewed changes

impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java Outdated Show resolved Hide resolved

CrazyHZM force-pushed the feat/MNG-8512 branch 3 times, most recently from 2928669 to dfb1acc Compare January 16, 2025 15:03

gnodet reviewed Jan 17, 2025

View reviewed changes

[MNG-8512] Fix org.slf4j.simpleLogger.defaultLogLevel Configuration c…

dc4328b

…onflict Signed-off-by: crazyhzm <[email protected]>

CrazyHZM force-pushed the feat/MNG-8512 branch from dfb1acc to dc4328b Compare January 18, 2025 05:04

gnodet approved these changes Jan 22, 2025

View reviewed changes

CrazyHZM added this to the 4.0.0-rc-3 milestone Jan 22, 2025

CrazyHZM merged commit e9805bb into apache:master Jan 22, 2025
13 checks passed

CrazyHZM deleted the feat/MNG-8512 branch January 22, 2025 14:39

	String styleColor = mavenOptions
	.color()
	.orElse(userProperties.getOrDefault(
	Constants.MAVEN_STYLE_COLOR_PROPERTY, userProperties.getOrDefault("style.color", "auto")));

	String current = System.getProperty(MavenSimpleLogger.DEFAULT_LOG_LEVEL_KEY);
	String current = System.setProperty(MavenSimpleLogger.DEFAULT_LOG_LEVEL_KEY, value);
	if (current != null && !value.equalsIgnoreCase(current)) {

Conversation

CrazyHZM commented Jan 12, 2025 • edited by gnodet Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gnodet Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet commented Jan 22, 2025

Uh oh!

Uh oh!

jira-importer commented Jul 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CrazyHZM commented Jan 12, 2025 •

edited by gnodet

Loading

gnodet Jan 17, 2025 •

edited

Loading