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

Generate OSGi Manifests #211

Merged
merged 4 commits into from Jan 17, 2017

Conversation

Projects
None yet
2 participants
@Godin
Member

Godin commented Jan 4, 2017

I removed IDE dependencies with commit ae311b9. Now we can create proper MANIFEST.MF files during the build with bnd: http://felix.apache.org/site/apache-felix-maven-bundle-plugin-bnd.html

@marchof marchof added the build label May 5, 2014

@Godin Godin added this to the 0.7.2 milestone May 8, 2014

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 13, 2014

Member

@marchof While working on this ticket I found that in org.jacoco.core we export one internal package as org.jacoco.core.internal.analysis;x-internal=true, but not others like org.jacoco.core.internal.data and no export of internal packages in other bundles. Hence questions: is it intentional? should we leave it as is, or export all internal packages from all bundles with "x-internal=true", or don't export internal packages at all? I'm asking because I'm not very familiar with concept "x-internal".

Member

Godin commented May 13, 2014

@marchof While working on this ticket I found that in org.jacoco.core we export one internal package as org.jacoco.core.internal.analysis;x-internal=true, but not others like org.jacoco.core.internal.data and no export of internal packages in other bundles. Hence questions: is it intentional? should we leave it as is, or export all internal packages from all bundles with "x-internal=true", or don't export internal packages at all? I'm asking because I'm not very familiar with concept "x-internal".

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 13, 2014

Member

@Godin Because this package is used in the report.test bundle to setup test data. I know this is bad design, but can we fix it separately?

Member

marchof commented May 13, 2014

@Godin Because this package is used in the report.test bundle to setup test data. I know this is bad design, but can we fix it separately?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 13, 2014

Member

@marchof ok, so I'll leave it exactly as it is now

Member

Godin commented May 13, 2014

@marchof ok, so I'll leave it exactly as it is now

@Godin Godin modified the milestones: 0.7.2, 0.7.3 Sep 12, 2014

@Godin Godin modified the milestones: 0.7.4, 0.7.3, 0.7.5 Feb 20, 2015

@marchof marchof modified the milestone: 0.7.5 May 20, 2015

@Godin Godin added this to the 0.7.9 milestone Jan 4, 2017

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 4, 2017

Member

@marchof could you please review?

There are some notable differences in resulting files, in particular:

  • no generation of OSGi attributes for all *.test and removal of their existing OSGi attributes - anyway they are not part of distribution and hence unused
  • removed MANIFEST.MF from jacocoant.jar (the one that has all dependencies included) that previously had OSGi attributes, while is not OSGi bundle
  • all internal packages are exported with x-internal:=true directive
  • added uses directive
  • added attribute Require-Capability
Member

Godin commented Jan 4, 2017

@marchof could you please review?

There are some notable differences in resulting files, in particular:

  • no generation of OSGi attributes for all *.test and removal of their existing OSGi attributes - anyway they are not part of distribution and hence unused
  • removed MANIFEST.MF from jacocoant.jar (the one that has all dependencies included) that previously had OSGi attributes, while is not OSGi bundle
  • all internal packages are exported with x-internal:=true directive
  • added uses directive
  • added attribute Require-Capability

@Godin Godin requested a review from marchof Jan 4, 2017

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 8, 2017

Member

@Godin First of all many thanks for picking this up!

Regarding jacocoant.jar: I agree this is not a bundle. But from my point of view every JAR should have a MANIFEST.MF at least with basic meta information. Can't we add a MANIFEST.MF without any OSGi header like in jacocoagent.jar?

I wanted to test the new bundles with EclEmma but I failed to include them with the EclEmma build as the build now picks up the JaCoCo bundles from orbit. Any ideas how I can use local JaCoCo snapshots builds with EclEmma?

Member

marchof commented Jan 8, 2017

@Godin First of all many thanks for picking this up!

Regarding jacocoant.jar: I agree this is not a bundle. But from my point of view every JAR should have a MANIFEST.MF at least with basic meta information. Can't we add a MANIFEST.MF without any OSGi header like in jacocoagent.jar?

I wanted to test the new bundles with EclEmma but I failed to include them with the EclEmma build as the build now picks up the JaCoCo bundles from orbit. Any ideas how I can use local JaCoCo snapshots builds with EclEmma?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 8, 2017

Member

