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.

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

Add module-info support #3222

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -6,7 +6,7 @@ plugins {

autostyle {
kotlinGradle {
ktlint()
ktlint("0.37.2")
trimTrailingWhitespace()
endWithNewline()
}
Expand Down
22 changes: 22 additions & 0 deletions pgjdbc-module-test/build.gradle.kts
@@ -0,0 +1,22 @@
import com.github.vlsi.gradle.dsl.configureEach

plugins {
id("build-logic.java-library")
`jvm-test-suite`
}

tasks.configureEach<JavaCompile> {
options.release = 11
}

testing {
suites {
val test by getting(JvmTestSuite::class) {
useJUnitJupiter("5.10.2")
dependencies {
implementation(platform("org.junit:junit-bom:5.10.2"))
implementation(project(":postgresql"))
}
}
}
}
5 changes: 5 additions & 0 deletions pgjdbc-module-test/src/test/java/module-info.java
@@ -0,0 +1,5 @@
open module org.postgresql.test.module {
requires org.junit.jupiter.api;
requires org.postgresql.jdbc;
requires java.sql;
}
@@ -0,0 +1,35 @@
package org.postgresql.test.module;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.fail;

import org.postgresql.Driver;

import org.junit.jupiter.api.Test;

public class PlainModuleTest {
@Test
public void moduleShouldNotBeAutomatic() {
var module = Driver.class.getModule().getDescriptor();
assertFalse(module.isAutomatic());
}

@Test
public void driverVersionShouldBePositive() throws Exception {
Class<?> driverClass = Class.forName("org.postgresql.Driver");
java.sql.Driver driver = (java.sql.Driver) driverClass.getConstructor().newInstance();

// We use regular assert instead of hamcrest since
// org.hamcrest.Matchers not found by org.ops4j.pax.tipi.hamcrest.core

assertPositive("driver.getMajorVersion()", driver.getMajorVersion());
assertPositive("driver.getMinorVersion()", driver.getMinorVersion());
}

private void assertPositive(String message, int value) {
if (value > 0) {
return;
}
fail(message + " should be positive, actual value is " + value);
}
}
14 changes: 8 additions & 6 deletions pgjdbc-osgi-test/build.gradle.kts
Expand Up @@ -74,12 +74,14 @@ tasks.test {
dependsOn(pgjdbcRepository)
systemProperty("logback.configurationFile", file("src/test/resources/logback-test.xml"))
// Regular systemProperty can't be used here as we need lazy evaluation of pgjdbcRepository
jvmArgumentProviders.add(CommandLineArgumentProvider {
listOf(
"-Dpgjdbc.org.ops4j.pax.url.mvn.repositories=" +
jvmArgumentProviders.add(
CommandLineArgumentProvider {
listOf(
"-Dpgjdbc.org.ops4j.pax.url.mvn.repositories=" +
"file:${pgjdbcRepository.singleFile.absolutePath}@snapshots@id=pgjdbc-current" +
",${project.findProperty("osgi.test.mavencentral.url")}@id=central",
"-Dpgjdbc.org.ops4j.pax.url.mvn.localRepository=file:${paxLocalCacheRepository.get().asFile.absolutePath}@id=pax-repo"
)
})
"-Dpgjdbc.org.ops4j.pax.url.mvn.localRepository=file:${paxLocalCacheRepository.get().asFile.absolutePath}@id=pax-repo"
)
}
)
}
104 changes: 74 additions & 30 deletions pgjdbc/build.gradle.kts
Expand Up @@ -71,15 +71,15 @@ val String.v: String get() = rootProject.extra["$this.version"] as String

dependencies {
constraints {
api("com.github.waffle:waffle-jna:1.9.1")
api("org.osgi:org.osgi.core:6.0.0")
api("org.osgi:org.osgi.service.jdbc:1.0.0")
api("com.github.waffle:waffle-jna:3.3.0")
Copy link
Member

Choose a reason for hiding this comment

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

Is this change required for module?

Copy link
Author

Choose a reason for hiding this comment

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

As-is, the dependencies don't have automatic module names, which causes compilation failure. The versions I bumped to do.

Copy link
Author

Choose a reason for hiding this comment

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

I could downgrade and remove the static requirements

  requires static com.sun.jna;
  requires static com.sun.jna.platform;
  requires static waffle.jna;
  requires static org.osgi.service.jdbc;
  requires static osgi.core;

from the module, but then if those classes (like PGBundleActivator) are loaded you'd see an exception

Exception in thread "main" java.lang.IllegalAccessError: superinterface check failed: class org.postgresql.osgi.PGBundleActivator (in module org.postgresql.jdbc) cannot access class org.osgi.framework.BundleActivator (in unnamed module @0x4fca772d) because module org.postgresql.jdbc does not read unnamed module @0x4fca772d

Copy link
Author

Choose a reason for hiding this comment

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

We could leave the requirements out and just mark waffle and osgi as non-required under modules. Not sure how that would affect future modular versions of waffle or osgi.

api("org.osgi:osgi.core:8.0.0")
api("org.osgi:org.osgi.service.jdbc:1.0.1")
}

"sspiImplementation"("com.github.waffle:waffle-jna")
// The dependencies are provided by OSGi container,
// so they should not be exposed as transitive dependencies
"osgiCompileOnly"("org.osgi:org.osgi.core")
"osgiCompileOnly"("org.osgi:osgi.core")
"osgiCompileOnly"("org.osgi:org.osgi.service.jdbc")
"testImplementation"("org.osgi:org.osgi.service.jdbc") {
because("DataSourceFactory is needed for PGDataSourceFactoryTest")
Expand All @@ -103,6 +103,31 @@ if (skipReplicationTests) {
}
}

val java11 by sourceSets.creating {
java.srcDir("src/main/java11")
compileClasspath = sourceSets["main"].compileClasspath
}

tasks.named<JavaCompile>("compileJava11Java") {
options.release = 11
val compileJavaDestinationDirectory = tasks.compileJava.flatMap { it.destinationDirectory }
// sets execution order and cache relationship
inputs.dir(compileJavaDestinationDirectory)
options.compilerArgumentProviders.add(
CommandLineArgumentProvider {
listOf(
"--patch-module", "org.postgresql.jdbc=${compileJavaDestinationDirectory.get().asFile.absolutePath}",
)
}
)
}

tasks.jar {
into("META-INF/versions/11") {
from(sourceSets["java11"].output)
}
}

tasks.configureEach<Test> {
outputs.cacheIf("test results on the database configuration, so we can't cache it") {
false
Expand Down Expand Up @@ -218,6 +243,7 @@ tasks.configureEach<Jar> {
manifest {
attributes["Main-Class"] = "org.postgresql.util.PGJDBCMain"
attributes["Automatic-Module-Name"] = "org.postgresql.jdbc"
Copy link
Member

Choose a reason for hiding this comment

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

Should Automatic-Module-Name be removed if we move to non-automatic modules?

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes. Actually, I was going to make a comment about this block. Doesn't this configure every jar, including javadoc and sources? Perhaps these should only be made in the main and shadow jar?

attributes["Multi-Release"] = "true"
}
}

Expand All @@ -228,9 +254,14 @@ tasks.shadowJar {
exclude("META-INF/NOTICE*")
into("META-INF") {
dependencyLicenses(shadedLicenseFiles)
into("versions/11") {
from(sourceSets["java11"].output)
}
}
// shadow excludes module-info.class files by default, see https://github.com/johnrengelman/shadow/issues/710#issuecomment-1553178619
excludes.remove("module-info.class")
listOf(
"com.ongres"
"com.ongres"
).forEach {
relocate(it, "${project.group}.shaded.$it")
}
Expand Down Expand Up @@ -262,21 +293,24 @@ val osgiJar by tasks.registering(Bundle::class) {
karaf {
features.apply {
xsdVersion = "1.5.0"
feature(closureOf<com.github.lburgazzoli.gradle.plugin.karaf.features.model.FeatureDescriptor> {
name = "postgresql"
description = "PostgreSQL JDBC driver karaf feature"
version = project.version.toString()
details = "Java JDBC 4.2 (JRE 8+) driver for PostgreSQL database"
feature("transaction-api")
includeProject = true
bundle(
project.group.toString(),
closureOf<com.github.lburgazzoli.gradle.plugin.karaf.features.model.BundleDescriptor> {
wrap = false
})
// List argument clears the "default" configurations
configurations(listOf(karafFeatures))
})
feature(
closureOf<com.github.lburgazzoli.gradle.plugin.karaf.features.model.FeatureDescriptor> {
name = "postgresql"
description = "PostgreSQL JDBC driver karaf feature"
version = project.version.toString()
details = "Java JDBC 4.2 (JRE 8+) driver for PostgreSQL database"
feature("transaction-api")
includeProject = true
bundle(
project.group.toString(),
closureOf<com.github.lburgazzoli.gradle.plugin.karaf.features.model.BundleDescriptor> {
wrap = false
}
)
// List argument clears the "default" configurations
configurations(listOf(karafFeatures))
}
)
}
}

Expand All @@ -290,12 +324,13 @@ val sourceWithoutCheckerAnnotations by configurations.creating {

val hiddenAnnotation = Regex(
"@(?:Nullable|NonNull|PolyNull|MonotonicNonNull|RequiresNonNull|EnsuresNonNull|" +
"Regex|" +
"Pure|" +
"KeyFor|" +
"Positive|NonNegative|IntRange|" +
"GuardedBy|UnderInitialization|" +
"DefaultQualifier)(?:\\([^)]*\\))?")
"Regex|" +
"Pure|" +
"KeyFor|" +
"Positive|NonNegative|IntRange|" +
"GuardedBy|UnderInitialization|" +
"DefaultQualifier)(?:\\([^)]*\\))?"
)
val hiddenImports = Regex("import org.checkerframework")

val removeTypeAnnotations by tasks.registering(Sync::class) {
Expand Down Expand Up @@ -356,11 +391,13 @@ val sourceDistribution by tasks.registering(Tar::class) {
}
dependsOn(tasks.generateKar)
into("src/main/resources") {
from(tasks.jar.map {
zipTree(it.archiveFile).matching {
include("META-INF/MANIFEST.MF")
from(
tasks.jar.map {
zipTree(it.archiveFile).matching {
include("META-INF/MANIFEST.MF")
}
}
})
)
into("META-INF") {
dependencyLicenses(shadedLicenseFiles)
}
Expand Down Expand Up @@ -420,3 +457,10 @@ val extraMavenPublications by configurations.getting
classifier = "features"
}
}

sourceSets {
main {
// todo: fix how preprocessVersion srcSets
java.setSrcDirs(listOf("src/main/java", preprocessVersion))
}
}
5 changes: 4 additions & 1 deletion pgjdbc/src/main/java/org/postgresql/sspi/SSPIClient.java
Expand Up @@ -229,7 +229,10 @@ public void continueSSPI(int msgLength) throws SQLException, IOException {
SEC_BUFFER_DESC_FACTORY.newInstance(Sspi.SECBUFFER_TOKEN, receivedToken);
} catch (ReflectiveOperationException ex) {
// A fallback just in case
continueToken = new SecBufferDesc(Sspi.SECBUFFER_TOKEN, receivedToken);
Sspi.SecBuffer[] secBuffers = new Sspi.SecBuffer[]{new Sspi.SecBuffer(Sspi.SECBUFFER_TOKEN, receivedToken)};
continueToken = new SecBufferDesc();
continueToken.pBuffers = secBuffers[0].getPointer();
continueToken.cBuffers = secBuffers.length;
}

sspiContext.initialize(sspiContext.getHandle(), continueToken, castNonNull(targetName));
Expand Down
18 changes: 18 additions & 0 deletions pgjdbc/src/main/java11/module-info.java
@@ -0,0 +1,18 @@
module org.postgresql.jdbc {
requires static java.desktop;
requires java.logging;
requires java.management;
requires java.naming;
requires java.security.jgss;
requires java.sql;
requires java.transaction.xa;
requires static org.checkerframework.checker.qual;
requires static com.sun.jna;
requires static com.sun.jna.platform;
requires static waffle.jna;
requires static org.osgi.service.jdbc;
requires static osgi.core;

exports org.postgresql;
Copy link
Member

Choose a reason for hiding this comment

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

I think we might need to declare more packages for the exports

Copy link
Author

Choose a reason for hiding this comment

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

We could mark the whole module as open, or try to be more selective. Is there a list of packages that should be exported?

provides java.sql.Driver with org.postgresql.Driver;
}
1 change: 1 addition & 0 deletions settings.gradle.kts
Expand Up @@ -38,6 +38,7 @@ includeBuild("build-logic")

// Renovate treats names as dependency coordinates when vararg include(...) is used, so we have separate include calls here
include("benchmarks")
include("pgjdbc-module-test")
include("pgjdbc-osgi-test")
include("postgresql")

Expand Down