Derive source file from builder when processing DS annotations

[laeubi]

Currently if one uses the bnd-maven-plugin and has a component that contains an error this is annotated at the pom.xml file as an error what is confusing.

This now checks if an error is created if the source file to the class can be found in the builder and then uses that as the file for reporting the error. Due to bnd operating on the class files we still can't know the exact line number (would require to parse it e.g. with JDT AST) but having the actual file annotated already helps to identify the error faster.

[chrisrueger]

Looks ok to me. A second opinion would be good.
@bjhargrave @timothyjward @pkriens

[timothyjward]

I approve the intent of this, as it is a pain having the error marker on the pom, but I have two questions:

1. Why can’t I see any tests for this? I’m sure that it works but it will be easy to break in future if a new DS version is released.
2. Does setting the location in all cases prevent the bndtools code in Eclipse from identifying the real location for errors? There are currently plenty of cases where bndtools will find the correct method/field to put the error marker based on the error. We don’t want to lose that.

Other than that it looks good.

[laeubi]

Bndtools seem to use the details (what are still provided) as far as I understand, the only difference is that the file field of the Location is set (what is used for example in bnd-maven-plugin)

[timothyjward]

Bndtools seem to use the details (what are still provided) as far as I understand, the only difference is that the file field of the Location is set (what is used for example in bnd-maven-plugin)

This is the code which tries to add the marker:

bnd/bndtools.builder/src/org/bndtools/builder/handlers/component/DSAnnotationErrorHandler.java

Lines 25 to 53 in 1588a4c

	DeclarativeServicesAnnotationError dsError = (DeclarativeServicesAnnotationError) location.details;
	IJavaProject javaProject = JavaCore.create(project);
	Map<String, Object> attribs = new HashMap<>();
	attribs.put(IMarker.MESSAGE, location.message.trim());
	MarkerData md = null;
	if (dsError.className != null) {
	if (dsError.methodName != null && dsError.methodSignature != null) {
	md = createMethodMarkerData(javaProject, dsError.className, dsError.methodName, dsError.methodSignature,
	attribs, false);
	}
	if (dsError.fieldName != null) {
	md = createFieldMarkerData(javaProject, dsError.className, dsError.fieldName, attribs, false);
	}
	if (md == null) {
	md = createTypeMarkerData(javaProject, dsError.className, attribs, false);
	}
	}
	if (md != null) {
	result.add(md);
	} else {
	// No other marker could be created, so add a marker to the bnd file
	result.add(new MarkerData(getDefaultResource(project), attribs, false));
	}

As you can see it uses the "default resource" namely the pom, when it either doesn't have a class name to operate on, or it can't find the named class. Setting the class name in all cases, then fixing this piece of code if it isn't working, seems like the correct solution.

[laeubi]

Setting the class name in all cases, then fixing this piece of code if it isn't working, seems like the correct solution.

I don't understand, this will still not report the correct file name in a maven build.

[timothyjward]

Setting the class name in all cases, then fixing this piece of code if it isn't working, seems like the correct solution.

I don't understand, this will still not report the correct file name in a maven build.

In which case I'm not sure that I understand - is this not an issue relating to Bndtools error markers in Eclipse?

[laeubi]

bnd-maven-plugin uses the BuildContext here

bnd/maven-plugins/bnd-maven-plugin/src/main/java/aQute/bnd/maven/plugin/AbstractBndMavenPlugin.java

Line 170 in 1588a4c

BuildContext buildContext;

BuildContext is a connection between the IDE (e.g. Eclipse, InteliJ, ...) and Maven Plugins to add markers to files and the bnd-maven-plugin forwards build problems (like ds) to that context here:

bnd/maven-plugins/bnd-maven-plugin/src/main/java/aQute/bnd/maven/plugin/AbstractBndMavenPlugin.java

Lines 524 to 571 in 1588a4c

	protected void reportErrorsAndWarnings(Builder builder) throws MojoFailureException {
	@SuppressWarnings("unchecked")
	Collection<File> markedFiles = (Collection<File>) buildContext.getValue(MARKED_FILES);
	if (markedFiles == null) {
	buildContext.removeMessages(propertiesFile);
	markedFiles = builder.getIncluded();
	}
	if (markedFiles != null) {
	for (File f : markedFiles) {
	buildContext.removeMessages(f);
	}
	}
	markedFiles = new HashSet<>();
	List<String> warnings = builder.getWarnings();
	for (String warning : warnings) {
	Location location = builder.getLocation(warning);
	if (location == null) {
	location = new Location();
	location.message = warning;
	}
	File f = location.file == null ? propertiesFile : new File(location.file);
	markedFiles.add(f);
	buildContext.addMessage(f, location.line + 1, location.length, location.message,
	BuildContext.SEVERITY_WARNING,
	null);
	}
	List<String> errors = builder.getErrors();
	for (String error : errors) {
	Location location = builder.getLocation(error);
	if (location == null) {
	location = new Location();
	location.message = error;
	}
	File f = location.file == null ? propertiesFile : new File(location.file);
	markedFiles.add(f);
	buildContext.addMessage(f, location.line + 1, location.length, location.message,
	BuildContext.SEVERITY_ERROR,
	null);
	}
	buildContext.setValue(MARKED_FILES, markedFiles);
	if (!builder.isOk()) {
	if (errors.size() == 1)
	throw new MojoFailureException(errors.get(0));
	else
	throw new MojoFailureException("Errors in bnd processing, see log for details.");
	}
	}

