Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-1215.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Apply `--add-exports` to Java 16+ services based on manifest entries
links:
- https://github.com/palantir/sls-packaging/pull/1215
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,6 @@ public abstract class LaunchConfigTask extends DefaultTask {
ImmutableList.of("-XX:+ShowCodeDetailsInExceptionMessages");
private static final ImmutableList<String> java15Options =
ImmutableList.of("-XX:+UnlockDiagnosticVMOptions", "-XX:+ExpandSubTypeCheckAtParseTime");
// Support safepoint metrics from the internal sun.management package in production. We prefer not
// to use '--illegal-access=permit' so that we can avoid unintentional and unbounded illegal access
// that we aren't aware of.
private static final ImmutableList<String> java16PlusOptions =
ImmutableList.of("--add-exports", "java.management/sun.management=ALL-UNNAMED");
private static final ImmutableList<String> disableBiasedLocking = ImmutableList.of("-XX:-UseBiasedLocking");

private static final ImmutableList<String> alwaysOnJvmOptions = ImmutableList.of(
Expand Down Expand Up @@ -199,10 +194,7 @@ public final void createConfig() throws IOException {
javaVersion.get().compareTo(JavaVersion.toVersion("15")) < 0
? disableBiasedLocking
: ImmutableList.of())
.addAllJvmOpts(
javaVersion.get().compareTo(JavaVersion.toVersion("16")) >= 0
? java16PlusOptions
: ImmutableList.of())
.addAllJvmOpts(ModuleExports.getExports(getProject(), javaVersion.get(), getClasspath()))
.addAllJvmOpts(gcJvmOptions.get())
.addAllJvmOpts(defaultJvmOpts.get())
.putAllEnv(defaultEnvironment)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* (c) Copyright 2021 Palantir Technologies Inc. All rights reserved.
*
* Licensed 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 com.palantir.gradle.dist.service.tasks;

import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.util.jar.JarFile;
import java.util.stream.Stream;
import org.gradle.api.JavaVersion;
import org.gradle.api.Project;
import org.gradle.api.file.FileCollection;

/**
* Utility functionality which mirrors {@code Add-Exports} plumbing from
* <a href="https://github.com/palantir/gradle-baseline/pull/1944">gradle-baseline#1944</a>.
*/
final class ModuleExports {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to to also have a small explanation here instead of just linking to the PR. Something similar to what you added in:

https://github.com/palantir/gradle-baseline/blob/1a052650da4af39680a7dd3f790a5b291086158e/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineModuleJvmArgs.java#L42-L47

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, updated!


private static final String ADD_EXPORTS_ATTRIBUTE = "Add-Exports";
private static final Splitter EXPORT_SPLITTER =
Splitter.on(' ').trimResults().omitEmptyStrings();

// Exists for backcompat until infrastructure has rolled out with Add-Exports manifest values.
// Support safepoint metrics from the internal sun.management package in production. We prefer not
// to use '--illegal-access=permit' so that we can avoid unintentional and unbounded illegal access
// that we aren't aware of.
private static final ImmutableList<String> DEFAULT_EXPORTS = ImmutableList.of("java.management/sun.management");

static ImmutableList<String> getExports(Project project, JavaVersion javaVersion, FileCollection classpath) {
// --add-exports is unnecessary prior to java 16
if (javaVersion.compareTo(JavaVersion.toVersion("16")) < 0) {
return ImmutableList.of();
}

return Stream.concat(
classpath.getFiles().stream().flatMap(file -> {
try {
if (file.getName().endsWith(".jar") && file.isFile()) {
JarFile jar = new JarFile(file);
Copy link
Contributor

Choose a reason for hiding this comment

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

driveby: shouldn't JarFiles be in a try-with-resources block?

    public void close() throws IOException {
        if (closeRequested) {
            return;
        }
        closeRequested = true;

        synchronized (this) {
            // Close streams, release their inflaters, release cached inflaters
            // and release zip source
            try {
                res.clean();
            } catch (UncheckedIOException ioe) {
                throw ioe.getCause();
            }
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout, we should have an error-prone rule for this!

String value = jar.getManifest()
.getMainAttributes()
.getValue(ADD_EXPORTS_ATTRIBUTE);
if (Strings.isNullOrEmpty(value)) {
return Stream.empty();
}
project.getLogger()
.debug(
"Found manifest entry {}: {} in jar {}",
ADD_EXPORTS_ATTRIBUTE,
value,
file);
return EXPORT_SPLITTER.splitToStream(value);
}
return Stream.empty();
} catch (IOException e) {
project.getLogger().warn("Failed to check jar {} for manifest attributes", file, e);
return Stream.empty();
}
}),
DEFAULT_EXPORTS.stream())
.distinct()
.sorted()
.flatMap(modulePackagePair -> Stream.of("--add-exports", modulePackagePair + "=ALL-UNNAMED"))
.collect(ImmutableList.toImmutableList());
}

private ModuleExports() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ import com.palantir.gradle.dist.GradleIntegrationSpec
import com.palantir.gradle.dist.SlsManifest
import com.palantir.gradle.dist.Versions
import com.palantir.gradle.dist.service.tasks.LaunchConfigTask

import java.util.jar.Attributes
import java.util.jar.JarOutputStream
import java.util.jar.Manifest
import java.util.zip.ZipFile
import org.gradle.testkit.runner.TaskOutcome
import org.junit.Assert
Expand Down Expand Up @@ -1131,6 +1135,45 @@ class JavaServiceDistributionPluginTests extends GradleIntegrationSpec {
])
}

def 'applies exports based on classpath manifests'() {
Manifest manifest = new Manifest()
manifest.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0")
manifest.getMainAttributes().putValue('Add-Exports', 'jdk.compiler/com.sun.tools.javac.file')
File testJar = new File(getProjectDir(),"test.jar");
testJar.withOutputStream { fos ->
new JarOutputStream(fos, manifest).close()
}
createUntarBuildFile(buildFile)
buildFile << """
dependencies {
implementation files("test.jar")
javaAgent "net.bytebuddy:byte-buddy-agent:1.10.21"
}
tasks.jar.archiveBaseName = "internal"
distribution {
javaVersion 17
}""".stripIndent()
file('src/main/java/test/Test.java') << "package test;\npublic class Test {}"

when:
runTasks(':build', ':distTar', ':untar')

then:
def actualOpts = OBJECT_MAPPER.readValue(
file('dist/service-name-0.0.1/service/bin/launcher-static.yml'),
LaunchConfigTask.LaunchConfig)
.jvmOpts()

// Quick check
actualOpts.containsAll([
"--add-exports",
"jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED"])

// Verify args are set in the correct order
int compilerPairIndex = actualOpts.indexOf("jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED")
actualOpts.get(compilerPairIndex - 1) == "--add-exports"
}

private static createUntarBuildFile(File buildFile) {
buildFile << '''
plugins {
Expand Down