paulrutter commented on code in PR #565:
URL: https://github.com/apache/felix-dev/pull/565#discussion_r4347816669
##########
framework/src/main/java/org/apache/felix/framework/StatefulResolver.java:
##########
@@ -1430,6 +1436,42 @@ private synchronized boolean
isSelectedSingleton(BundleRevision br)
return m_selectedSingletons.contains(br);
}
+ private synchronized BundleRevision
findConflictingSingleton(BundleRevision br)
+ {
+ List<BundleRevision> singletons =
m_singletons.get(br.getSymbolicName());
+ if (singletons == null)
+ {
+ return null;
+ }
+
+ for (BundleRevision singleton : singletons)
+ {
+ if ((singleton != br)
+ && ((singleton.getWiring() != null) ||
m_selectedSingletons.contains(singleton)))
+ {
+ return singleton;
+ }
+ }
+
+ return null;
+ }
+
+ private static String formatConflictingBundle(BundleRevision
conflictingRevision)
+ {
+ if (conflictingRevision == null)
+ {
+ return "?";
+ }
+
+ Bundle conflictingBundle = conflictingRevision.getBundle();
+ String details = "[id=" + conflictingBundle.getBundleId() + "]";
+ if (conflictingBundle.getState() == Bundle.UNINSTALLED)
+ {
+ details += ". This bundle is already uninstalled. Call
refreshPackages() to remove it from the framework.";
Review Comment:
For an uninstalled conflicting bundle this string already ends with a
period, and the caller appends another one, so the message ends in "..". Also,
`refreshPackages()` is the deprecated `PackageAdmin` API;
`FrameworkWiring.refreshBundles()` is the current one.
##########
framework/src/main/java/org/apache/felix/framework/StatefulResolver.java:
##########
@@ -463,7 +463,13 @@ void resolve(
}
else if (Util.isSingleton(br) && !isSelectedSingleton(br))
{
- throw new ResolveException("Singleton conflict.", br,
null);
+ BundleRevision conflictingRevision =
findConflictingSingleton(br);
+ throw new ResolveException(
+ "Singleton conflict for Bundle-SymbolicName " +
br.getSymbolicName()
Review Comment:
The message always says "Another bundle is already providing this singleton
BSN", but `findConflictingSingleton` can return null, e.g. when `br` was not
selected because a resolver hook whitelist filtered it out, or it lost to a
higher version in the same resolve. The text then reads "...singleton BSN: ?.",
which is wrong and misleading. Consider different wording for the null case.
##########
framework/src/main/java/org/apache/felix/framework/StatefulResolver.java:
##########
@@ -1430,6 +1436,42 @@ private synchronized boolean
isSelectedSingleton(BundleRevision br)
return m_selectedSingletons.contains(br);
}
+ private synchronized BundleRevision
findConflictingSingleton(BundleRevision br)
Review Comment:
No tests cover the new message or helpers. The null, uninstalled and normal
conflict branches are all unexercised, so a later change to the message format
or to singleton selection could break them silently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