@marchof

Regarding jacocoant.jar: I agree this is not a bundle. But from my point of view every JAR should have a MANIFEST.MF at least with basic meta information. Can't we add a MANIFEST.MF without any OSGi header like in jacocoagent.jar?

Well, removal was easy. I'll try to add basic entries. Manifest-Version, Implementation-Title, Implementation-Vendor and Implementation-Version would be enough?

I wanted to test the new bundles with EclEmma

Nice idea! 👍 I did only diff for MANIFEST.MF files previously.

but I failed to include them with the EclEmma build as the build now picks up the JaCoCo bundles from orbit. Any ideas how I can use local JaCoCo snapshots builds with EclEmma?

You can revert eclipse/eclemma@330ec84 locally:

git revert 330ec846686c63dfc9710e7619f9f92d714a7c9b

apply patch for usage of JaCoCo 0.7.9:

diff --git i/org.eclipse.eclemma.core/META-INF/MANIFEST.MF w/org.eclipse.eclemma.core/META-INF/MANIFEST.MF
index cc88add..57782d2 100644
--- i/org.eclipse.eclemma.core/META-INF/MANIFEST.MF
+++ w/org.eclipse.eclemma.core/META-INF/MANIFEST.MF
@@ -15,9 +15,9 @@ Require-Bundle: org.eclipse.core.runtime,
  org.eclipse.debug.core,
  org.eclipse.jdt.core,
  org.eclipse.jdt.launching,
- org.jacoco.core;bundle-version="[0.7.7,0.7.8)",
- org.jacoco.agent;bundle-version="[0.7.7,0.7.8)",
- org.jacoco.report;bundle-version="[0.7.7,0.7.8)"
+ org.jacoco.core;bundle-version="[0.7.9,0.7.10)",
+ org.jacoco.agent;bundle-version="[0.7.9,0.7.10)",
+ org.jacoco.report;bundle-version="[0.7.9,0.7.10)"
 Bundle-Activator: org.eclipse.eclemma.internal.core.EclEmmaCorePlugin
 Bundle-ActivationPolicy: lazy
 Bundle-RequiredExecutionEnvironment: J2SE-1.5
diff --git i/org.eclipse.eclemma.ui/META-INF/MANIFEST.MF w/org.eclipse.eclemma.ui/META-INF/MANIFEST.MF
index 62befeb..6b1a097 100644
--- i/org.eclipse.eclemma.ui/META-INF/MANIFEST.MF
+++ w/org.eclipse.eclemma.ui/META-INF/MANIFEST.MF
@@ -6,7 +6,7 @@ Bundle-Version: 3.0.0.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Require-Bundle: org.eclipse.eclemma.core,
- org.jacoco.core;bundle-version="[0.7.7,0.7.8)",
+ org.jacoco.core;bundle-version="[0.7.9,0.7.10)",
  org.eclipse.core.expressions,
  org.eclipse.core.runtime,
  org.eclipse.debug.ui,

and

mvn clean verify -Djacoco.version=0.7.9-SNAPSHOT

will use JaCoCo artifacts from local Maven repository.

Member

Godin commented Jan 8, 2017

@marchof

Regarding jacocoant.jar: I agree this is not a bundle. But from my point of view every JAR should have a MANIFEST.MF at least with basic meta information. Can't we add a MANIFEST.MF without any OSGi header like in jacocoagent.jar?

Well, removal was easy. I'll try to add basic entries. Manifest-Version, Implementation-Title, Implementation-Vendor and Implementation-Version would be enough?

I wanted to test the new bundles with EclEmma

Nice idea! 👍 I did only diff for MANIFEST.MF files previously.

but I failed to include them with the EclEmma build as the build now picks up the JaCoCo bundles from orbit. Any ideas how I can use local JaCoCo snapshots builds with EclEmma?

You can revert eclipse/eclemma@330ec84 locally:

git revert 330ec846686c63dfc9710e7619f9f92d714a7c9b

apply patch for usage of JaCoCo 0.7.9:

diff --git i/org.eclipse.eclemma.core/META-INF/MANIFEST.MF w/org.eclipse.eclemma.core/META-INF/MANIFEST.MF
index cc88add..57782d2 100644
--- i/org.eclipse.eclemma.core/META-INF/MANIFEST.MF
+++ w/org.eclipse.eclemma.core/META-INF/MANIFEST.MF
@@ -15,9 +15,9 @@ Require-Bundle: org.eclipse.core.runtime,
  org.eclipse.debug.core,
  org.eclipse.jdt.core,
  org.eclipse.jdt.launching,
