search

eclipse-sisu / sisu-project

Sisu Inject
https://www.eclipse.org/sisu
Eclipse Public License 2.0
17 stars 15 forks source link

Based on #71, generate MANIFEST automatically #72

Closed kwin closed 1 year ago

kwin commented 1 year ago

The MANIFEST.MF is now generated automatically. It looks like this:

Manifest-Version: 1.0
Build-Jdk-Spec: 17
Created-By: Maven Integration for Eclipse
Bundle-Activator: org.eclipse.sisu.launch.SisuExtender
Bundle-Copyright: Copyright (c) 2010-present Sonatype, Inc. and others
Bundle-Description: JSR330-based container; supports classpath scanning,
  auto-binding, and dynamic auto-wiring
Bundle-DocURL: http://www.eclipse.org/sisu/
Bundle-License: "Eclipse Public License, Version 1.0";link="http://www.e
 clipse.org/legal/epl-v10.html"
Bundle-ManifestVersion: 2
Bundle-Name: org.eclipse.sisu.inject
Bundle-SCM: url="https://github.com/eclipse/sisu.inject/org.eclipse.sisu
 .inject",connection="scm:git:git://github.com/eclipse/sisu.inject.git/o
 rg.eclipse.sisu.inject",developer-connection="scm:git:git@github.com:ec
 lipse/sisu.inject.git/org.eclipse.sisu.inject",tag=HEAD
Bundle-SymbolicName: org.eclipse.sisu.inject
Bundle-Vendor: The Eclipse Foundation
Bundle-Version: 0.9.0.SNAPSHOT
Export-Package: org.eclipse.sisu;version="1.0.0";uses:="javax.inject",or
 g.eclipse.sisu.bean;version="1.0.0";uses:="com.google.inject,com.google
 .inject.spi",org.eclipse.sisu.inject;version="1.0.0";uses:="com.google.
 inject,com.google.inject.spi,javax.inject,org.eclipse.sisu,org.sonatype
 .inject",org.eclipse.sisu.launch;version="1.0.0";uses:="com.google.inje
 ct,junit.framework,org.eclipse.sisu.inject,org.eclipse.sisu.space,org.e
 clipse.sisu.wire,org.junit,org.junit.jupiter.api,org.osgi.framework,org
 .osgi.util.tracker,org.testng.annotations",org.eclipse.sisu.osgi;versio
 n="1.0.0";uses:="org.eclipse.sisu.inject,org.osgi.framework",org.eclips
 e.sisu.space;version="1.0.0";uses:="com.google.inject,javax.annotation.
 processing,javax.lang.model,javax.lang.model.element,org.eclipse.sisu.i
 nject,org.osgi.framework",org.eclipse.sisu.wire;version="1.0.0";uses:="
 com.google.inject,com.google.inject.spi",org.sonatype.inject;version="1
 .0.0";uses:="javax.inject,org.eclipse.sisu"
Import-Package: org.slf4j;resolution:=optional;version="[1.7,2)",javax.a
 nnotation;resolution:=optional;version="[1.2,2)",javax.annotation.proce
 ssing;resolution:=optional,javax.enterprise.inject;resolution:=optional
 ;version="[1.1,2)",javax.lang.model;resolution:=optional,javax.lang.mod
 el.element;resolution:=optional,javax.lang.model.util;resolution:=optio
 nal,javax.servlet;resolution:=optional,javax.tools;resolution:=optional
 ,com.google.inject.servlet;resolution:=optional;version="[1.4,2)",junit
 .framework;resolution:=optional,org.junit;resolution:=optional,org.juni
 t.jupiter.api;resolution:=optional,org.testng.annotations;resolution:=o
 ptional;version="[6.7,7)",com.google.inject;version="[1.4,2)",com.googl
 e.inject.binder;version="[1.4,2)",com.google.inject.matcher;version="[1
 .4,2)",com.google.inject.name;version="[1.4,2)",com.google.inject.spi;v
 ersion="[1.4,2)",java.io,java.lang,java.lang.annotation,java.lang.ref,j
 ava.lang.reflect,java.net,java.nio.file,java.security,java.util,java.ut
 il.concurrent,java.util.concurrent.atomic,java.util.concurrent.locks,ja
 va.util.jar,java.util.logging,java.util.regex,java.util.zip,javax.injec
 t,javax.lang.model.type,javax.servlet.http,org.eclipse.sisu;version="[1
 .0,2)",org.eclipse.sisu.inject;version="[1.0,2)",org.eclipse.sisu.osgi;
 version="[1.0,2)",org.osgi.framework;version="[1.9,2)",org.osgi.util.tr
 acker;version="[1.5,2)",org.sonatype.inject;version="[1.0,2)"
