desruisseaux commented on code in PR #508:
URL: https://github.com/apache/maven-jar-plugin/pull/508#discussion_r3844473279


##########
src/main/java/org/apache/maven/plugins/jar/AbstractJarMojo.java:
##########
@@ -207,167 +205,208 @@ protected final Log getLog() {
     protected abstract String getType();
 
     /**
-     * Returns the JAR file to generate, based on an optional classifier.
+     * {@return the scope of dependencies}
+     * It should be {@link PathScope#MAIN_COMPILE} or {@link 
PathScope#TEST_COMPILE}.
+     * Note that we use compile scope rather than runtime scope because 
dependencies
+     * cannot appear in {@code requires} statement if they didn't had compile 
scope.
+     */
+    protected abstract PathScope getDependencyScope();
+
+    /**
+     * {@return the JAR tool to use for archiving the code}
      *
-     * @param basedir the output directory
-     * @param resultFinalName the name of the JAR file
-     * @param classifier an optional classifier
-     * @return the file to generate
+     * @throws MojoException if no JAR tool was found
+     *
+     * @since 4.0.0-beta-2
      */
-    protected Path getJarFile(Path basedir, String resultFinalName, String 
classifier) {
-        Objects.requireNonNull(basedir, "basedir is not allowed to be null");
-        Objects.requireNonNull(resultFinalName, "finalName is not allowed to 
be null");
-        String fileName = resultFinalName + (hasClassifier(classifier) ? '-' + 
classifier : "") + ".jar";
-        return basedir.resolve(fileName);
+    protected ToolProvider getJarTool() throws MojoException {
+        return ToolProvider.findFirst(toolId).orElseThrow(() -> new 
MojoException("No such \"" + toolId + "\" tool."));
     }
 
     /**
-     * Generates the JAR.
+     * Returns the output time stamp or, as a fallback, the {@code 
SOURCE_DATE_EPOCH} environment variable.
+     * If the time stamp is expressed in seconds, it is converted to ISO 8601 
format. Otherwise it is returned as-is.
      *
-     * @return the path to the created archive file
-     * @throws MojoException in case of an error
+     * @return the time stamp in presumed ISO 8601 format, or {@code null} if 
none
+     *
+     * @since 4.0.0-beta-2
      */
-    public Path createArchive() throws MojoException {
-        Path basedir = outputDirectory != null
-                ? outputDirectory
-                : Path.of(project.getBuild().getDirectory());
-        String resultFinalName =
-                finalName != null ? finalName : 
project.getBuild().getFinalName();
-        Path jarFile = getJarFile(basedir, resultFinalName, getClassifier());
-
-        FileSetManager fileSetManager = new FileSetManager();
-        FileSet jarContentFileSet = new FileSet();
-        
jarContentFileSet.setDirectory(getClassesDirectory().toAbsolutePath().toString());
-        jarContentFileSet.setIncludes(Arrays.asList(getIncludes()));
-        jarContentFileSet.setExcludes(Arrays.asList(getExcludes()));
-
-        String[] includedFiles = 
fileSetManager.getIncludedFiles(jarContentFileSet);
-
-        if (detectMultiReleaseJar
-                && Arrays.stream(includedFiles)
-                        .anyMatch(
-                                p -> p.startsWith("META-INF" + 
File.separatorChar + "versions" + File.separatorChar))) {
-            getLog().debug("Adding 'Multi-Release: true' manifest entry.");
-            archive.addManifestEntry(Attributes.Name.MULTI_RELEASE.toString(), 
"true");
+    protected String getOutputTimestamp() {
+        String time = nullIfAbsent(outputTimestamp);
+        if (time == null) {
+            time = nullIfAbsent(System.getenv("SOURCE_DATE_EPOCH"));
+            if (time == null) {
+                return null;
+            }
         }
+        if (Runtime.version().feature() < ToolExecutor.JDK_SUPPORT_DATE) {
+            log.warn("Reproducible build requires Java " + 
ToolExecutor.JDK_SUPPORT_DATE + " or later.");

Review Comment:
   Actually reproducible builds are never explicitly requested. We only infer 
that requirement from whether a value has been specified to the 
`outputTimestamp` property. By relaxing the coupling between whether 
reproducible builds is requested and whether the `--date` option is supported, 
we can address some of the issues.
   
   For the last remaining issue, when `--date` is not supported, we tried to 
replace the warning by an exception. But because `outputTimestamp` is defined 
in the super-POM since Maven 4,0.0, it causes all builds to fail by default on 
Java 17/18. It is not so easy to opt-out, as clearing the ` outputTimestamp` 
causes exceptions to be thrown in some combinations of Maven core and Maven 
Compiler Plugins 3.x, and users wanting reproducible builds on a "best effort" 
basis have to setup profiles.
   
   In the case of Maven JAR plugin, this proposal causes many integration tests 
to fail, even with opt-out. I think that throwing an exception here causes more 
difficulties than benefits.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to