Added OSGi metadata via build #6282
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,25 @@ | ||
| apply plugin: 'me.champeau.gradle.japicmp' | ||
| applyOsgi(this) | ||
|
|
||
| jar { | ||
| // modify these lines for MANIFEST.MF properties or for specific bnd instructions | ||
| // internal package needs to be exported too because sibling libraries use internal packages | ||
| // if a dependency is added which is not necessarily required at runtime add the packages to the optional list | ||
| bnd ''' | ||
| Export-Package: \ | ||
| okhttp3,\ | ||
| okhttp3.internal.* | ||
|
There was a problem hiding this comment. Is there a concept of a friend package? There was a problem hiding this comment. See okhttp-tls comment There was a problem hiding this comment. in zipkin we do things like this.. export your internal package.. explicitly import when internal is used block import when something is source retention.. There was a problem hiding this comment. Just to be sure I understand the approach you set the "braveinternal" attribute mandatory* which enforces that the "braveinternal" attribute is set for every Import-Package statement? Therefore if someone wants to Import one of the internal packages the "braveinternal" attribute must explicitly set otherwise the bundle would not resolve? So for my understanding this would be like a "I agree that what I'm importing is not public API and can change at will". I kind of like that approach because it prevents accidently importing a package which you shouldn't because it's internal. * For Reference: https://docs.osgi.org/specification/osgi.core/7.0.0/framework.module.html#i2515263 There was a problem hiding this comment. I think you understand it, yes. We have had bugs where we forgot to explicitly import the internal thing, but I think it is a better optimization anyway. There was a problem hiding this comment. I did hope that bnd does not specify mandatory attributes automatically but I have just tried it and bnd does in fact automatically infer mandatory attributes for Import-Package statements. Therefore if someone is using bnd as a build system he wouldn't even notice that there is an mandatory attribute called "okhttpinternal"/"braveinternal"... There was a problem hiding this comment. trying to understand the impact of this.. third parties outside this repo shouldn't be using the internal code anyway, so should be not impacted by ignorance of them. Only here, we have to make sure the bnds are coherent.. am I missing something? There was a problem hiding this comment. We want to ensure that only the okhttp sibling libraries are able to use the internal package of the main okhttp project. Therefore we add a mandatory "okhttpinternal" directive which ensures that for each Import-Package: okhttp3.internal statement the attribute must be explicitly defined. For us it's easy to define that attribute for the siblings okhttp3 libraries which use those internal packages. If a 3rdparty consumer would also import one of the internal packages without defining the attribute the bundle would resolve with an error according to OSGi spec because the attribute is not defined. Manifest-Version: 1.0 Automatic-Module-Name: okhttp3.sse Bnd-LastModified: 1601454505191 Bundle-ManifestVersion: 2 Bundle-Name: com.squareup.okhttp3.sse Bundle-SymbolicName: com.squareup.okhttp3.sse Bundle-Version: 4.10.0.SNAPSHOT Created-By: 14.0.2 (AdoptOpenJDK) Export-Package: okhttp3.sse;uses:="kotlin,kotlin.jvm,okhttp3";version= "4.10.0" Import-Package: kotlin;version="[1.4,2)",kotlin.io;version="[1.4,2)",k otlin.jvm;version="[1.4,2)",kotlin.jvm.internal;version="[1.4,2)",okh ttp3;version="[4.10,5)",okhttp3.internal;version="[4.10,5)";okhttpint ernal=true,okhttp3.internal.connection;version="[4.10,5)";okhttpinter nal=true,okio Private-Package: okhttp3.internal.sse Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))" Tool: Bnd-5.1.2.202007211702 There was a problem hiding this comment. If this automatically sucks in the internal stuff, it would seem to be a behavioral difference in manifest generation here vs for example maven-bundle-plugin. This might be a different interpretation of what is sane behavior, or simply an accident in one or the other. In any case, would you agree that the internal attribution is important to annotate even the gradle plugin or default configuration thwarts that guard rail? cc also @helgoboss for input as it is sometimes hard for project teams to come to a sane approach for things like this if they aren't actively using the tooling. There was a problem hiding this comment. I have added the attribute although when bnd is used the benefit is negligible. I did not have to explicitly define the import in the sibling libraries because bnd adds the imports with the property set to true automatically... |
||
| Import-Package: \ | ||
| android.*;resolution:=optional,\ | ||
| dalvik.system;resolution:=optional,\ | ||
| org.conscrypt;resolution:=optional,\ | ||
| org.bouncycastle.*;resolution:=optional,\ | ||
| org.openjsse.*;resolution:=optional,\ | ||
|
There was a problem hiding this comment. If we introduce new dependencies, is there a test that’ll fail to remind us to update this? There was a problem hiding this comment. If new dependencies are introduced you have to decide on a case by case basis if it's an optional dependency or not. If you forget to add the library in the list the worst case will be that a OSGi user of that library has to put that library on the classpath to ensure OSGi can resolve all imports. The resolution optional attribute basically means that those packages are not required at runtime. There was a problem hiding this comment. maybe a move the comment about optional (ex why not
There was a problem hiding this comment. If we add a simple "*" here all the android/ssl packages would be required on the classpath for the bundle to resolve therefore I explicitly define the packages which are not required in every case at runtime. Hmm... maybe we should add the comment to the build.gradle dependency block instead of the platform class because the dependency block must be touched to add new dependencies. I kind of dislike adding comments to code which are build related. There was a problem hiding this comment. I suppose I meant whatever the package-info equiv of kotlin. I suspect any new member of this directory would possibly taint here. https://github.com/square/okhttp/tree/master/okhttp/src/main/kotlin/okhttp3/internal/platform Not too fussed where the comment goes, it can be in the CONTRIBUTING file. thx for the clarification on not '*' that part could be a comment inline here in the gradle. There was a problem hiding this comment. Added documentation to the CONTRIBUTING file |
||
| sun.security.ssl;resolution:=optional,\ | ||
| * | ||
| Automatic-Module-Name: okhttp3 | ||
| Bundle-SymbolicName: com.squareup.okhttp3 | ||
| ''' | ||
| } | ||
|
|
||
| sourceSets { | ||
|
|
@@ -21,6 +37,16 @@ task copyJavaTemplates(type: Copy) { | |
| filteringCharset = 'UTF-8' | ||
| } | ||
|
|
||
| // make osgiTestImplementation configuration jars available to the test environment | ||
| configurations { | ||
| osgiTestImplementation | ||
| } | ||
| task copyOsgiTestDeployment(type: Copy) { | ||
| from configurations.osgiTestImplementation | ||
| into "${buildDir}/resources/test/okhttp3/osgi/deployments" | ||
| } | ||
| tasks.test.dependsOn(copyOsgiTestDeployment) | ||
|
|
||
| dependencies { | ||
| api deps.okio | ||
| api deps.kotlinStdlib | ||
|
|
@@ -37,10 +63,16 @@ dependencies { | |
| testImplementation project(':okhttp-urlconnection') | ||
| testImplementation project(':mockwebserver') | ||
| testImplementation project(':okhttp-logging-interceptor') | ||
| testImplementation project(':okhttp-brotli') | ||
| testImplementation project(':okhttp-dnsoverhttps') | ||
| testImplementation project(':okhttp-sse') | ||
| testImplementation deps.conscrypt | ||
| testImplementation deps.junit | ||
| testImplementation deps.assertj | ||
| testImplementation deps.openjsse | ||
| testImplementation deps.bndResolve | ||
| osgiTestImplementation deps.equinox | ||
| osgiTestImplementation deps.kotlinStdlibOsgi | ||
| testCompileOnly deps.jsr305 | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| package okhttp3.osgi; | ||
|
|
||
| import aQute.bnd.build.Workspace; | ||
| import aQute.bnd.service.RepositoryPlugin; | ||
| import aQute.lib.io.IO; | ||
| import biz.aQute.resolve.Bndrun; | ||
| import org.junit.Test; | ||
|
|
||
| import java.io.BufferedInputStream; | ||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.net.URISyntaxException; | ||
| import java.net.URL; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.nio.file.Paths; | ||
| import java.util.stream.Stream; | ||
|
|
||
| /** | ||
| * Tries to resolve the OSGi metadata specified in the | ||
|
There was a problem hiding this comment. Hah - fixed it 🤷♂️ |
||
| */ | ||
| public class OkHttpOsgiTest { | ||
| @Test | ||
| public void testMainModuleWithSiblings() throws Exception { | ||
| File workspaceDir = getWorkspaceDirectory().toFile(); | ||
| Path resourceDir = getResourceDirectory(); | ||
| Workspace workspace = new Workspace(workspaceDir); | ||
|
|
||
| RepositoryPlugin localRepo = workspace.getRepository("Local"); | ||
| // deploy the bundles in the deployments test directory | ||
| deployDirectory(localRepo, resourceDir.resolve("deployments")); | ||
| deployClassPath(localRepo); | ||
|
|
||
| try (Bndrun bndRun = new Bndrun(workspace, resourceDir.resolve("resolveTest.bndrun").toFile())) { | ||
| // this will fail when something is wrong | ||
| bndRun.resolve(false, true); | ||
| } | ||
| } | ||
|
|
||
| private Path getResourceDirectory() throws URISyntaxException { | ||
| return getWorkspaceDirectory().getParent(); | ||
| } | ||
|
|
||
| private Path getWorkspaceDirectory() throws URISyntaxException { | ||
| URL bndWorkspaceURL = OkHttpOsgiTest.class.getResource("ws1/cnf/build.bnd"); | ||
| Path bndWorkspaceFile = Paths.get(bndWorkspaceURL.toURI()); | ||
| return bndWorkspaceFile.getParent().getParent(); | ||
| } | ||
|
|
||
| private void deployDirectory(RepositoryPlugin repository, Path directory) throws IOException { | ||
| if (!Files.exists(directory)) | ||
| return; | ||
|
|
||
| try (Stream<Path> files = Files.list(directory)) { | ||
| files.forEach(f -> { | ||
| deployFile(repository, f); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| private void deployClassPath(RepositoryPlugin repository) { | ||
| String classpath = System.getProperty("java.class.path"); | ||
| String[] classpathEntries = classpath.split(File.pathSeparator); | ||
| for (String classPathEntry : classpathEntries) { | ||
| Path classPathFile = Paths.get(classPathEntry); | ||
| deployFile(repository, classPathFile); | ||
| } | ||
| } | ||
|
|
||
| private void deployFile(RepositoryPlugin repository, Path file) { | ||
| if (Files.isRegularFile(file)) { | ||
| String fileName = file.getFileName().toString(); | ||
| try { | ||
| deploy(repository, file); | ||
| successDeployment(fileName); | ||
| } catch (Exception e) { | ||
| failedDeployment(fileName, e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void deploy(RepositoryPlugin repository, Path bundleFile) throws Exception { | ||
| try (InputStream stream = new BufferedInputStream(IO.stream(bundleFile))) { | ||
| repository.put(stream, new RepositoryPlugin.PutOptions()); | ||
| } | ||
| } | ||
|
|
||
| private void successDeployment(String fileName) { | ||
| System.out.println("Deployed " + fileName); | ||
| } | ||
|
|
||
| private void failedDeployment(String fileName, Exception error) { | ||
| System.out.println("Failed to deploy " + fileName); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| -runfw: org.eclipse.osgi | ||
|
There was a problem hiding this comment. it feels weird to split responsibility between code and text like this. I'm not sure it is bad per se, but I think sometimes of testcontainers and enjoy seeing the setup logic in one markup (even if that is compiled :) ) There was a problem hiding this comment. I have moved the bnd instructions from the bndrun, cnf/build.bnd into code so everything should be in one place now. |
||
| -runee: JavaSE-1.8 | ||
| -runsystemcapabilities: ${native_capability} | ||
| -resolve.effective: active;skip:="osgi.service" | ||
|
|
||
| -runrequires: \ | ||
| bnd.identity;version='latest';id='com.squareup.okhttp3',\ | ||
| bnd.identity;version='latest';id='com.squareup.okhttp3.brotli',\ | ||
| bnd.identity;version='latest';id='com.squareup.okhttp3.dnsoverhttps',\ | ||
| bnd.identity;version='latest';id='com.squareup.okhttp3.logging',\ | ||
| bnd.identity;version='latest';id='com.squareup.okhttp3.sse',\ | ||
| bnd.identity;version='latest';id='com.squareup.okhttp3.tls',\ | ||
| bnd.identity;version='latest';id='com.squareup.okhttp3.urlconnection' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| -plugin.1.Local: \ | ||
| aQute.bnd.deployer.repository.LocalIndexedRepo; \ | ||
| name = Local; \ | ||
| pretty = true; \ | ||
| local = ${build}/local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a 4.2M .jar, compared to 1.4M for kotlin-stdlib, presumably because this includes kotlin-reflect.