Github user geomacy commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/746#discussion_r125920216
  
    --- Diff: 
core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java
 ---
    @@ -511,30 +597,76 @@ private void collectCatalogItems(String sourceYaml, 
Map<?,?> itemMetadata, List<
             // brooklyn.libraries we treat specially, to append the list, with 
the child's list preferred in classloading order
             // `libraries` is supported in some places as a legacy syntax; it 
should always be `brooklyn.libraries` for new apps
             // TODO in 0.8.0 require brooklyn.libraries, don't allow 
"libraries" on its own
    -        List<?> librariesNew = 
MutableList.copyOf(getFirstAs(itemMetadataWithoutItemDef, List.class, 
"brooklyn.libraries", "libraries").orNull());
    -        Collection<CatalogBundle> libraryBundlesNew = 
CatalogItemDtoAbstract.parseLibraries(librariesNew);
    -
    -        List<?> librariesCombined = MutableList.copyOf(librariesNew)
    -            .appendAll(getFirstAs(parentMetadata, List.class, 
"brooklyn.libraries", "libraries").orNull());
    -        if (!librariesCombined.isEmpty())
    -            catalogMetadata.put("brooklyn.libraries", librariesCombined);
    -        Collection<CatalogBundle> libraryBundles = 
CatalogItemDtoAbstract.parseLibraries(librariesCombined);
    +        List<?> librariesAddedHereNames = 
MutableList.copyOf(getFirstAs(itemMetadataWithoutItemDef, List.class, 
"brooklyn.libraries", "libraries").orNull());
    +        Collection<CatalogBundle> librariesAddedHereBundles = 
CatalogItemDtoAbstract.parseLibraries(librariesAddedHereNames);
    +        
    +        MutableSet<Object> librariesCombinedNames = MutableSet.of();
    +        if (!isNoBundleOrSimpleWrappingBundle(mgmt, containingBundle)) {
    +            // ensure containing bundle is declared, first, for search 
purposes
    +            
librariesCombinedNames.add(containingBundle.getVersionedName().toOsgiString());
    +        }
    +        librariesCombinedNames.putAll(librariesAddedHereNames);
    +        librariesCombinedNames.putAll(getFirstAs(parentMetadata, 
Collection.class, "brooklyn.libraries", "libraries").orNull());
    +        if (!librariesCombinedNames.isEmpty()) {
    +            catalogMetadata.put("brooklyn.libraries", 
librariesCombinedNames);
    +        }
    +        Collection<CatalogBundle> libraryBundles = 
CatalogItemDtoAbstract.parseLibraries(librariesCombinedNames);
     
    -        // TODO as this may take a while if downloading, the REST call 
should be async
    -        // (this load is required for the scan below and I think also for 
yaml resolution)
    -        CatalogUtils.installLibraries(mgmt, libraryBundlesNew);
    +        // TODO this may take a while if downloading; ideally the REST 
call would be async
    +        // but this load is required for resolving YAML in this BOM (and 
if java-scanning);
    +        // need to think through how we expect dependencies to be installed
    +        CatalogUtils.installLibraries(mgmt, librariesAddedHereBundles);
    +        
    +        // use resolved bundles
    +        librariesAddedHereBundles = resolveWherePossible(mgmt, 
librariesAddedHereBundles);
    +        libraryBundles = resolveWherePossible(mgmt, libraryBundles);
     
             Boolean scanJavaAnnotations = 
getFirstAs(itemMetadataWithoutItemDef, Boolean.class, "scanJavaAnnotations", 
"scan_java_annotations").orNull();
             if (scanJavaAnnotations==null || !scanJavaAnnotations) {
                 // don't scan
             } else {
    -            // scan for annotations: if libraries here, scan them; if 
inherited libraries error; else scan classpath
    -            if (!libraryBundlesNew.isEmpty()) {
    -                result.addAll(scanAnnotationsFromBundles(mgmt, 
libraryBundlesNew, catalogMetadata));
    -            } else if (libraryBundles.isEmpty()) {
    -                result.addAll(scanAnnotationsFromLocal(mgmt, 
catalogMetadata));
    +            if (isNoBundleOrSimpleWrappingBundle(mgmt, containingBundle)) {
    +                Collection<CatalogItemDtoAbstract<?, ?>> scanResult;
    +                // BOMs wrapped in JARs, or without JARs, have special 
treatment
    +                if 
(isLibrariesMoreThanJustContainingBundle(librariesAddedHereBundles, 
containingBundle)) {
    +                    // legacy mode, since 0.12.0, scan libraries 
referenced in a legacy non-bundle BOM
    +                    log.warn("Deprecated use of scanJavaAnnotations to 
scan other libraries ("+librariesAddedHereBundles+"); libraries should declare 
they scan themselves");
    +                    scanResult = 
scanAnnotationsLegacyInListOfLibraries(mgmt, librariesAddedHereBundles, 
catalogMetadata, containingBundle);
    +                } else if 
(!isLibrariesMoreThanJustContainingBundle(libraryBundles, containingBundle)) {
    +                    // for default catalog, no libraries declared, we want 
to scan local classpath
    +                    // bundle should be named "brooklyn-default-catalog"
    +                    if (containingBundle!=null && 
!containingBundle.getSymbolicName().contains("brooklyn-default-catalog")) {
    +                        // a user uplaoded a BOM trying to tell us to do a 
local java scan; previously supported but becoming unsupported
    +                        log.warn("Deprecated use of scanJavaAnnotations in 
non-Java BOM outwith the default catalog setup"); 
    +                    } else if (depth>0) {
    +                        // since 0.12.0, require this to be right next to 
where libraries are defined, or at root
    +                        log.warn("Deprecated use of scanJavaAnnotations 
declared in item; should be declared at the top level of the BOM");
    +                    }
    +                    scanResult = 
scanAnnotationsFromLocalNonBundleClasspath(mgmt, catalogMetadata, 
containingBundle);
    +                } else {
    +                    throw new IllegalStateException("Cannot scan for Java 
catalog items when libraries declared on an ancestor; scanJavaAnnotations 
should be specified alongside brooklyn.libraries (or ideally those libraries 
should specify to scan)");
    +                }
    +                if (scanResult!=null && !scanResult.isEmpty()) {
    +                    if (result!=null) {
    +                        result.addAll( scanResult );
    +                    } else {
    +                        // not returning a result; we need to add here
    +                        for (CatalogItem item: scanResult) {
    +                            mgmt.getCatalog().addItem(item);
    +                        }
    +                    }
    +                }
                 } else {
    -                throw new IllegalStateException("Cannot scan catalog node 
no local bundles, and with inherited bundles we will not scan the classpath");
    +                throw new IllegalArgumentException("Scanning for Java 
annotations is not supported in BOMs in bundles; "
    +                    + "entries should be listed explicitly in the 
catalog.bom");
    +                // see comments on scanAnnotationsInBundle
    +//                if (depth>0) {
    --- End diff --
    
    Best remove this commented code


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---

Reply via email to