- org.jacoco.core;bundle-version="[0.7.7,0.7.8)",
- org.jacoco.agent;bundle-version="[0.7.7,0.7.8)",
- org.jacoco.report;bundle-version="[0.7.7,0.7.8)"
+ org.jacoco.core;bundle-version="[0.7.9,0.7.10)",
+ org.jacoco.agent;bundle-version="[0.7.9,0.7.10)",
+ org.jacoco.report;bundle-version="[0.7.9,0.7.10)"
 Bundle-Activator: org.eclipse.eclemma.internal.core.EclEmmaCorePlugin
 Bundle-ActivationPolicy: lazy
 Bundle-RequiredExecutionEnvironment: J2SE-1.5
diff --git i/org.eclipse.eclemma.ui/META-INF/MANIFEST.MF w/org.eclipse.eclemma.ui/META-INF/MANIFEST.MF
index 62befeb..6b1a097 100644
--- i/org.eclipse.eclemma.ui/META-INF/MANIFEST.MF
+++ w/org.eclipse.eclemma.ui/META-INF/MANIFEST.MF
@@ -6,7 +6,7 @@ Bundle-Version: 3.0.0.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Require-Bundle: org.eclipse.eclemma.core,
- org.jacoco.core;bundle-version="[0.7.7,0.7.8)",
+ org.jacoco.core;bundle-version="[0.7.9,0.7.10)",
  org.eclipse.core.expressions,
  org.eclipse.core.runtime,
  org.eclipse.debug.ui,

and

mvn clean verify -Djacoco.version=0.7.9-SNAPSHOT

will use JaCoCo artifacts from local Maven repository.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 15, 2017

Member

For the record: appearance of OSGi attributes in jacocoant.jar - is a regression that was introduced in version 0.6.1 by #56 , where we also changed it Maven coordinates.

Member

Godin commented Jan 15, 2017

For the record: appearance of OSGi attributes in jacocoant.jar - is a regression that was introduced in version 0.6.1 by #56 , where we also changed it Maven coordinates.

Godin added some commits Jan 15, 2017

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 15, 2017

Member

@Godin Still looking for a simple method to test EclEmma with local snapshot versions (also for future versions). If we could create a local p2 repo with JaCoCo snapshots we could simply add this repor to the EclEmma target definition. I tried to create a small tycho build for exactly this but did not suceed so far.

Member

marchof commented Jan 15, 2017

@Godin Still looking for a simple method to test EclEmma with local snapshot versions (also for future versions). If we could create a local p2 repo with JaCoCo snapshots we could simply add this repor to the EclEmma target definition. I tried to create a small tycho build for exactly this but did not suceed so far.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 15, 2017

Member

@marchof while I agree that method described in #211 (comment) is not simple enough, I suggest to use it now. And come back to the question about simplification of testing of EclEmma with snapshot version of JaCoCo separately, especially that difficulty is caused by changes on EclEmma side and that there are multiple possible solutions that we need to evaluate:

  • on EclEmma side we can restore dependencies and pomDependencies=consider that were removed in eclipse/eclemma@330ec84 , placing them into dedicated separate profile for JaCoCo testing
  • also I like your idea about p2 repository
Member

Godin commented Jan 15, 2017

@marchof while I agree that method described in #211 (comment) is not simple enough, I suggest to use it now. And come back to the question about simplification of testing of EclEmma with snapshot version of JaCoCo separately, especially that difficulty is caused by changes on EclEmma side and that there are multiple possible solutions that we need to evaluate:

  • on EclEmma side we can restore dependencies and pomDependencies=consider that were removed in eclipse/eclemma@330ec84 , placing them into dedicated separate profile for JaCoCo testing
  • also I like your idea about p2 repository
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 15, 2017

Member

@Godin Your first proposal would make testing quite simple:

  • Adjust JaCoCo versions in MANFEST-MFs
  • Run build with profile
Member

marchof commented Jan 15, 2017

@Godin Your first proposal would make testing quite simple:

  • Adjust JaCoCo versions in MANFEST-MFs
  • Run build with profile
