rmannibucau commented on a change in pull request #565: URL: https://github.com/apache/maven/pull/565#discussion_r721141235
########## File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/logging/LogFactory.java ########## @@ -0,0 +1,62 @@ +package org.apache.maven.plugin.logging; + +/* + * 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. + */ + +import org.apache.maven.plugin.logging.internal.ILogFactory; + +/** + * This interface supplies {@link Log} instances to Mojos. Plugin code may freely use this log factory to obtain + * loggers that will be abridged to Maven internal logging system. + * + * @since TBD + */ +public final class LogFactory +{ + private static ILogFactory bridge; + + private LogFactory() + { + // no instances of this can be created + } + + /** + * Initialized Mojo LogFactory with a bridge. + */ + public static void initLogFactory( ILogFactory bridge ) Review comment: shouldn't be exposed IMO and we should ensure it is called very early in maven lifecycle + calling it twice with a different value should likely fail. Also means the log factory shouldnt call it in its constructor otherwise we can't change it after, no? ########## File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/Mojo.java ########## @@ -48,25 +46,4 @@ */ void execute() throws MojoExecutionException, MojoFailureException; - Review comment: not sure it makes much sense to drop them, the setLog is useful to keep a lazy init but more importantly, both are used by mojo so it will reduce migration cost (which is not handlable at maven level) to not break them IMHO ########## File path: maven-core/src/main/java/org/apache/maven/plugin/internal/MojoLogFactory.java ########## @@ -0,0 +1,56 @@ +package org.apache.maven.plugin.internal; + +/* + * 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. + */ + +import javax.inject.Named; + +import org.apache.maven.plugin.logging.Log; +import org.apache.maven.plugin.logging.LogFactory; +import org.apache.maven.plugin.logging.internal.ILogFactory; +import org.eclipse.sisu.EagerSingleton; +import org.slf4j.LoggerFactory; + +/** + * Bridge implementation for Mojo {@link Log}. + * + * @since TBD + */ +@EagerSingleton +@Named +public class MojoLogFactory + implements ILogFactory +{ + public MojoLogFactory() + { + LogFactory.initLogFactory( this ); // init Mojo Logging + } + + @Override + public Log getLog( final Class<?> clazz ) + { + return new MojoLog( LoggerFactory.getLogger( clazz ) ); Review comment: maybe lazy lookup the actual underlying logger since SimpleLoggerFactory#loggerMap will grow more than needed since some mojo don't use logging at all. Also wonder if we should provide a cleanup callback to drop the loggers once the mojo is dropped (likely requires some reference counting but will avoid any leak), wdyt? ########## File path: maven-plugin-api/src/main/java/org/apache/maven/plugin/AbstractMojo.java ########## @@ -144,55 +144,39 @@ implements Mojo, ContextEnabled { /** Instance logger */ - private Log log; + private final Log log = LogFactory.getLog( getClass() ); Review comment: open question: makes the lookup earlier, not sure it is good in all cases + it now prevents to reset the log which was sometimes done with extension components -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
