You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/11/14 21:58:03 UTC
[GitHub] [maven-jlink-plugin] bmarwell opened a new pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
bmarwell opened a new pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16
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/MJLINK) 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 `[MJLINK-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MJLINK-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in 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.
- [X] You have run the integration tests successfully (`mvn -Prun-its clean verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [X] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526671119
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkToolchainExecutor.java
##########
@@ -0,0 +1,204 @@
+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.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
+import org.apache.maven.shared.utils.cli.CommandLineException;
+import org.apache.maven.shared.utils.cli.CommandLineUtils;
+import org.apache.maven.shared.utils.cli.Commandline;
+import org.apache.maven.toolchain.Toolchain;
+import org.codehaus.plexus.util.StringUtils;
Review comment:
I was not going to refactor existing code. I will happily to that in another PR.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] elharo commented on a change in pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526427117
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkToolchainExecutor.java
##########
@@ -0,0 +1,204 @@
+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.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
+import org.apache.maven.shared.utils.cli.CommandLineException;
+import org.apache.maven.shared.utils.cli.CommandLineUtils;
+import org.apache.maven.shared.utils.cli.Commandline;
+import org.apache.maven.toolchain.Toolchain;
+import org.codehaus.plexus.util.StringUtils;
Review comment:
can you use commons lang instead?
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkToolchainExecutor.java
##########
@@ -0,0 +1,204 @@
+package org.apache.maven.plugins.jlink;
+
+/*
Review comment:
copyright usually goes above package
##########
File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.util.List;
+import java.util.Optional;
+import java.util.spi.ToolProvider;
+
+/**
+ * JDK9+ executor for jlink.
+ *
+ * <p>This implementation uses the JDK9+ Toolprovider SPI to find and execute jlink.
+ * This way, no fork needs to be created.</p>
+ */
+class JLinkExecutor extends AbstractJLinkToolchainExecutor
+{
+ private final ToolProvider toolProvider;
+
+ public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException
+ {
+ super( toolchain, log );
+ this.toolProvider = getJLinkExecutable();
+ }
+
+ protected final ToolProvider getJLinkExecutable()
+ {
+ return ToolProvider
+ .findFirst( "jlink" )
+ .orElseThrow( () -> new IllegalStateException( "No jlink tool found." ) );
+ }
+
+ @Override
+ public int executeJlink( List<String> jlinkArgs ) throws MojoExecutionException
+ {
+ if (getToolchain().isPresent())
+ {
+ return super.executeJlink( jlinkArgs );
+ }
+
+ if ( getLog().isDebugEnabled() )
+ {
+ // no quoted arguments ???
+ getLog().debug( this.toolProvider.name() + " " + jlinkArgs );
+ }
+
+ try ( ByteArrayOutputStream baosErr = new ByteArrayOutputStream();
+ PrintWriter err = new PrintWriter( baosErr );
+ ByteArrayOutputStream baosOut = new ByteArrayOutputStream();
+ PrintWriter out = new PrintWriter( baosOut ) )
Review comment:
dangerous! eats IO exceptions. Don't use PrintWriter.
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkToolchainExecutor.java
##########
@@ -0,0 +1,204 @@
+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.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
Review comment:
use SLF4J
##########
File path: src/main/java/org/apache/maven/plugins/jlink/JLinkMojo.java
##########
@@ -443,15 +416,14 @@ public void execute()
return modulepathElements;
}
- private String getExecutable()
- throws MojoFailureException
+ private JLinkExecutor getExecutor() throws MojoFailureException
{
- String jLinkExec;
+ JLinkExecutor jLinkExec;
try
{
- jLinkExec = getJLinkExecutable();
+ jLinkExec = getJlinkExecutor();
}
- catch ( IOException e )
+ catch ( RuntimeException e )
Review comment:
can you be more specific here?
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkExecutor.java
##########
@@ -0,0 +1,63 @@
+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.maven.plugin.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
Review comment:
No need to use this. SLF4J is the decision for new code in the project.
##########
File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.util.List;
+import java.util.Optional;
+import java.util.spi.ToolProvider;
+
+/**
+ * JDK9+ executor for jlink.
+ *
+ * <p>This implementation uses the JDK9+ Toolprovider SPI to find and execute jlink.
+ * This way, no fork needs to be created.</p>
+ */
+class JLinkExecutor extends AbstractJLinkToolchainExecutor
+{
+ private final ToolProvider toolProvider;
+
+ public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException
Review comment:
don't need a public constructor in a non-public class
##########
File path: src/main/java/org/apache/maven/plugins/jlink/JLinkMojo.java
##########
@@ -443,15 +416,14 @@ public void execute()
return modulepathElements;
}
- private String getExecutable()
- throws MojoFailureException
+ private JLinkExecutor getExecutor() throws MojoFailureException
{
- String jLinkExec;
+ JLinkExecutor jLinkExec;
try
{
- jLinkExec = getJLinkExecutable();
+ jLinkExec = getJlinkExecutor();
Review comment:
try block should be larger and not split the declaration from initialization
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526960377
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkToolchainExecutor.java
##########
@@ -0,0 +1,204 @@
+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.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
+import org.apache.maven.shared.utils.cli.CommandLineException;
+import org.apache.maven.shared.utils.cli.CommandLineUtils;
+import org.apache.maven.shared.utils.cli.Commandline;
+import org.apache.maven.toolchain.Toolchain;
+import org.codehaus.plexus.util.StringUtils;
Review comment:
Source: https://github.com/apache/maven-jlink-plugin/blob/2f2aa04f8cc93d9937a9a3f28647285dd5713937/src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkMojo.java#L39
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526677178
##########
File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.util.List;
+import java.util.Optional;
+import java.util.spi.ToolProvider;
+
+/**
+ * JDK9+ executor for jlink.
+ *
+ * <p>This implementation uses the JDK9+ Toolprovider SPI to find and execute jlink.
+ * This way, no fork needs to be created.</p>
+ */
+class JLinkExecutor extends AbstractJLinkToolchainExecutor
+{
+ private final ToolProvider toolProvider;
+
+ public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException
+ {
+ super( toolchain, log );
+ this.toolProvider = getJLinkExecutable();
+ }
+
+ protected final ToolProvider getJLinkExecutable()
+ {
+ return ToolProvider
+ .findFirst( "jlink" )
+ .orElseThrow( () -> new IllegalStateException( "No jlink tool found." ) );
+ }
+
+ @Override
+ public int executeJlink( List<String> jlinkArgs ) throws MojoExecutionException
+ {
+ if (getToolchain().isPresent())
+ {
+ return super.executeJlink( jlinkArgs );
+ }
+
+ if ( getLog().isDebugEnabled() )
+ {
+ // no quoted arguments ???
+ getLog().debug( this.toolProvider.name() + " " + jlinkArgs );
+ }
+
+ try ( ByteArrayOutputStream baosErr = new ByteArrayOutputStream();
+ PrintWriter err = new PrintWriter( baosErr );
+ ByteArrayOutputStream baosOut = new ByteArrayOutputStream();
+ PrintWriter out = new PrintWriter( baosOut ) )
Review comment:
Yes, I agree. This is also something for a new PR. I did chose `PrintWriter` on purpose here, as the original code contained a `PrintStream` with the same behaviour. Let’s open another JIRA issue for that and fix this in another PR.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526681588
##########
File path: src/main/java/org/apache/maven/plugins/jlink/JLinkMojo.java
##########
@@ -443,15 +416,14 @@ public void execute()
return modulepathElements;
}
- private String getExecutable()
- throws MojoFailureException
+ private JLinkExecutor getExecutor() throws MojoFailureException
{
- String jLinkExec;
+ JLinkExecutor jLinkExec;
try
{
- jLinkExec = getJLinkExecutable();
+ jLinkExec = getJlinkExecutor();
}
- catch ( IOException e )
+ catch ( RuntimeException e )
Review comment:
Done. Moved this outdated try-catch to the execution.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526444334
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkExecutor.java
##########
@@ -0,0 +1,63 @@
+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.maven.plugin.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
Review comment:
I was not going to refactor existing code. Is prefer to do this in another PR.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] elharo commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526849731
##########
File path: pom.xml
##########
@@ -65,7 +65,8 @@
</distributionManagement>
<properties>
- <mavenVersion>3.1.0</mavenVersion>
+ <!-- required do to conflict with asm:asm:3.3.1 in Maven 3.1.0, see MNG-5499. -->
Review comment:
do --> due
##########
File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.util.List;
+import java.util.Optional;
+import java.util.spi.ToolProvider;
+
+/**
+ * JDK9+ executor for jlink.
+ *
+ * <p>This implementation uses the JDK9+ Toolprovider SPI to find and execute jlink.
+ * This way, no fork needs to be created.</p>
+ */
+class JLinkExecutor extends AbstractJLinkToolchainExecutor
+{
+ private final ToolProvider toolProvider;
+
+ public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException
+ {
+ super( toolchain, log );
+ this.toolProvider = getJLinkExecutable();
+ }
+
+ protected final ToolProvider getJLinkExecutable()
+ {
+ return ToolProvider
+ .findFirst( "jlink" )
+ .orElseThrow( () -> new IllegalStateException( "No jlink tool found." ) );
+ }
+
+ @Override
+ public int executeJlink( List<String> jlinkArgs ) throws MojoExecutionException
+ {
+ if (getToolchain().isPresent())
+ {
+ return super.executeJlink( jlinkArgs );
+ }
+
+ if ( getLog().isDebugEnabled() )
+ {
+ // no quoted arguments ???
+ getLog().debug( this.toolProvider.name() + " " + jlinkArgs );
+ }
+
+ try ( ByteArrayOutputStream baosErr = new ByteArrayOutputStream();
+ PrintWriter err = new PrintWriter( baosErr );
+ ByteArrayOutputStream baosOut = new ByteArrayOutputStream();
+ PrintWriter out = new PrintWriter( baosOut ) )
Review comment:
You keep saying this is existing code, but Github shows this all as completely new code.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#issuecomment-730223612
@elharo while agree with **everything** you wrote, I do not want to change any algorithms, methods etc. for this method. You had a chance to review these things earlier in the development phase. I did not introduce them, and I will keep them as I do not want to touch any of the inner logic. Please see my comments and open separate JIRA issues for each of them.
Thanks for pointing out the unnecessary constructor and try-catch though. I fixed them.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#issuecomment-728902248
@rfscholte done, updated the issue. Sadly, the jdk8+toolchain-test fails. Logs see https://gist.github.com/bmarwell/0bdd1a939b65c4dd129846bc936fa875
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526443326
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkToolchainExecutor.java
##########
@@ -0,0 +1,204 @@
+package org.apache.maven.plugins.jlink;
+
+/*
Review comment:
I know, but the checkstyle prevents this. I am not *also* going to change the checkstyle configuration in this PR. It's already very big.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] rfscholte edited a comment on pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
rfscholte edited a comment on pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#issuecomment-728255698
PR looks much better already.
> What about JDK9+ and Toolchains?
Toolchains always wins
> Does JLinkMojo::getModulePathElements need an update?
Not sure if I understand the question, but what I do know is dat some ugly parts of the maven-compiler-plugin were copied to this plugin, which should have been solved cleaner.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r527044004
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkExecutor.java
##########
@@ -0,0 +1,63 @@
+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.maven.plugin.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
Review comment:
https://issues.apache.org/jira/browse/MJLINK-57
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#issuecomment-727927048
@rfscholte please also see the "open questions" from the main comment.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#issuecomment-730437952
@elharo I added links to all your comments. I hope this helps to understand why I think this is "out of scope of this PR" and keep talking about "existing code".
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526959284
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkToolchainExecutor.java
##########
@@ -0,0 +1,204 @@
+package org.apache.maven.plugins.jlink;
+
+/*
Review comment:
Actually, you can figure out which checks are being enforced this way:
```
(user@host)[~/git/apache/maven-jlink-plugin]$ mvn help:effective-pom
[INFO] Scanning for projects...
[INFO]
[INFO] ------------< org.apache.maven.plugins:maven-jlink-plugin >-------------
[INFO] Building Apache Maven JLink Plugin 3.0.1-SNAPSHOT
[INFO] ----------------------------[ maven-plugin ]----------------------------
[INFO]
[INFO] --- maven-help-plugin:3.2.0:effective-pom (default-cli) @ maven-jlink-plugin ---
Downloading from central: https://repo.maven.apache.org/maven2/org/apache/maven/maven-model/3.6.1/maven-model-3.6.1.pom
Downloaded from central: https://repo.maven.apache.org/maven2/org/apache/maven/maven-model/3.6.1/maven-model-3.6.1.pom (4.0 kB at 5.7 kB/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/apache/maven/maven/3.6.1/maven-3.6.1.pom
Downloaded from central: https://repo.maven.apache.org/maven2/org/apache/maven/maven/3.6.1/maven-3.6.1.pom (24 kB at 523 kB/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/apache/maven/maven-model/3.6.1/maven-model-3.6.1.jar
Downloaded from central: https://repo.maven.apache.org/maven2/org/apache/maven/maven-model/3.6.1/maven-model-3.6.1.jar (186 kB at 2.2 MB/s)
[INFO]
Effective POMs, after inheritance, interpolation, and profiles are applied:
[…]
<plugin>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>3.0.0</version>
<dependencies>
<dependency>
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-shared-resources</artifactId>
<version>2</version>
<scope>compile</scope>
</dependency>
</dependencies>
<configuration>
<configLocation>config/maven_checks.xml</configLocation>
<headerLocation>config/maven-header.txt</headerLocation>
[…]
```
Then: https://github.com/apache/maven-shared-resources/blob/master/src/main/resources/config/maven-header.txt
HTH
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526959685
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkExecutor.java
##########
@@ -0,0 +1,63 @@
+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.maven.plugin.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
Review comment:
Orig: https://github.com/apache/maven-jlink-plugin/blob/5e2b1272727400d81eb64a3de50557f343097319/src/main/java/org/apache/maven/plugins/jlink/JLinkMojo.java#L283
etc
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#issuecomment-727270900
This PR really needs Java 8 ITs including a toolchain. Otherwise the `JLinkExecutor` class from the `java` (i.e. non-`java9`)-source-folder is not being tested. Currently the ITs are skipped. Also see https://issues.apache.org/jira/projects/MJLINK/issues/MJLINK-53
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r527045163
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkToolchainExecutor.java
##########
@@ -0,0 +1,204 @@
+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.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
+import org.apache.maven.shared.utils.cli.CommandLineException;
+import org.apache.maven.shared.utils.cli.CommandLineUtils;
+import org.apache.maven.shared.utils.cli.Commandline;
+import org.apache.maven.toolchain.Toolchain;
+import org.codehaus.plexus.util.StringUtils;
Review comment:
https://issues.apache.org/jira/browse/MJLINK-58
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] rfscholte closed pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
rfscholte closed pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526953349
##########
File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.util.List;
+import java.util.Optional;
+import java.util.spi.ToolProvider;
+
+/**
+ * JDK9+ executor for jlink.
+ *
+ * <p>This implementation uses the JDK9+ Toolprovider SPI to find and execute jlink.
+ * This way, no fork needs to be created.</p>
+ */
+class JLinkExecutor extends AbstractJLinkToolchainExecutor
+{
+ private final ToolProvider toolProvider;
+
+ public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException
+ {
+ super( toolchain, log );
+ this.toolProvider = getJLinkExecutable();
+ }
+
+ protected final ToolProvider getJLinkExecutable()
+ {
+ return ToolProvider
+ .findFirst( "jlink" )
+ .orElseThrow( () -> new IllegalStateException( "No jlink tool found." ) );
+ }
+
+ @Override
+ public int executeJlink( List<String> jlinkArgs ) throws MojoExecutionException
+ {
+ if (getToolchain().isPresent())
+ {
+ return super.executeJlink( jlinkArgs );
+ }
+
+ if ( getLog().isDebugEnabled() )
+ {
+ // no quoted arguments ???
+ getLog().debug( this.toolProvider.name() + " " + jlinkArgs );
+ }
+
+ try ( ByteArrayOutputStream baosErr = new ByteArrayOutputStream();
+ PrintWriter err = new PrintWriter( baosErr );
+ ByteArrayOutputStream baosOut = new ByteArrayOutputStream();
+ PrintWriter out = new PrintWriter( baosOut ) )
Review comment:
Original printstream method: https://github.com/apache/maven-jlink-plugin/blob/master/src/main/java/org/apache/maven/plugins/jlink/JLinkMojo.java#L540-L664
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526953349
##########
File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.util.List;
+import java.util.Optional;
+import java.util.spi.ToolProvider;
+
+/**
+ * JDK9+ executor for jlink.
+ *
+ * <p>This implementation uses the JDK9+ Toolprovider SPI to find and execute jlink.
+ * This way, no fork needs to be created.</p>
+ */
+class JLinkExecutor extends AbstractJLinkToolchainExecutor
+{
+ private final ToolProvider toolProvider;
+
+ public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException
+ {
+ super( toolchain, log );
+ this.toolProvider = getJLinkExecutable();
+ }
+
+ protected final ToolProvider getJLinkExecutable()
+ {
+ return ToolProvider
+ .findFirst( "jlink" )
+ .orElseThrow( () -> new IllegalStateException( "No jlink tool found." ) );
+ }
+
+ @Override
+ public int executeJlink( List<String> jlinkArgs ) throws MojoExecutionException
+ {
+ if (getToolchain().isPresent())
+ {
+ return super.executeJlink( jlinkArgs );
+ }
+
+ if ( getLog().isDebugEnabled() )
+ {
+ // no quoted arguments ???
+ getLog().debug( this.toolProvider.name() + " " + jlinkArgs );
+ }
+
+ try ( ByteArrayOutputStream baosErr = new ByteArrayOutputStream();
+ PrintWriter err = new PrintWriter( baosErr );
+ ByteArrayOutputStream baosOut = new ByteArrayOutputStream();
+ PrintWriter out = new PrintWriter( baosOut ) )
Review comment:
Original printstream method: https://github.com/apache/maven-jlink-plugin/blob/5e2b1272727400d81eb64a3de50557f343097319/src/main/java/org/apache/maven/plugins/jlink/JLinkMojo.java#L540-L664
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] rfscholte commented on pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#issuecomment-733141016
Fixed in https://github.com/apache/maven-jlink-plugin/commit/f8bdf5050c266854524aaa51eb36109c00ca692a
Thanks for the patch!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r527045754
##########
File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.util.List;
+import java.util.Optional;
+import java.util.spi.ToolProvider;
+
+/**
+ * JDK9+ executor for jlink.
+ *
+ * <p>This implementation uses the JDK9+ Toolprovider SPI to find and execute jlink.
+ * This way, no fork needs to be created.</p>
+ */
+class JLinkExecutor extends AbstractJLinkToolchainExecutor
+{
+ private final ToolProvider toolProvider;
+
+ public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException
+ {
+ super( toolchain, log );
+ this.toolProvider = getJLinkExecutable();
+ }
+
+ protected final ToolProvider getJLinkExecutable()
+ {
+ return ToolProvider
+ .findFirst( "jlink" )
+ .orElseThrow( () -> new IllegalStateException( "No jlink tool found." ) );
+ }
+
+ @Override
+ public int executeJlink( List<String> jlinkArgs ) throws MojoExecutionException
+ {
+ if (getToolchain().isPresent())
+ {
+ return super.executeJlink( jlinkArgs );
+ }
+
+ if ( getLog().isDebugEnabled() )
+ {
+ // no quoted arguments ???
+ getLog().debug( this.toolProvider.name() + " " + jlinkArgs );
+ }
+
+ try ( ByteArrayOutputStream baosErr = new ByteArrayOutputStream();
+ PrintWriter err = new PrintWriter( baosErr );
+ ByteArrayOutputStream baosOut = new ByteArrayOutputStream();
+ PrintWriter out = new PrintWriter( baosOut ) )
Review comment:
https://issues.apache.org/jira/browse/MJLINK-59
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] rfscholte commented on pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#issuecomment-728255698
PR looks much better already.
> What about JDK9+ and Toolchains?
Toolchains always wins
> Does JLinkMojo::getModulePathElements need an update?
Not sure if I understand the question, but what I do know is dat some ugly parts of the maven-compiler-plugin were copied to this plugin, which should have been solved cleaner.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526953935
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkToolchainExecutor.java
##########
@@ -0,0 +1,204 @@
+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.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
Review comment:
Orig: https://github.com/apache/maven-jlink-plugin/blob/5e2b1272727400d81eb64a3de50557f343097319/src/main/java/org/apache/maven/plugins/jlink/JLinkMojo.java#L283
etc
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] rfscholte commented on a change in pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526299689
##########
File path: pom.xml
##########
@@ -65,7 +65,7 @@
</distributionManagement>
<properties>
- <mavenVersion>3.1.0</mavenVersion>
+ <mavenVersion>3.1.1</mavenVersion>
Review comment:
Please add xml comment: required do to conflict with asm:asm:3.3.1 in Maven 3.1.0
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526679134
##########
File path: src/main/java/org/apache/maven/plugins/jlink/JLinkMojo.java
##########
@@ -443,15 +416,14 @@ public void execute()
return modulepathElements;
}
- private String getExecutable()
- throws MojoFailureException
+ private JLinkExecutor getExecutor() throws MojoFailureException
{
- String jLinkExec;
+ JLinkExecutor jLinkExec;
try
{
- jLinkExec = getJLinkExecutable();
+ jLinkExec = getJlinkExecutor();
Review comment:
This is also something not in the scope of the PR. You are asking me to do a full refactoring here, while I only want to introduce ToolProvider. This line you marked is exactly as the existing code. Please file a JIRA issue and lets create a new PR for this.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526313292
##########
File path: pom.xml
##########
@@ -65,7 +65,7 @@
</distributionManagement>
<properties>
- <mavenVersion>3.1.0</mavenVersion>
+ <mavenVersion>3.1.1</mavenVersion>
Review comment:
Done, also added the maven issue number so others can verify it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] rfscholte commented on a change in pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#issuecomment-733079333
@rfscholte would you merge this one please? I do not have the karma to do so. Another review is also ok :) Thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526444198
##########
File path: src/main/java/org/apache/maven/plugins/jlink/AbstractJLinkToolchainExecutor.java
##########
@@ -0,0 +1,204 @@
+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.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
Review comment:
I was not going to refactor existing code. Is prefer to do this in another PR.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526681732
##########
File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.util.List;
+import java.util.Optional;
+import java.util.spi.ToolProvider;
+
+/**
+ * JDK9+ executor for jlink.
+ *
+ * <p>This implementation uses the JDK9+ Toolprovider SPI to find and execute jlink.
+ * This way, no fork needs to be created.</p>
+ */
+class JLinkExecutor extends AbstractJLinkToolchainExecutor
+{
+ private final ToolProvider toolProvider;
+
+ public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException
Review comment:
Done, thanks for mentioning.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] java9 ToolProvider via Multi-Release jar.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r526930416
##########
File path: src/main/java9/org/apache/maven/plugins/jlink/JLinkExecutor.java
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.util.List;
+import java.util.Optional;
+import java.util.spi.ToolProvider;
+
+/**
+ * JDK9+ executor for jlink.
+ *
+ * <p>This implementation uses the JDK9+ Toolprovider SPI to find and execute jlink.
+ * This way, no fork needs to be created.</p>
+ */
+class JLinkExecutor extends AbstractJLinkToolchainExecutor
+{
+ private final ToolProvider toolProvider;
+
+ public JLinkExecutor( Toolchain toolchain, Log log ) throws IOException
+ {
+ super( toolchain, log );
+ this.toolProvider = getJLinkExecutable();
+ }
+
+ protected final ToolProvider getJLinkExecutable()
+ {
+ return ToolProvider
+ .findFirst( "jlink" )
+ .orElseThrow( () -> new IllegalStateException( "No jlink tool found." ) );
+ }
+
+ @Override
+ public int executeJlink( List<String> jlinkArgs ) throws MojoExecutionException
+ {
+ if (getToolchain().isPresent())
+ {
+ return super.executeJlink( jlinkArgs );
+ }
+
+ if ( getLog().isDebugEnabled() )
+ {
+ // no quoted arguments ???
+ getLog().debug( this.toolProvider.name() + " " + jlinkArgs );
+ }
+
+ try ( ByteArrayOutputStream baosErr = new ByteArrayOutputStream();
+ PrintWriter err = new PrintWriter( baosErr );
+ ByteArrayOutputStream baosOut = new ByteArrayOutputStream();
+ PrintWriter out = new PrintWriter( baosOut ) )
Review comment:
That one, true. Before this was a `FileStream()`/`PrintStream` and in another file.
Due to refactoring and @rfscholte’s request to not write a file anymore, I chose `PrintWriter` on purpose here to simulate the same behaviour the `PrintStream` head.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] rfscholte commented on a change in pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r523765034
##########
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:
> Or should I change it for the java8 JLinkExecutor as well?
The method signatures must be the same, so yes, java8 LinkExecutor must be updated too. As Java 9 won't be using the args file, having it might also be misleading. With debug logging you should be able to see how jlink tool was called.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-jlink-plugin] bmarwell commented on a change in pull request #16: [MJLINK-36] WIP – draft for java9 ToolProvider via java9 source folder.
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #16:
URL: https://github.com/apache/maven-jlink-plugin/pull/16#discussion_r523750235
##########
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 was thinking of the same. But then, the execution would differ from both the original code and the Java8 code executing jlink via toolchain.
Also, it might be nice for transparency.
I can change that, but this will result in java8+toolchain builds having the arg file, and java9+ builds on Jenkins will not.
If that's okay for you, I'll change it.
Or should I change it for the java8 JLinkExecutor as well?
----------------------------------------------------------------
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:
users@infra.apache.org