@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 15, 2017

Member

@marchof do you remember that during EclipseCon Europe 2016 while testing removal of compatibility layer for older Eclipse versions in EclEmma we were observing some unwanted effects of pomDependencies=consider? 😜

Member

Godin commented Jan 15, 2017

@marchof do you remember that during EclipseCon Europe 2016 while testing removal of compatibility layer for older Eclipse versions in EclEmma we were observing some unwanted effects of pomDependencies=consider? 😜

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 15, 2017

Member

@Godin Then, alternatively we add a profile to JaCoCo which creates a p2 repo.

Member

marchof commented Jan 15, 2017

@Godin Then, alternatively we add a profile to JaCoCo which creates a p2 repo.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 16, 2017

Member

@marchof btw did you noticed that this PR was updated with fix for jacocoant.jar (tricky one - see comments inside of pom.xml) and update for changelog?

Member

Godin commented Jan 16, 2017

@marchof btw did you noticed that this PR was updated with fix for jacocoant.jar (tricky one - see comments inside of pom.xml) and update for changelog?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jan 16, 2017

Member

@marchof you can easily create p2 repository containing snapshot version of JaCoCo using https://github.com/reficio/p2-maven-plugin Just execute mvn p2:site in a directory with following pom.xml

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001 XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
  <modelVersion>4.0.0</modelVersion>

  <groupId>org.example</groupId>
  <artifactId>example</artifactId>
  <packaging>pom</packaging>
  <version>0.1-SNAPSHOT</version>

  <properties>
    <jacoco.version>0.7.9-SNAPSHOT</jacoco.version>
  </properties>

  <build>
    <plugins>
      <plugin>
        <groupId>org.reficio</groupId>
        <artifactId>p2-maven-plugin</artifactId>
        <version>1.1.1</version>
        <configuration>
          <artifacts>
            <artifact><id>org.jacoco:org.jacoco.core:${jacoco.version}</id></artifact>
            <artifact><id>org.jacoco:org.jacoco.report:${jacoco.version}</id></artifact>
            <artifact><id>org.jacoco:org.jacoco.agent:${jacoco.version}</id></artifact>
          </artifacts>
        </configuration>
      </plugin>
    </plugins>
  </build>
</project>

result will be in target/repository.

Member

Godin commented Jan 16, 2017

@marchof you can easily create p2 repository containing snapshot version of JaCoCo using https://github.com/reficio/p2-maven-plugin Just execute mvn p2:site in a directory with following pom.xml

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001 XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
  <modelVersion>4.0.0</modelVersion>

  <groupId>org.example</groupId>
  <artifactId>example</artifactId>
  <packaging>pom</packaging>
  <version>0.1-SNAPSHOT</version>

  <properties>
    <jacoco.version>0.7.9-SNAPSHOT</jacoco.version>
  </properties>

  <build>
    <plugins>
      <plugin>
        <groupId>org.reficio</groupId>
        <artifactId>p2-maven-plugin</artifactId>
        <version>1.1.1</version>
        <configuration>
          <artifacts>
            <artifact><id>org.jacoco:org.jacoco.core:${jacoco.version}</id></artifact>
            <artifact><id>org.jacoco:org.jacoco.report:${jacoco.version}</id></artifact>
            <artifact><id>org.jacoco:org.jacoco.agent:${jacoco.version}</id></artifact>
          </artifacts>
        </configuration>
      </plugin>
    </plugins>
  </build>
</project>

result will be in target/repository.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 17, 2017

Member

@Godin Yep, I noticed the change for jacocoant.jar! I still want to test whether the bundles actually work in an OSGi environment, i.e. building EclEmma with it. p2-maven-plugin works for me, thanks!

Member

marchof commented Jan 17, 2017

@Godin Yep, I noticed the change for jacocoant.jar! I still want to test whether the bundles actually work in an OSGi environment, i.e. building EclEmma with it. p2-maven-plugin works for me, thanks!

@marchof
  • All MANIFEST.MF look good now
  • Successfully built and installed EclEmma with new bundles
@marchof

@Godin Same problem again, cannot approve review -- can you please merge anyways? Thx

@Godin Godin merged commit d1a0f19 into master Jan 17, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Godin Godin deleted the issue-211 branch Jan 17, 2017

@jacoco jacoco locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.