Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MNG-8512] Fix org.slf4j.simpleLogger.defaultLogLevel Configuration conflict #2042

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[MNG-8512] Fix org.slf4j.simpleLogger.defaultLogLevel Configuration conflict #2042

wants to merge 1 commit into from
+16 −2

Conversation

CrazyHZM
Copy link
Member

@CrazyHZM CrazyHZM commented Jan 12, 2025 •
edited by gnodet
Loading

JIRA issue: MNG-8512

elharo
elharo reviewed Jan 12, 2025
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java Outdated
String current = System.getProperty(defaultLogLevelKey);
if (current == null) {
System.setProperty(defaultLogLevelKey, value);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

elharo
elharo reviewed Jan 13, 2025
View reviewed changes
impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java Outdated
String current = System.getProperty(defaultLogLevelKey);
if (current == null) {
System.setProperty(defaultLogLevelKey, value);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java Outdated
System.setProperty(defaultLogLevelKey, value);
} else {
LOGGER.warn(
"System property '{}' is already set to '{}' - ignoring log level from -X/-e/-q options",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@CrazyHZM CrazyHZM force-pushed the feat/MNG-8512 branch 3 times, most recently from 2928669 to dfb1acc Compare January 16, 2025 15:03
gnodet
gnodet reviewed Jan 17, 2025
View reviewed changes
impl/maven-cli/src/main/java/org/apache/maven/cling/logging/impl/MavenSimpleConfiguration.java Outdated
@@ -37,7 +41,14 @@ public void setRootLoggerLevel(Level level) {
case INFO -> "info";
default -> "error";
};
System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", value);

String current = System.getProperty(MavenSimpleLogger.DEFAULT_LOG_LEVEL_KEY);
Copy link
Contributor

@gnodet gnodet Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log msg also needs to be adjusted.

@gnodet
Copy link
Contributor

gnodet commented Jan 22, 2025

@CrazyHZM feel free to squash/merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elharo elharo elharo left review comments

@gnodet gnodet gnodet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CrazyHZM @gnodet @elharo