-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added OSGi metadata via build in a non intrusive way #777
Conversation
4b9d094
to
02ca158
Compare
okio/jvm/jvm.gradle
Outdated
buildscript { | ||
repositories { | ||
maven { | ||
url "https://plugins.gradle.org/m2/" |
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 already declared in the root build.gradle
as gradlePluginPortal()
.
okio/jvm/jvm.gradle
Outdated
} | ||
} | ||
dependencies { | ||
classpath deps.bnd |
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.
Please add this to the root build.gradle
. The buildscript classpath is global and it's much easier to deal with when there is only a single place where dependencies are added.
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.
For some reason gradle does not allow to access buildscript classes directly in a gradle file that is externally applied. Therefore as a workaround I have added the class as a property to the root project to make it accessible from the jvm.gradle file.
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.
Applying plugins in applied scripts works as intended but I don't want to apply the bnd plugin because on default the bnd plugin overwrites the jar task to provide the updated MANIFEST.MF and that could be counted as "intrusive" again.
|
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.
Works for me. Thanks for doing this.
okio/jvm/jvm.gradle
Outdated
''' | ||
} | ||
|
||
// extract only the MANIFEST.MF file from the OSGi compatible JAR file |
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.
Does this part do anything meaningful, or is it just because with the old plugin things went sideways here? If this is just ceremony to make me feel better, I think we should remove it.
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.
@swankjesse to be honest it's just ceremony to make you feel better. I wanted to reduce the surface area the bnd plugins is working on (if you do not trust what the plugin does) therefore my idea was to let the plugin generate the jar file and simply use only the META-INF/MANIFEST.MF file for the jar file generated by the jvmJar task. In practice for the bnd instructions used at the moment the resulting jar file of the bnd plugin will be exactly the same as the resulting jar from the jvmJar task except that the MANIFEST.MF contains the proper OSGi metadata.
@swankjesse I have changed the code to be simpler but potentially more intrusive. |
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.
If we're gonna support OSGi, I'd like to do it in the least creative, least invasive way possible.
okio/jvm/jvm.gradle
Outdated
|
||
// configure the kotlin JVM jar task | ||
tasks['jvmJar'].configure { t -> | ||
// the bnd task conention modifies this jar task accordingly |
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.
not: conention
okio/jvm/jvm.gradle
Outdated
manifest { | ||
attributes('Automatic-Module-Name': 'okio') | ||
|
||
// configure the kotlin JVM jar task |
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.
not: delete comment
Bundle-SymbolicName: com.squareup.okio | ||
''' | ||
// call the convention when the task has finished to modify the jar to contain OSGi metadata | ||
t.doLast { |
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.
One more question: what's different between this approach and typical use of bnd? The way this stuff is hooked up seems pretty atypical! doLast and bndBundleTaskConventionClass both seem awkward.
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.
Typically you would apply the plugin itself which will overwrite the jar task and read a file in the projects directory called bnd.bnd where the multiline string should be in. With the build of okio this is not possible because the jar task itself doesn't really do anything the real "jar" task is the "jvmJar" therefore I applied the stuff the plugin does normally to the jar task to the jvmJar task instead.
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.
Thanks for the explanation
* Added OSGi metadata via build in a non intrusive way * Add bnd to root buildscript * Added bnd class to root project only * Simplified bnd usage * Fixed comments
Implementation of the build discussed in the issue #776
Fixes #776