these will then result in error markers in Eclipse if for example m2e is used.

Because previously there was no source file given in the Location it falls back to the "properties" (what is a pom.xml by default in a maven build)

This is how it looks with current bnd-maven-plugin release (no PDE or Bndtools, just plain m2e):

With this change using a local build SNAPSHOT it looks like this:

[timothyjward]

That’s all great, but is there any way to engage the existing marker plugin during the maven build? That already does all the work to decorate the specific method/field/type and it seems silly to duplicate it.

[laeubi]

That’s all great, but is there any way to engage the existing marker plugin during the maven build?

I don't see how this could work as the plugin is working on the workspace model of eclipse (and therefore can provide richer support at the moment).

The next hing is that the place that creates the marker does not knows where an error originates and probably should not know that there is something like DS Plugin at all.

Beside that the decoration is part of m2e already so nothing much has to be "duplicated" beside that one needs correct information on the marker.

[chrisrueger]

In which case I'm not sure that I understand - is this not an issue relating to Bndtools error markers in Eclipse?

No. not bndtools.

Here is my interpretation:

---
title: bndlib, bndtools, m2e, bnd-maven-plugin
---
classDiagram

    bndlib <|-- bndtools
    bndlib <|-- m2e
    bndlib <|-- bnd-maven-plugin
    class bndtools{
        + DSAnnotationErrorHandler
    }

Loading

• bndtools uses bndlib
• bnd-maven-plugin uses bndlib, but not bndtools
• m2e uses bndlib, but not bndtools

--

• @laeubi added something in bndlib (very core, and NOT bndtools)
• he wants to use it in m2e and bnd-maven-plugin
• the DSAnnotationErrorHandler.java you have referenced

  bnd/bndtools.builder/src/org/bndtools/builder/handlers/component/DSAnnotationErrorHandler.java

  Lines 25 to 53 in 1588a4c

  	DeclarativeServicesAnnotationError dsError = (DeclarativeServicesAnnotationError) location.details;
  	IJavaProject javaProject = JavaCore.create(project);
  	Map<String, Object> attribs = new HashMap<>();
  	attribs.put(IMarker.MESSAGE, location.message.trim());
  	MarkerData md = null;
  	if (dsError.className != null) {
  	if (dsError.methodName != null && dsError.methodSignature != null) {
  	md = createMethodMarkerData(javaProject, dsError.className, dsError.methodName, dsError.methodSignature,
  	attribs, false);
  	}
  	if (dsError.fieldName != null) {
  	md = createFieldMarkerData(javaProject, dsError.className, dsError.fieldName, attribs, false);
  	}
  	if (md == null) {
  	md = createTypeMarkerData(javaProject, dsError.className, attribs, false);
  	}
  	}
  	if (md != null) {
  	result.add(md);
  	} else {
  	// No other marker could be created, so add a marker to the bnd file
  	result.add(new MarkerData(getDefaultResource(project), attribs, false));
  	}

  is bndtools , but is not used by m2e and not by bnd-maven-plugin

So:

• the additional file information added in the details in bndlib could be used by all 3 (bndtools, bnd-maven-plugin, m2e),
• but fancy stuff in bndtools (DSAnnotationErrorHandler) cannot be used by bnd-maven-plugin, m2e

Correct me if I am wrong.

[laeubi]

I think this is mostly correct, basically in bndlib there is the Builder and there is a DSPlugin generating the xml.

The DSPlugin generates some errors that have a Location with a DeclarativeServicesAnnotationError attached to them, in the bndtools this details are currently used to create error markers based on JDT and the Eclipse Workspace you run with.

In the maven plugins (or elsewhere when you use the builder) this can not be used as there is no UI and no Eclipse Workspace.

In m2e (that provides a BuildContext) we also have an Eclipse Workspace but currently the BuildContext does not support supply all the details, and even if DSAnnotationErrorHandler is baked into bndtools and can not be reused. It would be interesting to check if one can convert it to an OSGi Reusable Components for bndlib so it can be used in m2e directly but then still BuildContext needs to be enhanced.

So in the meanwhile this at least forwards the filename to other users of bnd outside the bndtools build.

[chrisrueger]

so in the meanwhile this at least forwards the filename to other users of bnd outside the bndtools build.

So to me this sounds like an improvement of user/developer experience, by adding a small information which wasn't there before (the filename).

The expectation of @timothyjward to have a more centralized approach similar to the bndtools DSAnnotationErrorHandler.java but more deep down in bndlib is great, but maybe out of scope for this PR.

It would be interesting to check if one can convert it to an OSGi Reusable Components for bndlib so it can be used in m2e directly but then still BuildContext needs to be enhanced.

And maybe @laeubi already has an idea for another PR :)

1. Why can’t I see any tests for this? I’m sure that it works but it will be easy to break in future if a new DS version is released.

@laeubi maybe have a look at test.ClazzTest.testRecursiveAnnotation() to build on. It uses the DSAnnotationReader. Just a thought