Main-Class: org.eclipse.sisu.launch.Main
Private-Package: org.eclipse.sisu.space.asm
Require-Capability: osgi.contract;osgi.contract=JavaInject;filter:="(&(o
 sgi.contract=JavaInject)(version=1.0.0))",osgi.ee;filter:="(&(osgi.ee=J
 avaSE)(version=1.7))"

compare to the manual one

Manifest-Version: 1.0
Main-Class: org.eclipse.sisu.launch.Main
Bundle-ManifestVersion: 2
Bundle-SymbolicName: org.eclipse.sisu.inject;singleton:=true
Bundle-Version: ${project.version}
Bundle-Name: Sisu-Inject (Incubation)
Bundle-RequiredExecutionEnvironment: JavaSE-1.7
Bundle-Vendor: Eclipse Sisu
Bundle-Copyright: Copyright (c) 2010-present Sonatype, Inc. and others
Bundle-DocURL: http://www.eclipse.org/sisu/
Bundle-License: http://www.eclipse.org/legal/epl-v10.html
Import-Package: javax.inject,
 org.eclipse.sisu,
 com.google.inject;version="1.3",
 com.google.inject.binder;version="1.3",
 com.google.inject.matcher;version="1.3",
 com.google.inject.name;version="1.3",
 com.google.inject.spi;version="1.3",
 org.osgi.framework;version="1.5",
 org.osgi.util.tracker;version="1.4",
 org.slf4j;resolution:=optional,
 javax.annotation;resolution:=optional,
 javax.annotation.processing;resolution:=optional,
 javax.enterprise.inject;resolution:=optional,
 javax.lang.model;resolution:=optional,
 javax.lang.model.element;resolution:=optional,
 javax.lang.model.util;resolution:=optional,
 javax.servlet;resolution:=optional,
 javax.tools;resolution:=optional,
 com.google.inject.servlet;resolution:=optional,
 junit.framework;resolution:=optional,
 org.junit;resolution:=optional,
 org.junit.jupiter.api;resolution:=optional,
 org.testng.annotations;resolution:=optional
Export-Package: org.eclipse.sisu;uses:="javax.inject",
 org.eclipse.sisu.bean;
  uses:="com.google.inject,
   com.google.inject.spi",
 org.eclipse.sisu.inject;
  uses:="javax.inject,
   com.google.inject,
   com.google.inject.spi,
   org.eclipse.sisu,
   org.sonatype.inject",
 org.eclipse.sisu.launch;
  uses:="com.google.inject,
   org.eclipse.sisu.inject,
   org.eclipse.sisu.space,
   org.eclipse.sisu.wire,
   org.osgi.framework,
   org.osgi.util.tracker,
   junit.framework",
 org.eclipse.sisu.osgi;
  uses:="com.google.inject,
   com.google.inject.spi,
   org.eclipse.sisu.inject,
   org.osgi.framework,
   org.osgi.util.tracker",
 org.eclipse.sisu.space;
  uses:="javax.inject,
   javax.annotation.processing,
   javax.lang.model,
   javax.lang.model.element,
   com.google.inject,
   com.google.inject.matcher,
   com.google.inject.spi,
   org.eclipse.sisu,
   org.eclipse.sisu.inject,
   org.osgi.framework",
 org.eclipse.sisu.wire;
  uses:="javax.inject,
   com.google.inject,
   com.google.inject.spi",
 org.sonatype.inject;x-internal:=true;uses:="org.eclipse.sisu"

