Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MCOMPILER-542] rework log and conditions to run #1

Merged
merged 1 commit into from
Nov 28, 2023
Merged

[MCOMPILER-542] rework log and conditions to run #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions src/it/MCOMPILER-542/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,46 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>@project.version@</version>
<configuration>
<release>${java.specification.version}</release>
</configuration>
</plugin>
</plugins>
</pluginManagement>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<executions>
<execution>
<id>default-compile</id>
<goals>
<goal>compile</goal>
</goals>
<configuration>
<release>${java.specification.version}</release>
</configuration>
</execution>
<execution>
<id>compile-release-9</id>
<goals>
<goal>compile</goal>
</goals>
<configuration>
<release>9</release>
<outputDirectory>${project.build.outputDirectory}-9</outputDirectory>
</configuration>
</execution>
<execution>
<id>compile-target</id>
<goals>
<goal>compile</goal>
</goals>
<configuration>
<source>${java.specification.version}</source>
<target>${java.specification.version}</target>
<outputDirectory>${project.build.outputDirectory}-target</outputDirectory>
</configuration>
</execution>
</executions>
</plugin>
</plugins>
</build>
</project>
3 changes: 2 additions & 1 deletion src/it/MCOMPILER-542/verify.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ if (javaVersion != javaSpecVersion) { // handle the case when is the first relea
}
assert out.contains('// ' + javaSpecVersion) : "java specification version not found in module descriptor"

// Additional validation that the checksum is always the same
// Additional validation that the checksum is always the same: useful because constant pool reordering happens when
// transforming bytecode, then we need to check results precisely
def checksumMap = [
'21': 'SHA-256 checksum ccc6515c8fc1bf4e675e205b2a5200d02545b06014b304c292eeddc68cffee8d',
'17': 'SHA-256 checksum 102f24c71aff97210d66ef791b7d56f8a25ff8692d2c97b21682bc7170aaca9c',
Expand Down
21 changes: 18 additions & 3 deletions src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,16 @@ public abstract class AbstractCompilerMojo extends AbstractMojo {

@Parameter(defaultValue = "false", property = "maven.compiler.showCompilationChanges")
private boolean showCompilationChanges = false;

/**
* Timestamp for reproducible output archive entries, either formatted as ISO 8601
* <code>yyyy-MM-dd'T'HH:mm:ssXXX</code> or as an int representing seconds since the epoch (like
* <a href="https://reproducible-builds.org/docs/source-date-epoch/">SOURCE_DATE_EPOCH</a>).
* @since 3.12.0
*/
@Parameter(defaultValue = "${project.build.outputTimestamp}")
private String outputTimestamp;

/**
* Resolves the artifacts needed.
*/
Expand Down Expand Up @@ -1186,7 +1196,10 @@ public void execute() throws MojoExecutionException, CompilationFailureException
}
}

patchJdkModuleVersion(compilerResult, sources);
if (outputTimestamp != null && (outputTimestamp.length() > 1 || Character.isDigit(outputTimestamp.charAt(0)))) {
// if Reproducible Builds mode, apply workaround
patchJdkModuleVersion(compilerResult, sources);
}

if (useIncrementalCompilation) {
if (incrementalBuildHelperRequest.getOutputDirectory().exists()) {
Expand Down Expand Up @@ -1811,7 +1824,7 @@ final String getImplicit() {
}

/**
* Patch module-info.class to set the java release version for java/jdk modules.
* JDK-8318913 workaround: Patch module-info.class to set the java release version for java/jdk modules.
*
* @param compilerResult should succeed.
* @param sources the list of the source files to check for the "module-info.java"
Expand All @@ -1827,7 +1840,9 @@ private void patchJdkModuleVersion(CompilerResult compilerResult, Set<File> sour
final byte[] descriptorOriginal = Files.readAllBytes(moduleDescriptor);
final byte[] descriptorMod =
ModuleInfoTransformer.transform(descriptorOriginal, getRelease(), getLog());
Files.write(moduleDescriptor, descriptorMod);
if (descriptorMod != null) {
Files.write(moduleDescriptor, descriptorMod);
}
} catch (IOException ex) {
throw new MojoExecutionException("Error reading or writing module-info.class", ex);
}
Expand Down
20 changes: 18 additions & 2 deletions src/main/java/org/apache/maven/plugin/compiler/ModuleInfoTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
package org.apache.maven.plugin.compiler;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import org.apache.maven.plugin.logging.Log;
import org.objectweb.asm.ClassReader;
Expand All @@ -28,12 +30,18 @@
import org.objectweb.asm.ModuleVisitor;
import org.objectweb.asm.Opcodes;

/**
* {@code module-info.class} bytecode transformer for
* <a href="https://issues.apache.org/jira/browse/MCOMPILER-542">MCOMPILER-542</a>:
* drops detailed JDK version.
*/
final class ModuleInfoTransformer {

private ModuleInfoTransformer() {}

static byte[] transform(byte[] originalBytecode, String javaVersion, Log log) {
List<String> modulesModified = new ArrayList<>();
Set<String> foundVersions = new HashSet<>();
ClassReader reader = new ClassReader(originalBytecode);
ClassWriter writer = new ClassWriter(0);

Expand All @@ -45,10 +53,11 @@ public ModuleVisitor visitModule(String name, int access, String version) {
@Override
public void visitRequire(String module, int access, String version) {
// Check if the module name matches the java/jdk modules
if (module.startsWith("java.") || module.startsWith("jdk.")) {
if (version != null && (module.startsWith("java.") || module.startsWith("jdk."))) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This null check will preserve the current behavior when compiling from a newer JDK to an older target (eg. using JDK 11 to compile the module-info using --release 9).
In other words, it will not patch anything, while this is perfectly valid, the JDK-8318913 aims to modify this behavior to set the simple version always, I'm ok to preserve the current behavior and check later if we need to drop or not this null check.

// Patch the version from the java.* and jdk.* modules
// with the --release N version.
super.visitRequire(module, access, javaVersion);
foundVersions.add(version);
modulesModified.add(module);
} else {
// Keep the original require statement
Expand All @@ -61,7 +70,14 @@ public void visitRequire(String module, int access, String version) {

reader.accept(classVisitor, 0);

log.info(String.format("Patch module-info.class %s with version %s", modulesModified, javaVersion));
if (modulesModified.isEmpty()) {
return null;
}

log.info(String.format(
"JDK-8318913 workaround: patched module-info.class requires version from %s to [%s] on %d JDK modules %s",
foundVersions, javaVersion, modulesModified.size(), modulesModified));

return writer.toByteArray();
}
}