Sb bitbake multi layer recipes #516

stevebillings · 2022-01-25T21:38:34Z

A solution for IDETECT-2925. The problem seems to have been caused by deriving the layer value for each dependency by picking the first layer listed the show-recipes command output (even when multiple layers are listed). Based on this customer's log, it looks more reliable to use the dependency layer from task-depends.dot dependency details.

The slight challenge to getting the layer from task-depends.dot dependency information is that the layer name is part of the path to the recipe. With this change, we first determine the list of layers, then for each dependency, for each layer: do a path.contains("/layername/").

A side effect of this change: the detector now only executes the show-recipes command once (early), rather than once per target image. Among other things, show-recipes provides the list of layers in the target image.

This PR does not address the audit feedback... that is next.

…ely needed (avoiding a conversion to it later)

…d/wrong task-depends.dot file

# Conflicts: # detectable/src/main/java/com/synopsys/integration/detectable/detectables/bitbake/BitbakeExtractor.java # detectable/src/main/java/com/synopsys/integration/detectable/detectables/bitbake/parse/BitbakeGraphTransformer.java # detectable/src/main/java/com/synopsys/integration/detectable/factory/DetectableFactory.java

# Conflicts: # detectable/src/main/java/com/synopsys/integration/detectable/detectables/bitbake/BitbakeExtractor.java

stevebillings · 2022-01-25T22:49:58Z

...a/com/synopsys/integration/detectable/detectables/bitbake/parse/BitbakeGraphTransformer.java

+        }
+        return dependencyLayer;
+    }
 }


I'm half tempted to remove this fallback-to-first-in-list. I'm hoping it's never used. If we did remove it, we could execute bitbake-layers show-layers instead of bitbake-layers show-recipes, and do a whole lot less parsing. I'm hoping that after reading a few customer logs it'll become more clear that we don't need the fallback.

I'm perfectly fine with defaulting to the easier approach and waiting for real customer examples to come through via support. For example, assuming something doesn't happen, but throw exception if we detect it and wait for a diagnostic zip with real world example.

@JakeMathews I'd love your opinion on this. The bitbake code does not assume it'll find the recipe version in the task-depends.dot label (GraphParseTransformer.getVersionFromNode() has returned an Optional even before I started mucking with things). Given the vast experience you have now, do you believe that caution makes sense? And should it apply to finding the layer there as well? (I realize this is largely guesswork, but I trust your guesses more than mine.)

Looking more closely: the detector does not assume that the label attribute is present, but if the label attribute is present, it assumes it will contain the version. My current plan is to do the same for layer.

I think if the label is present its pretty safe to assume the layer is in the path. I always prefer to air on the side of caution especially when understanding is murky at best.
It would be great if we could phone home suspected discrepancies rather than failing or silently use a default.

i think what you have now will likely be the safest approach, but we won’t know if its right until we can collect more data.

style(doc): bitbake fix doc and release note

taikuukaits · 2022-01-27T20:23:09Z

.../src/main/java/com/synopsys/integration/detectable/detectables/bitbake/BitbakeExtractor.java

+        try {
+            showRecipesResults = bitbakeSession.executeBitbakeForRecipeLayerCatalog();
+        } catch (IOException | ExecutableFailedException e) {
+            String msg = String.format("Error collecting recipe layer information from show-recipes command: %s", e.getMessage());


You could just bubble these up - if this message helpful for debugging though, looks good.

taikuukaits · 2022-01-27T20:23:49Z

.../src/main/java/com/synopsys/integration/detectable/detectables/bitbake/BitbakeExtractor.java

+                codeLocations.add(generateCodeLocationForTargetImage(followSymLinks, searchDepth, dependencyTypeFilter, bitbakeSession, buildDir, bitbakeEnvironment, showRecipesResults, targetImage));
            } catch (IOException | IntegrationException | NotImplementedException | ExecutableFailedException e) {
-                logger.error(String.format("Failed to extract a Code Location while running Bitbake against package '%s': %s", packageName, e.getMessage()));
+                logger.error(String.format("Failed to extract a Code Location while running Bitbake against package '%s': %s", targetImage, e.getMessage()));


Same thing, you could just bubble these up.

Here we want to just log and continue on to the next target image / codelocation

taikuukaits · 2022-01-27T20:28:52Z

...a/com/synopsys/integration/detectable/detectables/bitbake/parse/BitbakeGraphTransformer.java

+        }
+        return dependencyLayer;
+    }
 }


I'm perfectly fine with defaulting to the easier approach and waiting for real customer examples to come through via support. For example, assuming something doesn't happen, but throw exception if we detect it and wait for a diagnostic zip with real world example.

taikuukaits · 2022-01-27T20:32:54Z

...va/com/synopsys/integration/detectable/detectables/bitbake/parse/GraphParserTransformer.java