The important changes are:

  1. all exported packages have a version 1.0.0
  2. imported packages have a version range with lower bound being equal to the pom dependency
  3. slight adaptations in the information purpose only headers like Bundle-Vendor, Bundle-License, ....
  4. OSGi dependencies raised to R6
  5. SisuExtender is a BundleActivator in the bundle org.eclipse.sisu.inject but only active in case the framework/system property sisu.extender.enabled is set to true (this makes the previous extender bundle completely obsolete)
  6. ASM dependency dynamically embedded and relocated during build (and no longer part of Git repo)
kwin commented 1 year ago

@mcculls I would go ahead and merge next week if you don’t have any concerns.

cstamas commented 1 year ago

Strange thing noticed with this build: When you build all (the "evil" mvn clean install), then you end up with WARNING in build:

[INFO] --- jacoco:0.8.8:report-aggregate (jacoco-report) @ org.eclipse.sisu.inject.tests ---
[INFO] Loading execution data file /home/cstamas/Worx/eclipse/sisu.inject/org.eclipse.sisu.inject.tests/target/jacoco.exec
[INFO] Analyzed bundle 'org.eclipse.sisu.inject' with 207 classes
[WARNING] Classes in bundle 'org.eclipse.sisu.inject' do not match with execution data. For report generation the same class files must be used as at runtime.
[WARNING] Execution data for class org/eclipse/sisu/space/SpaceScanner does not match.
[WARNING] Execution data for class org/eclipse/sisu/wire/DynamicGlue does not match.
[WARNING] Execution data for class org/eclipse/sisu/space/SpaceScanner$1 does not match.

But, when you do this: mvn clean install -rf :org.eclipse.sisu.inject.tests then no warnings. Seems it has to do with shading that happens in same reactor?

cstamas commented 1 year ago

Reported Jacoco issue here https://github.com/jacoco/jacoco/issues/1394

In short: this is jacoco bug, they (wrongly) source reactor project by going directly for target/classes instead to get main artifact (that is packaged, and may be post-processed, like in out case, relocations), hence the non-matching execution data: tests were executed with JAR that was including relocated ASM, while report will find non-relocated stuff.

mcculls commented 1 year ago

Hi @kwin - ideally I would prefer to keep the local copy of ASM code. There's a script which makes it really easy to update from the upstream project, and it also means we could pull in changes which haven't yet been released to central.

But if I don't get round to finishing my review this weekend then we can move to commit-then-review (CTR)

mcculls commented 1 year ago

Update: I've merged most of the commits from this PR that remove the tycho packaging and have also moved the tests under org.eclipse.sisu.inject - tomorrow (Monday) I'm going to merge in the bnd-maven-plugin changes and do some additional cleanup. I'll then repeat this process for sisu.plexus.

kwin commented 1 year ago

Hi @mcculls, thanks for merging in the first pieces. Regarding

ideally I would prefer to keep the local copy of ASM code

I disagree here. That code is not written by us and the origins should be more visible. Also this shouldn't be taken into consideration by any code coverage calculation. Having an explicit dependency in the pom.xml and letting Maven embed it allows to

Regarding the jacoco issue, we should just ignore that whole package as that should not be considered for coverage calculation. It is a third party library we just use. IMHO that should be possible to ignore with just https://www.eclemma.org/jacoco/trunk/doc/prepare-agent-mojo.html#excludes.

Can we go forward with the next steps?

mcculls commented 1 year ago

I'll be cleaning up and merging in the manifest generation this weekend - regarding ASM, we really need to repackage ASM either way to avoid packaging conflicts, so the question is whether to do this once per upgrade or on every single build.

Given builds happen much more than upgrades IMHO it makes more sense to do the repackaging only once rather than on every build. Especially since ASM is such a small dependency of only a few classes. It also helps with debugging issues that touch ASM because the source is there and matches the binaries.

Note we use ASM as an implementation detail, but we avoid exposing it via the API - we still record the attribution in the project source and the final binary. And as mentioned earlier, we now have a script that lets you update ASM to a given tag with a single command.

kwin commented 1 year ago

Everything relevant has been cherry-picked to master meanwhile.