Skip to content

Commit

Permalink
make 'auto' the new default
Browse files Browse the repository at this point in the history 
This makes 'auto' the default again and fixes the previously failing tests.
The problem was that the "@Serviceprovider" from our own classpath added Java-17 (or whatever was the Java version on the machine you are compiling) to the list of ees which had an impact on how osgi.ee was calculated... this broke all the tests.
the fix was in Analyzer to ignore all classes added via addClasspathDefault() for calculation of the ees variable.

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>

fix failing gradle tests

filter out invalid fqn names e.g. key=value

e.g. fixes [ERROR] [org.gradle.api.Task] error  : analyzer processing annotation key=value but the associated class is not found in the JAR

because key=value is not a valid serviceImplementation name

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>

add resolution: optional to try to fix test

auto-registered annotations are added with "resolution: optional" directive, in order to fix tests which failed due to Unresolved requirements: osgi.extender; (&(osgi.extender=osgi.serviceloader.registrar)(version>=1.0.0)(!(version>=2.0.0))) which was added by the auto-registed annotations

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
  • Loading branch information
@chrisrueger
chrisrueger committed Oct 8, 2024
1 parent f31f44c commit cfbef6f
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 35 deletions.
20 changes: 16 additions & 4 deletions biz.aQute.bndlib.tests/test/test/annotationheaders/SPIAnnotationsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,12 @@ public void testServiceProvider_existingdescriptor() throws Exception {
.getMainAttributes();

Header req = Header.parseHeader(mainAttributes.getValue(Constants.REQUIRE_CAPABILITY));
assertEquals(1, req.size());
assertEquals(2, req.size());

assertEE(req);

Header cap = Header.parseHeader(mainAttributes.getValue(Constants.PROVIDE_CAPABILITY));
assertEquals(1, cap.size());
assertEquals(3, cap.size());

Props p = cap.get("osgi.serviceloader");
assertNotNull(p);
Expand Down Expand Up @@ -151,7 +151,11 @@ public void testServiceProvider_nowarning_onexisting() throws Exception {
b.getJar()
.getManifest()
.write(System.out);
assertTrue(b.check());
assertFalse(b.check());
assertThat(b.getErrors()).hasSize(1);
assertThat(b.getErrors()
.get(0)).contains(
"analyzer processing annotation some.other.Provider but the associated class is not found in the JAR");

Attributes mainAttributes = b.getJar()
.getManifest()
Expand Down Expand Up @@ -188,7 +192,15 @@ public void testServiceProvider_mergeDescriptor() throws Exception {
b.getJar()
.getManifest()
.write(System.out);
assertTrue(b.check());
assertFalse(b.check());

assertThat(b.getErrors()).hasSize(2);
assertThat(b.getErrors()
.get(0)).contains(
"analyzer processing annotation some.provider.Provider but the associated class is not found in the JAR");
assertThat(b.getErrors()
.get(1)).contains(
"analyzer processing annotation another.provider.ProviderImpl but the associated class is not found in the JAR");

Attributes mainAttributes = b.getJar()
.getManifest()
Expand Down
27 changes: 27 additions & 0 deletions biz.aQute.bndlib.tests/test/test/annotationheaders/ServiceProviderFileTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,33 @@ public void testAutoGenerateServiceProviderAnnotation() throws Exception {
}
}

@SuppressWarnings("unchecked")
@Test
public void testInvalidServiceImplementationNamesShouldBeIgnored() throws Exception {
try (Builder b = new Builder();) {
b.addClasspath(IO.getFile("bin_test"));
b.setProperty("-includeresource", """
META-INF/services/com.example.service.Type;\
literal='\
key=value'
""");
b.setProperty("-metainf-services", "auto");
b.build();
assertTrue(b.check());
Domain manifest = Domain.domain(b.getJar()
.getManifest());
Parameters provideCapability = manifest.getProvideCapability();

assertThat(provideCapability.get("osgi.service")).isNull();

Parameters requireCapability = manifest.getRequireCapability();

System.out.println(provideCapability.toString()
.replace(',', '\n'));
System.out.println(requireCapability.toString()
.replace(',', '\n'));
}
}


}
63 changes: 34 additions & 29 deletions biz.aQute.bndlib/src/aQute/bnd/osgi/Analyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ public class Analyzer extends Processor {
private final static Logger logger = LoggerFactory.getLogger(Analyzer.class);
private final static Version frameworkR7 = new Version("1.9");
private final SortedSet<Clazz.JAVA> ees = new TreeSet<>();
private final Set<Clazz> ignoreForEE = new HashSet<>();

// Bundle parameters
private Jar dot;
Expand Down Expand Up @@ -224,7 +225,7 @@ private void analyze0() throws Exception {
//
classspace.values()
.stream()
.filter(c -> !c.isModule())
.filter(c -> !c.isModule() && !ignoreForEE.contains(c))
.map(Clazz::getFormat)
.forEach(ees::add);

Expand Down Expand Up @@ -3911,33 +3912,37 @@ public void addAnnotation(Annotation ann, TypeRef c) throws Exception {
annotationHeaders.classEnd();
}

/**
* Get a class from our own class path
*
* @param type a local type on the bnd classpath
*/
public void addClasspathDefault(Class<?> type) {
assert type != null : "type must be given";

try {
TypeRef ref = getTypeRefFrom(type);
URL resource = type.getClassLoader()
.getResource(ref.getPath());
if (resource == null) {
error("analyzer.addclasspathdefault expected class %s to be on the classpath since we have a type",
type);
} else {

Resource r = new URLResource(resource, null);
Clazz c = new Clazz(this, ref.getFQN(), r);
if (c != null) {
c.parseClassFile();
classspace.putIfAbsent(ref, c);
}
}
} catch (Exception e) {
error("analyzer.findclassorlocal Failed to read a class from the bnd classpath");
}
}
/**
* Get a class from our own class path
*
* @param type a local type on the bnd classpath
*/
public void addClasspathDefault(Class<?> type) {
assert type != null : "type must be given";

try {
TypeRef ref = getTypeRefFrom(type);
URL resource = type.getClassLoader()
.getResource(ref.getPath());
if (resource == null) {
error("analyzer.addclasspathdefault expected class %s to be on the classpath since we have a type",
type);
} else {

Resource r = new URLResource(resource, null);
Clazz c = new Clazz(this, ref.getFQN(), r);
if (c != null) {
c.parseClassFile();
classspace.putIfAbsent(ref, c);

// classes we add here need to be ignored
// when calculating class versions to use
ignoreForEE.add(c);
}
}
} catch (Exception e) {
error("analyzer.findclassorlocal Failed to read a class from the bnd classpath");
}
}

}
10 changes: 10 additions & 0 deletions biz.aQute.bndlib/src/aQute/bnd/osgi/metainf/MetaInfService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public class MetaInfService {
static final String META_INF_SERVICES_STEM = "META-INF/services";
static final String META_INF_SERVICES_PREFIX = META_INF_SERVICES_STEM + "/";
static final String FQN_S = "(\\p{javaJavaIdentifierStart}\\p{javaJavaIdentifierPart}*\\.)*\\p{javaJavaIdentifierStart}\\p{javaJavaIdentifierPart}*";
static final Pattern FQN = Pattern.compile(FQN_S);
static final Pattern IMPORT_P = Pattern
.compile("#import\\s+(?<fqn>" + FQN_S + ")\\s*;?\\s*$");
static final Pattern ANNOTATION_P = Pattern
Expand Down Expand Up @@ -228,6 +229,15 @@ public MetaInfService(String serviceName, String source) {
// just comment
continue;
}
else {

// valid implementationName?
Matcher m = FQN.matcher(line);
if (!m.matches()) {
// invalid fqn: skip
continue;
}
}
implementations.put(line, new Implementation(line, annotations, comments));
annotations.clear();
comments.clear();
Expand Down
14 changes: 12 additions & 2 deletions biz.aQute.bndlib/src/aQute/bnd/osgi/metainf/MetaInfServiceParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@
import java.lang.annotation.RetentionPolicy;
import java.util.Map;

import aQute.bnd.annotation.Resolution;
import aQute.bnd.annotation.spi.ServiceProvider;
import aQute.bnd.header.Attrs;
import aQute.bnd.header.Parameters;
import aQute.bnd.osgi.Analyzer;
import aQute.bnd.osgi.Annotation;
import aQute.bnd.osgi.Annotation.ElementType;
import aQute.bnd.osgi.Constants;
import aQute.bnd.osgi.Descriptors.TypeRef;
import aQute.bnd.osgi.Processor;
import aQute.bnd.osgi.metainf.MetaInfService.Implementation;
Expand All @@ -39,6 +41,11 @@ public boolean analyzeJar(Analyzer analyzer) throws Exception {
return false;
case METAINF_SERVICES_STRATEGY_AUTO : {
analyzer.addClasspathDefault(ServiceProvider.class);
break;
}
case METAINF_SERVICES_STRATEGY_ANNOTATION : {
analyzer.addClasspathDefault(ServiceProvider.class);
break;
}
}

Expand All @@ -52,7 +59,10 @@ public boolean analyzeJar(Analyzer analyzer) throws Exception {
Parameters annotations = impl.getAnnotations();

if (annotations.isEmpty() && METAINF_SERVICES_STRATEGY_AUTO.equals(strategy)) {
annotations.add(ServiceProvider.class.getName(), Attrs.EMPTY_ATTRS);
Attrs attrs = new Attrs();
attrs.put(Constants.RESOLUTION_DIRECTIVE, Resolution.OPTIONAL);
attrs.addDirectiveAliases();
annotations.add(ServiceProvider.class.getName(), attrs);
}

annotations.forEach((annotationName, attrs) -> {
Expand Down Expand Up @@ -80,6 +90,6 @@ private void doAnnotationsforMetaInf(Analyzer analyzer, Implementation impl, Str
}

private String strategy(Analyzer analyzer) {
return analyzer.getProperty(METAINF_SERVICES, METAINF_SERVICES_STRATEGY_ANNOTATION);
return analyzer.getProperty(METAINF_SERVICES, METAINF_SERVICES_STRATEGY_AUTO);
}
}

0 comments on commit cfbef6f

Please sign in to comment.