michael-o commented on code in PR #827: URL: https://github.com/apache/maven/pull/827#discussion_r995798745
########## maven-core/src/main/java/org/apache/maven/plugin/MavenPluginPrerequisiteChecker.java: ########## @@ -0,0 +1,38 @@ +package org.apache.maven.plugin; + +/* + * 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 java.util.function.Consumer; + +import org.apache.maven.plugin.descriptor.PluginDescriptor; + +/** + * Service responsible for checking if plugin's prerequisites are met. + */ +@FunctionalInterface +public interface MavenPluginPrerequisiteChecker extends Consumer<PluginDescriptor> Review Comment: `Prerequisites` sind it could check multiple ones, no? ########## maven-core/src/main/java/org/apache/maven/plugin/MavenPluginPrerequisiteChecker.java: ########## @@ -0,0 +1,38 @@ +package org.apache.maven.plugin; + +/* + * 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 java.util.function.Consumer; + +import org.apache.maven.plugin.descriptor.PluginDescriptor; + +/** + * Service responsible for checking if plugin's prerequisites are met. + */ +@FunctionalInterface +public interface MavenPluginPrerequisiteChecker extends Consumer<PluginDescriptor> +{ Review Comment: `public` is redundant ########## maven-core/src/main/java/org/apache/maven/plugin/MavenPluginPrerequisiteChecker.java: ########## @@ -0,0 +1,38 @@ +package org.apache.maven.plugin; + +/* + * 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 java.util.function.Consumer; + +import org.apache.maven.plugin.descriptor.PluginDescriptor; + +/** + * Service responsible for checking if plugin's prerequisites are met. + */ +@FunctionalInterface +public interface MavenPluginPrerequisiteChecker extends Consumer<PluginDescriptor> +{ + /** + * + * @param pluginDescriptor + * @throws IllegalStateException in case the checked prerequisite is not met Review Comment: prerequisites ########## maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java: ########## @@ -303,25 +308,36 @@ public MojoDescriptor getMojoDescriptor( Plugin plugin, String goal, List<Remote return mojoDescriptor; } - public void checkRequiredMavenVersion( PluginDescriptor pluginDescriptor ) + + @Override + public void checkPrerequisites( PluginDescriptor pluginDescriptor ) throws PluginIncompatibleException { - String requiredMavenVersion = pluginDescriptor.getRequiredMavenVersion(); - if ( StringUtils.isNotBlank( requiredMavenVersion ) ) - { - try - { - if ( !runtimeInformation.isMavenVersion( requiredMavenVersion ) ) - { - throw new PluginIncompatibleException( pluginDescriptor.getPlugin(), - "The plugin " + pluginDescriptor.getId() - + " requires Maven version " + requiredMavenVersion ); - } - } - catch ( RuntimeException e ) - { - logger.warn( "Could not verify plugin's Maven prerequisite: " + e.getMessage() ); - } + List<IllegalStateException> prerequisiteExceptions = new ArrayList<>(); + prerequisiteCheckers.forEach( c -> + { + try + { + c.accept( pluginDescriptor ); + } + catch ( IllegalStateException e ) + { + prerequisiteExceptions.add( e ); + } + } ); + // aggregate all exceptions + if ( !prerequisiteExceptions.isEmpty() ) + { + String messages = prerequisiteExceptions.stream() + .map( IllegalStateException::getMessage ) + .collect( Collectors.joining( ", " ) ); + PluginIncompatibleException pie = new PluginIncompatibleException( pluginDescriptor.getPlugin(), + "The plugin " + pluginDescriptor.getId() + + " has unmet requirements: " + + messages, prerequisiteExceptions.get( 0 ) ); + // the first exception is added as cause, all other ones as suppressed exceptions + prerequisiteExceptions.stream().skip( 1 ).forEach( pie::addSuppressed ); + throw pie; Review Comment: lambda pr0n! ########## maven-core/src/main/java/org/apache/maven/plugin/internal/MavenPluginJavaPrerequisiteChecker.java: ########## @@ -0,0 +1,52 @@ +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 javax.inject.Singleton; + +import org.apache.maven.model.profile.activation.JdkVersionProfileActivator; +import org.apache.maven.plugin.MavenPluginPrerequisiteChecker; +import org.apache.maven.plugin.descriptor.PluginDescriptor; +import org.codehaus.plexus.util.StringUtils; + +@Named +@Singleton +public class MavenPluginJavaPrerequisiteChecker + implements MavenPluginPrerequisiteChecker +{ + + @Override + public void accept( PluginDescriptor pluginDescriptor ) + { + String requiredJavaVersion = pluginDescriptor.getRequiredJavaVersion(); + if ( StringUtils.isNotBlank( requiredJavaVersion ) ) + { + String currentJavaVersion = System.getProperty( "java.version" ); Review Comment: Does it use the same approach as the JDK profile activation? ########## maven-core/src/main/java/org/apache/maven/plugin/MavenPluginPrerequisiteChecker.java: ########## @@ -0,0 +1,38 @@ +package org.apache.maven.plugin; + +/* + * 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 java.util.function.Consumer; + +import org.apache.maven.plugin.descriptor.PluginDescriptor; + +/** + * Service responsible for checking if plugin's prerequisites are met. + */ +@FunctionalInterface +public interface MavenPluginPrerequisiteChecker extends Consumer<PluginDescriptor> +{ + /** + * + * @param pluginDescriptor + * @throws IllegalStateException in case the checked prerequisite is not met + */ + void accept( PluginDescriptor pluginDescriptor ) throws IllegalStateException; Review Comment: throws is redundant for runtime exceptions ########## maven-core/src/main/java/org/apache/maven/plugin/internal/MavenPluginMavenPrerequisiteChecker.java: ########## @@ -0,0 +1,74 @@ +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.Inject; +import javax.inject.Named; +import javax.inject.Singleton; + +import org.apache.maven.plugin.MavenPluginPrerequisiteChecker; +import org.apache.maven.plugin.descriptor.PluginDescriptor; +import org.apache.maven.rtinfo.RuntimeInformation; +import org.codehaus.plexus.util.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Named +@Singleton +public class MavenPluginMavenPrerequisiteChecker + implements MavenPluginPrerequisiteChecker +{ + private final Logger logger = LoggerFactory.getLogger( getClass() ); + private final RuntimeInformation runtimeInformation; + + @Inject + public MavenPluginMavenPrerequisiteChecker( RuntimeInformation runtimeInformation ) + { + super(); + this.runtimeInformation = runtimeInformation; + } + + @Override + public void accept( PluginDescriptor pluginDescriptor ) + { + String requiredMavenVersion = pluginDescriptor.getRequiredMavenVersion(); + if ( StringUtils.isNotBlank( requiredMavenVersion ) ) + { + boolean isRequirementMet = false; + try + { + isRequirementMet = runtimeInformation.isMavenVersion( requiredMavenVersion ); + } + catch ( RuntimeException e ) Review Comment: How? The API spec says: @throws IllegalArgumentException If the specified version range is {@code null}, empty or otherwise not a valid version specification. ########## maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java: ########## @@ -303,25 +308,36 @@ public MojoDescriptor getMojoDescriptor( Plugin plugin, String goal, List<Remote return mojoDescriptor; } - public void checkRequiredMavenVersion( PluginDescriptor pluginDescriptor ) + + @Override + public void checkPrerequisites( PluginDescriptor pluginDescriptor ) throws PluginIncompatibleException { - String requiredMavenVersion = pluginDescriptor.getRequiredMavenVersion(); - if ( StringUtils.isNotBlank( requiredMavenVersion ) ) - { - try - { - if ( !runtimeInformation.isMavenVersion( requiredMavenVersion ) ) - { - throw new PluginIncompatibleException( pluginDescriptor.getPlugin(), - "The plugin " + pluginDescriptor.getId() - + " requires Maven version " + requiredMavenVersion ); - } - } - catch ( RuntimeException e ) - { - logger.warn( "Could not verify plugin's Maven prerequisite: " + e.getMessage() ); - } + List<IllegalStateException> prerequisiteExceptions = new ArrayList<>(); + prerequisiteCheckers.forEach( c -> + { + try + { + c.accept( pluginDescriptor ); + } + catch ( IllegalStateException e ) + { + prerequisiteExceptions.add( e ); + } + } ); + // aggregate all exceptions + if ( !prerequisiteExceptions.isEmpty() ) + { + String messages = prerequisiteExceptions.stream() + .map( IllegalStateException::getMessage ) + .collect( Collectors.joining( ", " ) ); + PluginIncompatibleException pie = new PluginIncompatibleException( pluginDescriptor.getPlugin(), + "The plugin " + pluginDescriptor.getId() + + " has unmet requirements: " Review Comment: requirements >= prerequisites? -- 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]
