Merge pull request #377 from open-edge-platform/fix/deduplicate-dot-edges

fix: Improvements to DOT dependency graph generation

Author: arodage

docs/architecture/adr-dep-analyzer.md

@@ -94,10 +94,6 @@ is already required for DOT rendering).
    - Multiple output formats supported: DOT, SVG, PNG, PDF
    - Auto-generated filenames reflect analysis parameters
 
-4. **Color Preservation**  
-   - Maintain semantic colors from os-image-composer output
-   - Sliced subgraphs retain visual context
-
 ---
 
 ## Command Line Interface
@@ -170,22 +166,6 @@ Examples:
 
 ---
 
-## Semantic Color Preservation
-
-The os-image-composer assigns colors to indicate package categories:
-
-| Color | Hex Code | Package Category |
-|-------|----------|------------------|
-| Yellow | `#fff4d6` | Essential packages |
-| Green | `#d4efdf` | System packages (user-specified) |
-| Blue | `#d6eaf8` | Kernel packages |
-| Orange | `#fdebd0` | Bootloader packages |
-
-The dep-analyzer preserves these colors in sliced subgraphs, maintaining
-visual context and category information.
-
----
-
 ## Analysis Modes
 
 ### Slicing Mode

docs/architecture/os-image-composer-cli-specification.md

@@ -144,7 +144,7 @@ os-image-composer build [flags] TEMPLATE_FILE
 | `--cache-dir, -d DIR` | Package cache directory (overrides config). Proper caching significantly improves build times. |
 | `--work-dir DIR` | Working directory for builds (overrides config). This directory is where images are constructed before being finalized. |
 | `--verbose, -v` | Enable verbose output (equivalent to --log-level debug). Displays detailed information about each step of the build process. |
-| `--dotfile, -f FILE` | Generate a dot file for the merged template dependency graph (user + defaults with resolved packages). Nodes are color-coded: essentials (pale yellow), template packages (green), kernel (blue), bootloader (orange). |
+| `--dotfile, -f FILE` | Generate a dot file for the merged template dependency graph (user + defaults with resolved packages). |
 | `--system-packages-only` | When paired with `--dotfile`, limit the dependency graph to roots defined in `SystemConfig.Packages`. Dependencies pulled in by those roots still appear, but essentials/kernel/bootloader packages aren't drawn unless required by a system package. |
 
 **Example:**

docs/index.md

@@ -220,7 +220,7 @@ Flags:
 - `--cache-dir, -d`: Package cache directory (overrides the configuration file)
 - `--work-dir`: Working directory for builds (overrides the configuration file)
 - `--verbose, -v`: Enable verbose output
-- `--dotfile, -f`: Generate a dependency graph for the merged template as a dot file (color legend: essential = pale yellow, user packages = green, kernel = blue, bootloader = orange)
+- `--dotfile, -f`: Generate a dependency graph for the merged template as a dot file
 - `--system-packages-only`: Use together with `--dotfile` to keep only `SystemConfig.Packages` roots in the graph (dependencies still appear if required)
 - `--config`: Path to the configuration file
 - `--log-level`: Log level (debug, info, warn, and error)

internal/config/config.go

@@ -346,10 +346,10 @@ func (t *ImageTemplate) GetPackages() []string {
 
 var packageSourcePriority = map[PackageSource]int{
 	PackageSourceUnknown:    0,
-	PackageSourceEssential:  10,
+	PackageSourceSystem:     10,
 	PackageSourceKernel:     20,
 	PackageSourceBootloader: 20,
-	PackageSourceSystem:     30,
+	PackageSourceEssential:  30,
 }
 
 // GetPackageSourceMap returns a map of package name to the template section that requested it.
@@ -367,10 +367,10 @@ func (t *ImageTemplate) GetPackageSourceMap() map[string]PackageSource {
 		}
 	}
 
-	setSources(t.EssentialPkgList, PackageSourceEssential)
+	setSources(t.SystemConfig.Packages, PackageSourceSystem)
 	setSources(t.KernelPkgList, PackageSourceKernel)
 	setSources(t.BootloaderPkgList, PackageSourceBootloader)
-	setSources(t.SystemConfig.Packages, PackageSourceSystem)
+	setSources(t.EssentialPkgList, PackageSourceEssential)
 
 	return sources
 }

internal/config/config_test.go

