rfscholte commented on a change in pull request #16: URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r523726205
########## File path: src/main/java/org/apache/maven/plugins/jlink/JLinkExecutor.java ########## @@ -0,0 +1,171 @@ +package org.apache.maven.plugins.jlink; + +/* + * 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.commons.lang3.SystemUtils; +import org.apache.maven.plugin.logging.Log; +import org.apache.maven.toolchain.Toolchain; +import org.codehaus.plexus.util.StringUtils; +import org.codehaus.plexus.util.cli.CommandLineUtils; +import org.codehaus.plexus.util.cli.Commandline; + +import java.io.File; +import java.io.IOException; +import java.util.Optional; +import java.util.Properties; + +/** + * JDK 8-only Jlink executor. + * + * <p>As JDK8 does not ship jlink, either a toolchain or JAVA_HOME is required.</p> + */ +class JLinkExecutor extends AbstractJLinkExecutor +{ + private final String jLinkExec; + + JLinkExecutor( Toolchain toolchain, Log log ) throws IOException + { + super( toolchain, log ); + this.jLinkExec = getJLinkExecutable(); + } + + public File getJlinkExecutable() + { + return new File( this.jLinkExec ); + } + + @Override + public Optional<File> getJmodsFolder( /* nullable */ File sourceJdkModules ) + { + // Really Hacky...do we have a better solution to find the jmods directory of the JDK? + File jLinkParent = getJlinkExecutable().getParentFile().getParentFile(); + File jmodsFolder; + if ( sourceJdkModules != null && sourceJdkModules.isDirectory() ) + { + jmodsFolder = new File( sourceJdkModules, JMODS ); + } + else + { + jmodsFolder = new File( jLinkParent, JMODS ); + } + + getLog().debug( " Parent: " + jLinkParent.getAbsolutePath() ); + getLog().debug( " jmodsFolder: " + jmodsFolder.getAbsolutePath() ); + + return Optional.of( jmodsFolder ); + } + + /** + * Execute JLink via any means. + * + * @return the exit code ({@code 0} on success). + */ + @Override + public int executeJlink( File argsFile ) + { + getLog().info( "Toolchain in maven-jlink-plugin: jlink [ " + this.jLinkExec + " ]" ); + + Commandline cmd = createJLinkCommandLine( argsFile ); + cmd.setExecutable( this.jLinkExec ); + + throw new UnsupportedOperationException( "not implemented" ); + } + + private Commandline createJLinkCommandLine( File argsFile ) + { + Commandline cmd = new Commandline(); + cmd.createArg().setValue( '@' + argsFile.getAbsolutePath() ); + + return cmd; + } + + + protected final String getJLinkExecutable() throws IOException + { + String jLinkExecutable = null; + if ( getToolchain() != null ) + { + jLinkExecutable = getToolchain().findTool( "jlink" ); + } + + // TODO: Check if there exist a more elegant way? + String jLinkCommand = "jlink" + ( SystemUtils.IS_OS_WINDOWS ? ".exe" : "" ); + + File jLinkExe; + + if ( StringUtils.isNotEmpty( jLinkExecutable ) ) + { + jLinkExe = new File( jLinkExecutable ); + + if ( jLinkExe.isDirectory() ) + { + jLinkExe = new File( jLinkExe, jLinkCommand ); + } + + if ( SystemUtils.IS_OS_WINDOWS && jLinkExe.getName().indexOf( '.' ) < 0 ) + { + jLinkExe = new File( jLinkExe.getPath() + ".exe" ); + } + + if ( !jLinkExe.isFile() ) + { + throw new IOException( "The jlink executable '" + jLinkExe + "' doesn't exist or is not a file." ); + } + return jLinkExe.getAbsolutePath(); + } + + // ---------------------------------------------------------------------- Review comment: I think everything below here can be replaced with a message that either a JDK 9+ as runtime is required or a toolchain ########## File path: src/main/java/org/apache/maven/plugins/jlink/JLinkExecutor.java ########## @@ -0,0 +1,171 @@ +package org.apache.maven.plugins.jlink; + +/* + * 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.commons.lang3.SystemUtils; +import org.apache.maven.plugin.logging.Log; +import org.apache.maven.toolchain.Toolchain; +import org.codehaus.plexus.util.StringUtils; +import org.codehaus.plexus.util.cli.CommandLineUtils; +import org.codehaus.plexus.util.cli.Commandline; + +import java.io.File; +import java.io.IOException; +import java.util.Optional; +import java.util.Properties; + +/** + * JDK 8-only Jlink executor. + * + * <p>As JDK8 does not ship jlink, either a toolchain or JAVA_HOME is required.</p> Review comment: `JAVA_HOME` is the runtime for Maven, so that's not going to help here. Only toolchains will work. The JAVA_HOME based recipe was for Java 9+ via commandline, which will now be replaced with the ToolProvider. ########## File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java ########## @@ -0,0 +1,175 @@ +/* + * 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.plugins.jlink; + +import org.apache.commons.lang3.StringUtils; +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugin.logging.Log; +import org.apache.maven.toolchain.Toolchain; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.io.PrintWriter; +import java.nio.file.Files; +import java.util.ArrayDeque; +import java.util.Deque; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.Optional; +import java.util.spi.ToolProvider; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +class JLinkExecutor extends AbstractJLinkExecutor +{ + + private final ToolProvider toolProvider; + + public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException + { + super( toolchain, log ); + this.toolProvider = getJLinkExecutable(); + } + + @Override + public Optional<File> getJmodsFolder( /* nullable */ File sourceJdkModules ) + { + if ( sourceJdkModules != null && sourceJdkModules.isDirectory() ) + { + return Optional.of( new File( sourceJdkModules, JMODS ) ); + } + + // ToolProvider does not need jmods folder to be set. + return Optional.empty(); + } + + protected final ToolProvider getJLinkExecutable() + { + Optional<ToolProvider> jlink = ToolProvider.findFirst( "jlink" ); + + if ( !jlink.isPresent() ) Review comment: Looks like this can be replaced with a oneliner: `return jlink.orElseThrow( new IllegalStateException( "No jlink tool found." ) );` ########## File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java ########## @@ -0,0 +1,175 @@ +/* + * 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.plugins.jlink; + +import org.apache.commons.lang3.StringUtils; +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugin.logging.Log; +import org.apache.maven.toolchain.Toolchain; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.io.PrintWriter; +import java.nio.file.Files; +import java.util.ArrayDeque; +import java.util.Deque; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.Optional; +import java.util.spi.ToolProvider; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +class JLinkExecutor extends AbstractJLinkExecutor +{ + + private final ToolProvider toolProvider; + + public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException + { + super( toolchain, log ); + this.toolProvider = getJLinkExecutable(); + } + + @Override + public Optional<File> getJmodsFolder( /* nullable */ File sourceJdkModules ) + { + if ( sourceJdkModules != null && sourceJdkModules.isDirectory() ) + { + return Optional.of( new File( sourceJdkModules, JMODS ) ); + } + + // ToolProvider does not need jmods folder to be set. + return Optional.empty(); + } + + protected final ToolProvider getJLinkExecutable() + { + Optional<ToolProvider> jlink = ToolProvider.findFirst( "jlink" ); + + if ( !jlink.isPresent() ) + { + throw new IllegalStateException( "No jlink tool found." ); + } + + return jlink.orElseThrow( NoSuchElementException::new ); + } + + + protected Stream<String> argsfileToArgs( File argsFile ) + { + try + { + List<String> strings = Files.readAllLines( argsFile.toPath() ); + Deque<String> out = new ArrayDeque<>(); + + for ( String line : strings ) + { + if ( line.startsWith( "-" ) ) + { + out.add( line ); + continue; + } + + if ( line.startsWith( "\"" ) && line.endsWith( "\"" ) ) + { + out.add( line.substring( 1, line.lastIndexOf( "\"" ) ) ); + continue; + } + + out.add( line ); + } + + return out.stream(); + } + catch ( IOException e ) + { + throw new IllegalStateException( "Unable to read jlinkArgs file: " + argsFile.getAbsolutePath() ); + } + + } + + @Override + public int executeJlink( File argsFile ) throws MojoExecutionException Review comment: I would prefer to pass a `List<String> args` here, there's no need to write and reuse an argFile in 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. For queries about this service, please contact Infrastructure at: [email protected]