+
+    @NotNull
+    private String[] getLabelParts(final String label) {
+        return label.split("\\\\n:|\\\\n");


This seems inappropriate in a transformer. What you could do is split out this, make a LabelParser.parseLayerLabel that returns LayerDetails(path) with Optional getPath() . This would let you get away from handling the String[] in other methods and let you unit test label parsing. I'd suggets both getLayerFromLabel and getLabelParts become LabelParser.parseLayer -> LayerDetails.

taikuukaits · 2022-01-27T20:34:58Z

...com/synopsys/integration/detectable/detectables/bitbake/unit/GraphParserTransformerTest.java

-        BitbakeGraph bitbakeGraph = buildGraph(nodes, edges);
+        addNode("name", "name\\n:version\\n/some/meta/path/to.bb", nodes, edges);
+        Set<String> knownLayers = new HashSet<>(Arrays.asList("aaa", "meta", "bbb"));
+        BitbakeGraph bitbakeGraph = buildGraph(nodes, edges, knownLayers);


Seems a bit odd BitBakeGraph needs to be tested? It looks like a model object. Perhaps the logic this is testing is better suited in a transformer or a parser. (Not necessarily related to your PR I know, but I wanted to mention it).

I think the main objective of this test is to test is to test the transformer, but BitbakeGraph is a mix of code and data, and I think you might be pointing out that that should be separated. I'm adding a note to the bitbake audit ticket to address that.

JakeMathews · 2022-01-28T15:36:03Z

...java/com/synopsys/integration/detectable/detectables/bitbake/parse/BitbakeRecipesParser.java

            // Parse beginning of new component
            if (currentRecipe != null) {
-                bitbakeRecipes.add(currentRecipe);
+                bitbakeRecipes.put(currentRecipe.getName(), currentRecipe.getLayerNames());


Why do we not want to add the currentRecipe to the bitbakeRecipes map in the other else-if statements in this method?

I don't know... all I did in that method was change that list to a map (so I could delete BitbakeRecipesToLayerMapConverter). It's been that way for a while and I was unaware of a reason to question it.

# Conflicts: # docs/markdown/releasenotes.md

stevebillings added 19 commits January 18, 2022 11:29

refactor: preserve the full recipe layer list a bit longer

51e054b

Merge branch 'master' into sb_bitbakeMultiLayerRecipes

a0878c9

feat: collect recipe layers only once

4eba072

refactor: ShowRecipesResult object

bfeb7ef

feat: parse dependency recipe layer from task-depends.dot

05ab2f7

Merge branch 'master' into sb_bitbakeMultiLayerRecipes

55b9e90

feat: prefer dependency recipe layer parsed from task-depends.dot

ab53844

style: added log msg

e100f81

refactor: cleanup

351d58c

refactor: when parsing show-layers, directly generate the map ultimat…

c490705

…ely needed (avoiding a conversion to it later)

refactor: cleanup

e732b9e

refactor: cleanup

b458c44

style: added a todo based on QA's experience with 7.9.0 reading an ol…

2cef78d

…d/wrong task-depends.dot file

Merge branch 'master' into sb_bitbakeMultiLayerRecipes

b3f8cf6

refactor: cleaning up the extractor a little bit

c94d1a0

refactor: cleanup

073d430

test: test layer parsing

aae22f0

Merge branch 'master' into sb_bitbakeMultiLayerRecipes

bc30893

# Conflicts: # detectable/src/main/java/com/synopsys/integration/detectable/detectables/bitbake/BitbakeExtractor.java

stevebillings requested a review from JakeMathews January 25, 2022 21:38

refactor: minor simplification

d9eb3fa

stevebillings commented Jan 25, 2022

View reviewed changes

style(doc): bitbake fix doc and release note

e6fd2e5

stevebillings requested a review from taikuukaits January 26, 2022 03:15

stevebillings added 3 commits January 27, 2022 14:47

Merge branch 'master' into sb_bitbakeMultiLayerRecipes

5ef067e

Merge branch 'sb_bitbakeMultiLayerRecipes' into docForIDETECT-2925

44eae26

Merge pull request #517 from blackducksoftware/docForIDETECT-2925

ecfb60c

style(doc): bitbake fix doc and release note

taikuukaits approved these changes Jan 27, 2022

View reviewed changes

chore: extractor throw, rather than catch, IO/Execute exceptions

3f5c33e

JakeMathews approved these changes Jan 28, 2022

View reviewed changes

stevebillings added 2 commits January 28, 2022 11:56

refactor: added GraphNodeLabelParser and GraphNodeLabelDetails

94a213e

Merge branch 'master' into sb_bitbakeMultiLayerRecipes

7311737

# Conflicts: # docs/markdown/releasenotes.md

stevebillings merged commit 7c6653e into master Jan 28, 2022

dmamidibd deleted the sb_bitbakeMultiLayerRecipes branch June 7, 2023 19:45

Sb bitbake multi layer recipes #516

Sb bitbake multi layer recipes #516

Uh oh!

Conversation

stevebillings commented Jan 25, 2022

Uh oh!

stevebillings Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevebillings Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevebillings Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stevebillings Jan 25, 2022 •

edited

Loading

stevebillings Jan 28, 2022 •

edited

Loading

stevebillings Jan 28, 2022 •

edited

Loading