@@ -3307,8 +3307,8 @@ func TestGetPackageSourceMap(t *testing.T) {
 	if got := sources["vim"]; got != PackageSourceSystem {
 		t.Fatalf("vim source = %s, want system", got)
 	}
-	if got := sources["bash"]; got != PackageSourceSystem {
-		t.Fatalf("bash source = %s, want system override", got)
+	if got := sources["bash"]; got != PackageSourceEssential {
+		t.Fatalf("bash source = %s, want essential (essential has higher priority than system)", got)
 	}
 	if _, exists := sources[""]; exists {
 		t.Fatalf("unexpected empty key in package source map")

internal/ospackage/debutils/resolver.go

@@ -24,26 +24,6 @@ type VersionConstraint struct {
 	Alternative string // Alternative package name for constraints like "logsave | e2fsprogs (<< 1.45.3-1~)"
 }
 
-type dotStyle struct {
-	fillColor   string
-	borderColor string
-	legendLabel string
-}
-
-var packageSourceStyles = map[config.PackageSource]dotStyle{
-	config.PackageSourceEssential:  {fillColor: "#fff4d6", borderColor: "#f5c518", legendLabel: "EssentialPkgList"},
-	config.PackageSourceSystem:     {fillColor: "#d4efdf", borderColor: "#27ae60", legendLabel: "SystemConfig.Packages"},
-	config.PackageSourceKernel:     {fillColor: "#d6eaf8", borderColor: "#1f618d", legendLabel: "Kernel"},
-	config.PackageSourceBootloader: {fillColor: "#fdebd0", borderColor: "#d35400", legendLabel: "Bootloader"},
-}
-
-var legendOrder = []config.PackageSource{
-	config.PackageSourceEssential,
-	config.PackageSourceSystem,
-	config.PackageSourceKernel,
-	config.PackageSourceBootloader,
-}
-
 func GenerateDot(pkgs []ospackage.PackageInfo, file string, pkgSources map[string]config.PackageSource) error {
 	log := logger.Logger()
 	log.Infof("Generating DOT file %s", file)
@@ -63,44 +43,32 @@ func GenerateDot(pkgs []ospackage.PackageInfo, file string, pkgSources map[strin
 	if _, err := fmt.Fprintln(writer, "  rankdir=LR;"); err != nil {
 		return fmt.Errorf("writing DOT attributes: %w", err)
 	}
-	if _, err := fmt.Fprintln(writer, "  node [shape=box, style=filled, fillcolor=\"#ffffff\", color=\"#666666\"];"); err != nil {
+	if _, err := fmt.Fprintln(writer, "  node [shape=box];"); err != nil {
 		return fmt.Errorf("writing DOT node defaults: %w", err)
 	}
 
-	legendUsed := make(map[config.PackageSource]bool)
+	edgesWritten := make(map[string]bool)
 
 	for _, pkg := range pkgs {
 		if pkg.Name == "" {
 			continue
 		}
-		source := config.PackageSourceUnknown
-		if pkgSources != nil {
-			if val, ok := pkgSources[pkg.Name]; ok {
-				source = val
-			}
-		}
-		attr := fmt.Sprintf("label=\"%s\"", pkg.Name)
-		if style, ok := packageSourceStyles[source]; ok {
-			legendUsed[source] = true
-			attr += fmt.Sprintf(", fillcolor=\"%s\", color=\"%s\"", style.fillColor, style.borderColor)
-		}
-		if _, err := fmt.Fprintf(writer, "  \"%s\" [%s];\n", pkg.Name, attr); err != nil {
+		if _, err := fmt.Fprintf(writer, "  \"%s\";\n", pkg.Name); err != nil {
 			return fmt.Errorf("writing DOT node for %s: %w", pkg.Name, err)
 		}
 		for _, dep := range pkg.Requires {
 			depName := CleanDependencyName(dep)
 			if depName == "" {
 				continue
 			}
+			edgeKey := pkg.Name + "|" + depName
+			if edgesWritten[edgeKey] {
+				continue
+			}
 			if _, err := fmt.Fprintf(writer, "  \"%s\" -> \"%s\";\n", pkg.Name, depName); err != nil {
 				return fmt.Errorf("writing DOT edge %s->%s: %w", pkg.Name, depName, err)
 			}
-		}
-	}
-
-	if len(legendUsed) > 0 {
-		if err := writeLegend(writer, legendUsed); err != nil {
-			return err
+			edgesWritten[edgeKey] = true
 		}
 	}
 
@@ -111,47 +79,6 @@ func GenerateDot(pkgs []ospackage.PackageInfo, file string, pkgSources map[strin
 	return nil
 }
 
-func writeLegend(writer *bufio.Writer, legendUsed map[config.PackageSource]bool) error {
-	if _, err := fmt.Fprintln(writer, "  subgraph cluster_legend {"); err != nil {
-		return fmt.Errorf("writing legend header: %w", err)
-	}
-	if _, err := fmt.Fprintln(writer, "    label=\"Legend\";"); err != nil {
-		return fmt.Errorf("writing legend label: %w", err)
-	}
-	if _, err := fmt.Fprintln(writer, "    style=\"dashed\";"); err != nil {
-		return fmt.Errorf("writing legend style: %w", err)
-	}
-	if _, err := fmt.Fprintln(writer, "    color=\"#bbbbbb\";"); err != nil {
-		return fmt.Errorf("writing legend color: %w", err)
-	}
-
-	var previous string
-	for _, source := range legendOrder {
-		if !legendUsed[source] {
-			continue
-		}
-		style, ok := packageSourceStyles[source]
-		if !ok {
-			continue
-		}
-		nodeName := fmt.Sprintf("legend_%s", source)
-		if _, err := fmt.Fprintf(writer, "    %s [label=\"%s\", style=\"filled\", fillcolor=\"%s\", color=\"%s\"];\n", nodeName, style.legendLabel, style.fillColor, style.borderColor); err != nil {
-			return fmt.Errorf("writing legend node for %s: %w", source, err)
-		}
-		if previous != "" {
-			if _, err := fmt.Fprintf(writer, "    %s -> %s [style=invis];\n", previous, nodeName); err != nil {
-				return fmt.Errorf("writing legend spacing edge: %w", err)
-			}
-		}
-		previous = nodeName
-	}
-
-	if _, err := fmt.Fprintln(writer, "  }"); err != nil {
-		return fmt.Errorf("writing legend footer: %w", err)
-	}
-	return nil
-}
-
 // ParseRepositoryMetadata parses the Packages.gz file from gzHref.
 func ParseRepositoryMetadata(baseURL string, pkggz string, releaseFile string, releaseSign string, pbGPGKey string, buildPath string, arch string) ([]ospackage.PackageInfo, error) {
 	log := logger.Logger()

internal/ospackage/debutils/resolver_test.go

@@ -139,6 +139,16 @@ func TestGenerateDot(t *testing.T) {
 			filename:    filepath.Join(tmpDir, "complex-deps.dot"),
 			expectError: false,
 		},
+		{
+			name: "duplicate dependencies should be deduplicated",
+			pkgs: []ospackage.PackageInfo{
+				{Name: "libstdc++", Version: "1.0", Requires: []string{"libc6", "libc6", "libc6", "libgcc", "libgcc"}},
+				{Name: "libc6", Version: "1.0"},
+				{Name: "libgcc", Version: "1.0", Requires: []string{"libc6"}},
+			},
+			filename:    filepath.Join(tmpDir, "dedup-deps.dot"),
+			expectError: false,
+		},
 		{
 			name: "invalid path",
 			pkgs: []ospackage.PackageInfo{
@@ -169,18 +179,6 @@ func TestGenerateDot(t *testing.T) {
 			}
 			contentStr := string(content)
 
-			if tc.pkgSources != nil {
-				if !strings.Contains(contentStr, "legend_system") {
-					t.Errorf("legend for system packages not found in DOT output")
-				}
-				if !strings.Contains(contentStr, "\"sys\" [label=\"sys\", fillcolor=\"#d4efdf\", color=\"#27ae60\"];") {
-					t.Errorf("expected system package styling for sys node")
-				}
-				if !strings.Contains(contentStr, "\"ess\" [label=\"ess\", fillcolor=\"#fff4d6\", color=\"#f5c518\"];") {
-					t.Errorf("expected essential package styling for ess node")
-				}
-			}
-
 			if !strings.Contains(contentStr, "digraph G {") {
 				t.Error("DOT file should start with 'digraph G {'")
 			}
@@ -195,10 +193,13 @@ func TestGenerateDot(t *testing.T) {
 				if pkg.Name == "" {
 					continue
 				}
-				nodePrefix := fmt.Sprintf("\"%s\" [label=\"%s\"", pkg.Name, pkg.Name)
-				if !strings.Contains(contentStr, nodePrefix) {
+				nodeDef := fmt.Sprintf("\"%s\";", pkg.Name)
+				if !strings.Contains(contentStr, nodeDef) {
 					t.Errorf("DOT file should contain node for %s", pkg.Name)
 				}
+
+				// Check dependencies - each unique edge should appear exactly once
+				seenEdges := make(map[string]bool)
 				for _, dep := range pkg.Requires {
 					depName := debutils.CleanDependencyName(dep)
 					if depName == "" {
@@ -208,6 +209,17 @@ func TestGenerateDot(t *testing.T) {
 					if !strings.Contains(contentStr, edge) {
 						t.Errorf("DOT file should contain edge: %s", edge)
 					}
+					seenEdges[edge] = true
+				}
+
+				// For duplicate dependency test, verify each unique edge appears only once
+				if tc.name == "duplicate dependencies should be deduplicated" {
+					for edge := range seenEdges {
+						count := strings.Count(contentStr, edge)
+						if count != 1 {
+							t.Errorf("Edge %s should appear exactly once, but appears %d times", edge, count)
+						}
+					}
 				}
 			}
 		})