You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "rmannibucau (via GitHub)" <gi...@apache.org> on 2023/03/07 19:39:42 UTC
[GitHub] [maven] rmannibucau opened a new pull request, #1041: [MNG-7724] adjust log level for the message when no known binding is found in Slf4jConfigurationFactory
rmannibucau opened a new pull request, #1041:
URL: https://github.com/apache/maven/pull/1041
Following this checklist to help us incorporate your
contribution quickly and easily:
- [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] Format the pull request title like `[MNG-XXX] SUMMARY`,
where you replace `MNG-XXX` and `SUMMARY` with the appropriate JIRA issue.
- [X] Also format the first line of the commit message like `[MNG-XXX] SUMMARY`.
Best practice is to use the JIRA issue title in both the pull request title and in the first line of the commit message.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
[core-its]: https://maven.apache.org/core-its/core-it-suite/
The issue with current code is that using a SLF4J binding which is not known by maven will lead to WARNINGs.
There are several builds which intend to run without warning or consider warnings as error and therefore fail - not using maven CLI options but other mecanism - either log analyzis on CI, the old way with appenders/handlers or stdout/stderr check if the handlers support the redirection properly.
This behavior defeats a bit using a logger abstraction since the support is pretty limited.
The idea is to keep the message - we can refine it later if relevant - but log it at info level to not make the build failling when there is no issue loading the impl maven should use and keep warning when a loading failed (we can evaluate the move to error later too for this case).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #1041:
URL: https://github.com/apache/maven/pull/1041#issuecomment-1460420631
Fully agree JUL is way saner for products like maven but this is a long debate.
If we remove it we drop the `--fail-on-severity` flag so not sure we can in 4.x.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] elharo commented on a diff in pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129400840
##########
maven-embedder/src/main/java/org/apache/maven/cli/logging/Slf4jConfigurationFactory.java:
##########
@@ -65,8 +67,14 @@ public static Slf4jConfiguration getConfiguration(ILoggerFactory loggerFactory)
}
} catch (IOException | ClassNotFoundException | IllegalAccessException | InstantiationException e) {
e.printStackTrace();
+ hadException = true;
}
- return new UnsupportedSlf4jBindingConfiguration(slf4jBinding, supported);
+ return new UnsupportedSlf4jBindingConfiguration(
+ slf4jBinding,
+ supported,
Review Comment:
If this is valid, we shouldn't log at all. If it isn't, log the warning. An INFO level log isn't right here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] elharo commented on a diff in pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129424545
##########
maven-embedder/pom.xml:
##########
@@ -140,6 +140,12 @@ under the License.
<artifactId>slf4j-simple</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-jdk14</artifactId>
Review Comment:
Does this introduce JDK 9+ byte code into the classpath? That would be a deal breaker for now.
##########
maven-embedder/src/main/java/org/apache/maven/cli/logging/impl/NoopConfiguration.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cli.logging.impl;
+
+import org.apache.maven.cli.logging.BaseSlf4jConfiguration;
+
+/**
+ * Configuration available for users setting a custom binding and not using --fail-on-severity to avoid warnings.
+ * This is basically a no-op implementation not issuing any warning.
+ * To use it add to maven classpath (m2.conf) a folder/jar with a META-INF/maven/slf4j-configuration.properties
+ * containing {@code loggerClass = org.apache.maven.cli.logging.impl.NoopConfiguration}.
+ */
+public class NoopConfiguration extends BaseSlf4jConfiguration {
Review Comment:
I thought the point of log frameworks was that this sort of thing could be done in client projects without requiring support from projects doing the logging.
##########
maven-embedder/src/main/java/org/apache/maven/cli/logging/impl/JDKConfiguration.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cli.logging.impl;
+
+import java.util.logging.Logger;
+
+import org.apache.maven.cli.logging.BaseSlf4jConfiguration;
+
+/**
+ * Configuration for slf4j-jdk14.
+ */
+public class JDKConfiguration extends BaseSlf4jConfiguration {
Review Comment:
This is additional API I'd rather not carry around. I need to understand the root issue better, but it still seems to me either warning is correct, in which case we should do nothing, or we should remove the log message completely. I don't like half measures and work arounds for misconfigured builds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1041:
URL: https://github.com/apache/maven/pull/1041#issuecomment-1725693996
@rmannibucau can this be closed ? [MNG-7724](http://issues.apache.org/jira/browse/MNG-7724) has been closed with https://github.com/apache/maven/pull/1051
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #1041: [MNG-7724] adjust log level for the message when no known binding is found in Slf4jConfigurationFactory
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129155652
##########
maven-embedder/src/main/java/org/apache/maven/cli/logging/Slf4jConfigurationFactory.java:
##########
@@ -65,8 +67,14 @@ public static Slf4jConfiguration getConfiguration(ILoggerFactory loggerFactory)
}
} catch (IOException | ClassNotFoundException | IllegalAccessException | InstantiationException e) {
e.printStackTrace();
+ hadException = true;
}
- return new UnsupportedSlf4jBindingConfiguration(slf4jBinding, supported);
+ return new UnsupportedSlf4jBindingConfiguration(
+ slf4jBinding,
+ supported,
Review Comment:
Not really, maven is wrong assuming it knows the entire ecosystem, it is perfectly valid to use a slf4j binding maven does not know - or if you are right it means we should drop slf4j from maven because we don't use the abstraction at the end.
If it is right then we must not issue a warning (an error) when it is an expected case - the use case triggering this change.
Guess it was more a default implementation than an intent to say a custom binding is intended.
Assuming any custom binding will bring a custom maven integration is also a high assumption for the little usage we do of that integration (a single CLI toggle which is rarely used in practise) so hope we don't go that path which would be the alternative.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129441351
##########
maven-embedder/pom.xml:
##########
@@ -140,6 +140,12 @@ under the License.
<artifactId>slf4j-simple</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-jdk14</artifactId>
Review Comment:
14 = 1.4, code is from 2006 ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #1041:
URL: https://github.com/apache/maven/pull/1041#issuecomment-1460329789
@elharo jul is jdk14 one (the PR adds its support, was not there not sure why), noop is kind of built-in but defeats the fact to log a warning on a random logger (but this is another ticket/issue I guess). The root cause is really the fact maven adds an integration layer with each slf4j implementation so at the end can't reliably rely on the slf4j abstraction.
For maven - as of today - slf4j binding == slf4j-$impl + maven-slf4j-$impl (in embedder module), this is what can need rework the future cause the relationship of slf4j binding should be sufficient - fully agree with your statement but not current state AFAIK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #1041: [MNG-7724] adjust log level for the message when no known binding is found in Slf4jConfigurationFactory
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129153394
##########
maven-embedder/src/test/java/org/apache/maven/cli/logging/Slf4jConfigurationFactoryTest.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cli.logging;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.BiConsumer;
+
+import org.apache.maven.cli.logging.impl.UnsupportedSlf4jBindingConfiguration;
+import org.junit.jupiter.api.Test;
+import org.slf4j.ILoggerFactory;
+import org.slf4j.Logger;
+
+import static java.util.Collections.singletonList;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.fail;
+
+/**
+ * Validates some edge cases of Slf4jConfigurationFactory.
+ */
+public class Slf4jConfigurationFactoryTest {
+ @Test
+ void logInfoWhenNoBindingIsFound() {
+ final UnsupportedSlf4jBindingConfiguration configuration = assertInstanceOf(
+ UnsupportedSlf4jBindingConfiguration.class,
+ Slf4jConfigurationFactory.getConfiguration(new UnknownLoggerFactory()));
+ final BiConsumer<Logger, String> logHandler = findLogHandler(configuration);
+ final CapturingLogger logger = new CapturingLogger();
+ logHandler.accept(logger, "should be info");
+ assertEquals(singletonList("info"), logger.events);
+ }
+
+ @Test
+ void logWarnWhenNoBindingIsFoundButSomeFailedToLoad() throws IOException {
+ final Path fakeProvider = Paths.get("target/test-classes/META-INF/maven/slf4j-configuration.properties");
+ Files.createDirectories(fakeProvider.getParent());
+ Files.write(
+ fakeProvider,
+ ("org.apache.maven.cli.logging.Slf4jConfigurationFactoryTest.UnknownLoggerFactory will.fail.for.test")
+ .getBytes(StandardCharsets.UTF_8));
+ try {
+ final UnsupportedSlf4jBindingConfiguration configuration = assertInstanceOf(
+ UnsupportedSlf4jBindingConfiguration.class,
+ Slf4jConfigurationFactory.getConfiguration(new UnknownLoggerFactory()));
+ final BiConsumer<Logger, String> logHandler = findLogHandler(configuration);
+ final CapturingLogger logger = new CapturingLogger();
+ logHandler.accept(logger, "should be warn");
+ assertEquals(singletonList("warn"), logger.events);
+ } finally {
+ Files.deleteIfExists(fakeProvider);
+ }
+ }
+
+ private BiConsumer<Logger, String> findLogHandler(final UnsupportedSlf4jBindingConfiguration configuration) {
+ try {
+ final Field log = configuration.getClass().getDeclaredField("log");
+ log.setAccessible(true);
+ return (BiConsumer<Logger, String>) log.get(configuration);
+ } catch (final Exception e) {
Review Comment:
Well this is to make it more readable, having 3 exception there does not help anything since it is really a "pass or fail" thing and handling is the same so for tests this looks a better compromise IMHO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129443535
##########
maven-embedder/src/main/java/org/apache/maven/cli/logging/impl/JDKConfiguration.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cli.logging.impl;
+
+import java.util.logging.Logger;
+
+import org.apache.maven.cli.logging.BaseSlf4jConfiguration;
+
+/**
+ * Configuration for slf4j-jdk14.
+ */
+public class JDKConfiguration extends BaseSlf4jConfiguration {
Review Comment:
well, this one is easy, maven integrates with logback, log4j2, etc so it integrated with slf4j bindings, jdk14 is just a standard one so we must integrate with, no real ambiguity.
now if I didn't get it and you mean maven shouldn't integrate with slf4j binding - I can agree with that at some point - then I'd be happy to drop logback, log4j2 and other integrations too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] elharo commented on a diff in pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129397818
##########
maven-embedder/src/test/java/org/apache/maven/cli/logging/Slf4jConfigurationFactoryTest.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cli.logging;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.BiConsumer;
+
+import org.apache.maven.cli.logging.impl.UnsupportedSlf4jBindingConfiguration;
+import org.junit.jupiter.api.Test;
+import org.slf4j.ILoggerFactory;
+import org.slf4j.Logger;
+
+import static java.util.Collections.singletonList;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.fail;
+
+/**
+ * Validates some edge cases of Slf4jConfigurationFactory.
+ */
+public class Slf4jConfigurationFactoryTest {
+ @Test
+ void logInfoWhenNoBindingIsFound() {
+ final UnsupportedSlf4jBindingConfiguration configuration = assertInstanceOf(
+ UnsupportedSlf4jBindingConfiguration.class,
+ Slf4jConfigurationFactory.getConfiguration(new UnknownLoggerFactory()));
+ final BiConsumer<Logger, String> logHandler = findLogHandler(configuration);
+ final CapturingLogger logger = new CapturingLogger();
+ logHandler.accept(logger, "should be info");
+ assertEquals(singletonList("info"), logger.events);
+ }
+
+ @Test
+ void logWarnWhenNoBindingIsFoundButSomeFailedToLoad() throws IOException {
+ final Path fakeProvider = Paths.get("target/test-classes/META-INF/maven/slf4j-configuration.properties");
+ Files.createDirectories(fakeProvider.getParent());
+ Files.write(
+ fakeProvider,
+ ("org.apache.maven.cli.logging.Slf4jConfigurationFactoryTest.UnknownLoggerFactory will.fail.for.test")
+ .getBytes(StandardCharsets.UTF_8));
+ try {
+ final UnsupportedSlf4jBindingConfiguration configuration = assertInstanceOf(
+ UnsupportedSlf4jBindingConfiguration.class,
+ Slf4jConfigurationFactory.getConfiguration(new UnknownLoggerFactory()));
+ final BiConsumer<Logger, String> logHandler = findLogHandler(configuration);
+ final CapturingLogger logger = new CapturingLogger();
+ logHandler.accept(logger, "should be warn");
+ assertEquals(singletonList("warn"), logger.events);
+ } finally {
+ Files.deleteIfExists(fakeProvider);
+ }
+ }
+
+ private BiConsumer<Logger, String> findLogHandler(final UnsupportedSlf4jBindingConfiguration configuration) {
+ try {
+ final Field log = configuration.getClass().getDeclaredField("log");
+ log.setAccessible(true);
+ return (BiConsumer<Logger, String>) log.get(configuration);
+ } catch (final Exception e) {
Review Comment:
Strongly disagree. Catching Exception here risks pulling in runtime exceptions we're not actually handling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129400007
##########
maven-embedder/src/test/java/org/apache/maven/cli/logging/Slf4jConfigurationFactoryTest.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cli.logging;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.BiConsumer;
+
+import org.apache.maven.cli.logging.impl.UnsupportedSlf4jBindingConfiguration;
+import org.junit.jupiter.api.Test;
+import org.slf4j.ILoggerFactory;
+import org.slf4j.Logger;
+
+import static java.util.Collections.singletonList;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.fail;
+
+/**
+ * Validates some edge cases of Slf4jConfigurationFactory.
+ */
+public class Slf4jConfigurationFactoryTest {
+ @Test
+ void logInfoWhenNoBindingIsFound() {
+ final UnsupportedSlf4jBindingConfiguration configuration = assertInstanceOf(
+ UnsupportedSlf4jBindingConfiguration.class,
+ Slf4jConfigurationFactory.getConfiguration(new UnknownLoggerFactory()));
+ final BiConsumer<Logger, String> logHandler = findLogHandler(configuration);
+ final CapturingLogger logger = new CapturingLogger();
+ logHandler.accept(logger, "should be info");
+ assertEquals(singletonList("info"), logger.events);
+ }
+
+ @Test
+ void logWarnWhenNoBindingIsFoundButSomeFailedToLoad() throws IOException {
+ final Path fakeProvider = Paths.get("target/test-classes/META-INF/maven/slf4j-configuration.properties");
+ Files.createDirectories(fakeProvider.getParent());
+ Files.write(
+ fakeProvider,
+ ("org.apache.maven.cli.logging.Slf4jConfigurationFactoryTest.UnknownLoggerFactory will.fail.for.test")
+ .getBytes(StandardCharsets.UTF_8));
+ try {
+ final UnsupportedSlf4jBindingConfiguration configuration = assertInstanceOf(
+ UnsupportedSlf4jBindingConfiguration.class,
+ Slf4jConfigurationFactory.getConfiguration(new UnknownLoggerFactory()));
+ final BiConsumer<Logger, String> logHandler = findLogHandler(configuration);
+ final CapturingLogger logger = new CapturingLogger();
+ logHandler.accept(logger, "should be warn");
+ assertEquals(singletonList("warn"), logger.events);
+ } finally {
+ Files.deleteIfExists(fakeProvider);
+ }
+ }
+
+ private BiConsumer<Logger, String> findLogHandler(final UnsupportedSlf4jBindingConfiguration configuration) {
+ try {
+ final Field log = configuration.getClass().getDeclaredField("log");
+ log.setAccessible(true);
+ return (BiConsumer<Logger, String>) log.get(configuration);
+ } catch (final Exception e) {
Review Comment:
we do handle them all -> fail, all good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] elharo commented on a diff in pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129448236
##########
maven-embedder/src/main/java/org/apache/maven/cli/logging/impl/JDKConfiguration.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cli.logging.impl;
+
+import java.util.logging.Logger;
+
+import org.apache.maven.cli.logging.BaseSlf4jConfiguration;
+
+/**
+ * Configuration for slf4j-jdk14.
+ */
+public class JDKConfiguration extends BaseSlf4jConfiguration {
Review Comment:
OK, as long as this means JDK 1.4 and not JDK 14.0 no problem here. Time marches on...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] elharo commented on pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #1041:
URL: https://github.com/apache/maven/pull/1041#issuecomment-1460414960
Why does Maven add that layer? Can it simply remove it instead? If I had a greenfield, I'd just use java.util.logging and not allow any other log libraries within ten dependency chains of core, but if wishes were horses...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129421297
##########
maven-embedder/src/main/java/org/apache/maven/cli/logging/Slf4jConfigurationFactory.java:
##########
@@ -65,8 +67,14 @@ public static Slf4jConfiguration getConfiguration(ILoggerFactory loggerFactory)
}
} catch (IOException | ClassNotFoundException | IllegalAccessException | InstantiationException e) {
e.printStackTrace();
+ hadException = true;
}
- return new UnsupportedSlf4jBindingConfiguration(slf4jBinding, supported);
+ return new UnsupportedSlf4jBindingConfiguration(
+ slf4jBinding,
+ supported,
Review Comment:
I rewrote the PR to not play with levels, please check the new proposal to solve this issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] elharo commented on a diff in pull request #1041: [MNG-7724] adjust log level for the message when no known binding is found in Slf4jConfigurationFactory
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1128785518
##########
maven-embedder/src/main/java/org/apache/maven/cli/logging/Slf4jConfigurationFactory.java:
##########
@@ -65,8 +67,14 @@ public static Slf4jConfiguration getConfiguration(ILoggerFactory loggerFactory)
}
} catch (IOException | ClassNotFoundException | IllegalAccessException | InstantiationException e) {
e.printStackTrace();
+ hadException = true;
}
- return new UnsupportedSlf4jBindingConfiguration(slf4jBinding, supported);
+ return new UnsupportedSlf4jBindingConfiguration(
+ slf4jBinding,
+ supported,
Review Comment:
warning seems the right level error. If someone is treating this as an error, that's their choice and problem, not ours.
##########
maven-embedder/src/test/java/org/apache/maven/cli/logging/Slf4jConfigurationFactoryTest.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cli.logging;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.BiConsumer;
+
+import org.apache.maven.cli.logging.impl.UnsupportedSlf4jBindingConfiguration;
+import org.junit.jupiter.api.Test;
+import org.slf4j.ILoggerFactory;
+import org.slf4j.Logger;
+
+import static java.util.Collections.singletonList;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.fail;
+
+/**
+ * Validates some edge cases of Slf4jConfigurationFactory.
+ */
+public class Slf4jConfigurationFactoryTest {
+ @Test
+ void logInfoWhenNoBindingIsFound() {
+ final UnsupportedSlf4jBindingConfiguration configuration = assertInstanceOf(
+ UnsupportedSlf4jBindingConfiguration.class,
+ Slf4jConfigurationFactory.getConfiguration(new UnknownLoggerFactory()));
+ final BiConsumer<Logger, String> logHandler = findLogHandler(configuration);
+ final CapturingLogger logger = new CapturingLogger();
+ logHandler.accept(logger, "should be info");
+ assertEquals(singletonList("info"), logger.events);
+ }
+
+ @Test
+ void logWarnWhenNoBindingIsFoundButSomeFailedToLoad() throws IOException {
+ final Path fakeProvider = Paths.get("target/test-classes/META-INF/maven/slf4j-configuration.properties");
+ Files.createDirectories(fakeProvider.getParent());
+ Files.write(
+ fakeProvider,
+ ("org.apache.maven.cli.logging.Slf4jConfigurationFactoryTest.UnknownLoggerFactory will.fail.for.test")
+ .getBytes(StandardCharsets.UTF_8));
+ try {
+ final UnsupportedSlf4jBindingConfiguration configuration = assertInstanceOf(
+ UnsupportedSlf4jBindingConfiguration.class,
+ Slf4jConfigurationFactory.getConfiguration(new UnknownLoggerFactory()));
+ final BiConsumer<Logger, String> logHandler = findLogHandler(configuration);
+ final CapturingLogger logger = new CapturingLogger();
+ logHandler.accept(logger, "should be warn");
+ assertEquals(singletonList("warn"), logger.events);
+ } finally {
+ Files.deleteIfExists(fakeProvider);
+ }
+ }
+
+ private BiConsumer<Logger, String> findLogHandler(final UnsupportedSlf4jBindingConfiguration configuration) {
+ try {
+ final Field log = configuration.getClass().getDeclaredField("log");
+ log.setAccessible(true);
+ return (BiConsumer<Logger, String>) log.get(configuration);
+ } catch (final Exception e) {
Review Comment:
1, No need to mark this final
2. Make exception specific if possible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #1041:
URL: https://github.com/apache/maven/pull/1041#discussion_r1129447663
##########
maven-embedder/src/main/java/org/apache/maven/cli/logging/impl/NoopConfiguration.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cli.logging.impl;
+
+import org.apache.maven.cli.logging.BaseSlf4jConfiguration;
+
+/**
+ * Configuration available for users setting a custom binding and not using --fail-on-severity to avoid warnings.
+ * This is basically a no-op implementation not issuing any warning.
+ * To use it add to maven classpath (m2.conf) a folder/jar with a META-INF/maven/slf4j-configuration.properties
+ * containing {@code loggerClass = org.apache.maven.cli.logging.impl.NoopConfiguration}.
+ */
+public class NoopConfiguration extends BaseSlf4jConfiguration {
Review Comment:
Fully agree but not how maven integrated the fail on severity flag, just trying to not drop the flag - shouldn't have been introduced since it is 100% a log framework feature for me - and get it running properly without warnings which actually just mention the exact case you setup (not supported by maven slf4j binding).
There are actually two cases assuming we keep this kind of integration:
* maven supports the binding but the loading fails (missing dep - missing logback-classic when adding logback-core for ex or the opposite) -> we should issue a warning
* maven does not know the binding -> guess we can discuss info (user intended to use a custom binding) or warning (user didnt intend to use the binding added to mvn classpath due to an extension transitive dep) but having this noop impl enables maven to issue a warning and the user to drop it when he knows it is not relevant
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau closed pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau closed pull request #1041: [MNG-7724] Don't log warnings when runtime is not broken (slf4j integrations)
URL: https://github.com/apache/maven/pull/1041
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org