[
https://issues.apache.org/jira/browse/MJARSIGNER-72?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17795957#comment-17795957
]
ASF GitHub Bot commented on MJARSIGNER-72:
------------------------------------------
elharo commented on code in PR #18:
URL:
https://github.com/apache/maven-jarsigner-plugin/pull/18#discussion_r1424669170
##########
src/main/java/org/apache/maven/plugins/jarsigner/AbstractJarsignerMojo.java:
##########
@@ -437,14 +437,26 @@ protected void validateParameters() throws
MojoExecutionException {
// Default implementation does nothing
}
+ /**
+ * Process (sign/verify) a list of archives.
+ *
+ * @param archives list of jar files to process
+ * @throws MojoExecutionException If an error occurs during the processing
of archives.
Review Comment:
no period
##########
src/main/java/org/apache/maven/plugins/jarsigner/AbstractJarsignerMojo.java:
##########
@@ -279,65 +280,70 @@ public final void execute() throws MojoExecutionException
{
jarSigner.setToolchain(toolchain);
}
- int processed = 0;
+ List<File> archives = findJarfiles();
+ processArchives(archives);
+ getLog().info(getMessage("processed", archives.size()));
+ }
+ /**
+ * Finds all jar files, by looking at the Maven project and user
configuration.
+ *
+ * @return a List of File objects
+ * @throws MojoExecutionException if it was not possible to build a list
of jar files.
Review Comment:
no period at end, per Oracle guidelines
##########
src/test/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojoParallelTest.java:
##########
@@ -0,0 +1,218 @@
+/*
+ * 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.jarsigner;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.Duration;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.maven.artifact.Artifact;
+import org.apache.maven.plugin.MojoExecutionException;
+import org.apache.maven.plugin.logging.Log;
+import org.apache.maven.project.MavenProject;
+import org.apache.maven.shared.jarsigner.JarSigner;
+import org.apache.maven.shared.jarsigner.JarSignerSignRequest;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import static
org.apache.maven.plugins.jarsigner.TestJavaToolResults.RESULT_ERROR;
+import static org.apache.maven.plugins.jarsigner.TestJavaToolResults.RESULT_OK;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.*;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.contains;
+import static org.mockito.ArgumentMatchers.isA;
+import static org.mockito.Mockito.*;
+
+public class JarsignerSignMojoParallelTest {
+ @Rule
+ public TemporaryFolder folder = new TemporaryFolder();
+
+ private MavenProject project = mock(MavenProject.class);
+ private JarSigner jarSigner = mock(JarSigner.class);
+ private File projectDir;
+ private Map<String, String> configuration = new LinkedHashMap<>();
+ private MojoTestCreator<JarsignerSignMojo> mojoTestCreator;
+ private ExecutorService executor;
+ private Log log;
+
+ @Before
+ public void setUp() throws Exception {
+ projectDir = folder.newFolder("dummy-project");
+ configuration.put("processMainArtifact", "false");
+ mojoTestCreator =
+ new
MojoTestCreator<JarsignerSignMojo>(JarsignerSignMojo.class, project,
projectDir, jarSigner);
+ log = mock(Log.class);
+ mojoTestCreator.setLog(log);
+ executor =
+
Executors.newSingleThreadExecutor(namedThreadFactory(getClass().getSimpleName()));
+ }
+
+ @After
+ public void tearDown() {
+ executor.shutdown();
+ }
+
+ @Test(timeout = 30000)
+ public void test10Files2Parallel() throws Exception {
+ configuration.put("archiveDirectory", createArchives(10).getPath());
+ configuration.put("threadCount", "2");
+
+ // Make one jar file wait until some external event happens and let
nine pass
+ Semaphore semaphore = new Semaphore(9);
+
when(jarSigner.execute(isA(JarSignerSignRequest.class))).then(invocation -> {
+ semaphore.acquire();
+ return RESULT_OK;
+ });
+ JarsignerSignMojo mojo = mojoTestCreator.configure(configuration);
+
+ Future<Void> future = executor.submit(() -> {
+ mojo.execute();
+ return null;
+ });
+
+ // Wait until 10 invocation to execute has happened (nine files are
done and one are hanging)
+ verify(jarSigner,
timeout(Duration.ofSeconds(10).toMillis()).times(10)).execute(any());
+ // Even though 10 invocations to execute() has happened, mojo is not
yet done executing (it is waiting for one)
Review Comment:
has --> have
to execute --> of execute
##########
src/main/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojo.java:
##########
@@ -174,6 +197,36 @@ protected JarSignerRequest createRequest(File archive)
throws MojoExecutionExcep
return request;
}
+ /**
+ * {@inheritDoc} Processing of files may be parallelized for increased
performance.
+ */
+ @Override
+ protected void processArchives(List<File> archives) throws
MojoExecutionException {
+ ExecutorService executor = Executors.newFixedThreadPool(threadCount);
Review Comment:
should this be the minimum of threadCount and the number of archives? No
reason to spawn ten threads with only 5 files to sign
##########
src/test/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojoTest.java:
##########
@@ -401,4 +412,54 @@ public void testSetCustomFileEncoding() throws Exception {
.execute(MockitoHamcrest.argThat(
RequestMatchers.hasArguments(new String[]
{"-J-Dfile.encoding=ISO-8859-1", "argument2"})));
}
+
+ /**
+ * Test what is logged when verbose=true. The sign-mojo.html documentation
indicates that the verbose flag should
+ * be sent in to the jarsigner command. That is true, bit in addotion to
this it is also (undocumented) used to
+ * control the level of some logging events.
+ */
+ @Test
+ public void testLoggingVerboseTrue() throws Exception {
Review Comment:
Personally I prefer not to test logging since I don't consider it part of
the contract of a method
##########
src/main/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojo.java:
##########
@@ -115,6 +122,17 @@ public class JarsignerSignMojo extends
AbstractJarsignerMojo {
@Parameter(property = "jarsigner.maxRetryDelaySeconds", defaultValue = "0")
private int maxRetryDelaySeconds;
+ /**
+ * How many threads to use (in parallel) when signing jar files. Increases
performance when signing multiple jar
Review Comment:
Is there a plausible maximum here?
##########
src/test/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojoTest.java:
##########
@@ -401,4 +412,54 @@ public void testSetCustomFileEncoding() throws Exception {
.execute(MockitoHamcrest.argThat(
RequestMatchers.hasArguments(new String[]
{"-J-Dfile.encoding=ISO-8859-1", "argument2"})));
}
+
+ /**
+ * Test what is logged when verbose=true. The sign-mojo.html documentation
indicates that the verbose flag should
+ * be sent in to the jarsigner command. That is true, bit in addotion to
this it is also (undocumented) used to
Review Comment:
bit --> but
> Parallel signing for increased speed
> ------------------------------------
>
> Key: MJARSIGNER-72
> URL: https://issues.apache.org/jira/browse/MJARSIGNER-72
> Project: Maven Jar Signer Plugin
> Issue Type: New Feature
> Affects Versions: 3.0.0
> Reporter: Lennart Schedin
> Priority: Minor
> Labels: performance
>
> *Background:*
> As of June 1 2023, a new industry standard mandates the storage of private
> keys used for code signing on external hardware devices. Refer to
> [https://knowledge.digicert.com/general-information/new-private-key-storage-requirement-for-standard-code-signing-certificates-november-2022]
> for details. Various devices, from the Thales SafeNet USB eToken (about
> $30), Yubico YubiHSM 2 FIPS (about €1000) up to Thales Luna S700 Series
> (about €30000) can store these keys. Cloud-based HSM solutions (like DigiCert
> KeyLocker ($90/year)) also exist.
>
> This ticket primarily targets HSM as a service but could benefit network
> attached HSM solutions as well.
>
> *Problem:*
> Using the {{jarsigner:sign}} goal it is possible to specify
> {{{}archiveDirectory{}}}, that points to a directory with many jar files.
> This is useful for signing every dependency the project has.
>
> Using the DigiCert Keylocker HSM as a service I measured that it took 240
> seconds to sign 128 jar files. I was in Sweden and the DigiCert Keylocker
> service is in USA. The response time of server is about 500 to 700 ms
> (without any login and without any signing).
>
> I created a quick parallel hack (using the Linux command parallel) that used
> 8 threads and it took only 31 seconds. That is: for this specific HSM service
> it scales linearly with the number of threads used.
>
> *To implement:*
> I propose to implement a parallelization for maven-jarsigner-plugin that can
> be used when signing many jar files at once.
>
> The configuration for this could be a new parameter named {{threadCount}}
> (with user property {{{}jarsigner.threadCount{}}}) with default to 1 (no
> parallelization).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
