fix: report in case of cyclic dependency

pkg/client/client.go

@@ -17,7 +17,6 @@ import (
 	"kcl-lang.io/kpm/pkg/env"
 	"kcl-lang.io/kpm/pkg/errors"
 	"kcl-lang.io/kpm/pkg/git"
-	pkgGraph "kcl-lang.io/kpm/pkg/graph"
 	"kcl-lang.io/kpm/pkg/oci"
 	"kcl-lang.io/kpm/pkg/opt"
 	pkg "kcl-lang.io/kpm/pkg/package"
@@ -596,7 +595,7 @@ func (c *KpmClient) AddDepToPkg(kclPkg *pkg.KclPkg, d *pkg.Dependency) error {
 	}
 
 	// download all the dependencies.
-	changedDeps, _, err := c.downloadDeps(kclPkg.ModFile.Dependencies, kclPkg.Dependencies)
+	changedDeps, _, err := c.InitGraphAndDownloadDeps(kclPkg)
 
 	if err != nil {
 		return err
@@ -1097,34 +1096,24 @@ func (c *KpmClient) ParseOciOptionFromString(oci string, tag string) (*opt.OciOp
 	return ociOpt, nil
 }
 
-// GetDependencyGraph will get the dependency graph of kcl package dependencies
-func (c *KpmClient) GetDependencyGraph(kclPkg *pkg.KclPkg) (graph.Graph[string, string], error) {
-	_, depGraph, err := c.downloadDeps(kclPkg.ModFile.Dependencies, kclPkg.Dependencies)
-	if err != nil {
-		return nil, err
-	}
+// InitGraphAndDownloadDeps initializes a dependency graph and call downloadDeps function.
+func (c *KpmClient) InitGraphAndDownloadDeps(kclPkg *pkg.KclPkg) (*pkg.Dependencies, graph.Graph[string, string], error) {
 
-	sources, err := pkgGraph.FindSources(depGraph)
-	if err != nil {
-		return nil, err
-	}
+	depGraph := graph.New(graph.StringHash, graph.Directed(), graph.PreventCycles())
 
 	// add the root vertex(package name) to the dependency graph.
 	root := fmt.Sprintf("%s@%s", kclPkg.GetPkgName(), kclPkg.GetPkgVersion())
-	err = depGraph.AddVertex(root)
+	err := depGraph.AddVertex(root)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	// make an edge between the root vertex and all the sources of the dependency graph.
-	for _, source := range sources {
-		err = depGraph.AddEdge(root, source)
-		if err != nil {
-			return nil, err
-		}
+	changedDeps, err := c.downloadDeps(kclPkg.ModFile.Dependencies, kclPkg.Dependencies, depGraph, root)
+	if err != nil {
+		return nil, nil, err
 	}
 
-	return depGraph, nil
+	return changedDeps, depGraph, nil
 }
 
 // dependencyExists will check whether the dependency exists in the local filesystem.
@@ -1151,15 +1140,15 @@ func (c *KpmClient) dependencyExists(dep *pkg.Dependency, lockDeps *pkg.Dependen
 }
 
 // downloadDeps will download all the dependencies of the current kcl package.
-func (c *KpmClient) downloadDeps(deps pkg.Dependencies, lockDeps pkg.Dependencies) (*pkg.Dependencies, graph.Graph[string, string], error) {
+func (c *KpmClient) downloadDeps(deps pkg.Dependencies, lockDeps pkg.Dependencies, depGraph graph.Graph[string, string], parent string) (*pkg.Dependencies, error) {
 	newDeps := pkg.Dependencies{
 		Deps: make(map[string]pkg.Dependency),
 	}
 
 	// Traverse all dependencies in kcl.mod
 	for _, d := range deps.Deps {
 		if len(d.Name) == 0 {
-			return nil, nil, errors.InvalidDependency
+			return nil, errors.InvalidDependency
 		}
 
 		existDep := c.dependencyExists(&d, &lockDeps)
@@ -1171,24 +1160,23 @@ func (c *KpmClient) downloadDeps(deps pkg.Dependencies, lockDeps pkg.Dependencie
 		expectedSum := lockDeps.Deps[d.Name].Sum
 		// Clean the cache
 		if len(c.homePath) == 0 || len(d.FullName) == 0 {
-			return nil, nil, errors.InternalBug
+			return nil, errors.InternalBug
 		}
 		dir := filepath.Join(c.homePath, d.FullName)
 		os.RemoveAll(dir)
 
 		// download dependencies
-
 		lockedDep, err := c.Download(&d, dir)
 		if err != nil {
-			return nil, nil, err
+			return nil, err
 		}
 
 		if !lockedDep.IsFromLocal() {
 			if !c.noSumCheck && expectedSum != "" &&
 				lockedDep.Sum != expectedSum &&
 				existDep != nil &&
 				existDep.FullName == d.FullName {
-				return nil, nil, reporter.NewErrorEvent(
+				return nil, reporter.NewErrorEvent(
 					reporter.CheckSumMismatch,
 					errors.CheckSumMismatchError,
 					fmt.Sprintf("checksum for '%s' changed in lock file", lockedDep.Name),
@@ -1201,10 +1189,16 @@ func (c *KpmClient) downloadDeps(deps pkg.Dependencies, lockDeps pkg.Dependencie
 		lockDeps.Deps[d.Name] = *lockedDep
 	}
 
-	depGraph := graph.New(graph.StringHash, graph.Directed())
+	// necessary to make a copy as when we are updating kcl.mod in below for loop
+	// then newDeps.Deps gets updated and range gets an extra value to iterate through
+	// this messes up the dependency graph
+	newDepsCopy := make(map[string]pkg.Dependency)
+	for k, v := range newDeps.Deps {
+		newDepsCopy[k] = v
+	}
 
 	// Recursively download the dependencies of the new dependencies.
-	for _, d := range newDeps.Deps {
+	for _, d := range newDepsCopy {
 		// Load kcl.mod file of the new downloaded dependencies.
 		deppkg, err := pkg.LoadKclPkg(filepath.Join(c.homePath, d.FullName))
 		if len(d.LocalFullPath) != 0 {
@@ -1215,37 +1209,31 @@ func (c *KpmClient) downloadDeps(deps pkg.Dependencies, lockDeps pkg.Dependencie
 			if os.IsNotExist(err) {
 				continue
 			}
-			return nil, nil, err
-		}
-
-		// Download the dependencies.
-		nested, nestedDepGraph, err := c.downloadDeps(deppkg.ModFile.Dependencies, lockDeps)
-		if err != nil {
-			return nil, nil, err
+			return nil, err
 		}
 
 		source := fmt.Sprintf("%s@%s", d.Name, d.Version)
 		err = depGraph.AddVertex(source)
 		if err != nil && err != graph.ErrVertexAlreadyExists {
-			return nil, nil, err
+			return nil, err
 		}
 
-		sourcesOfNestedDepGraph, err := pkgGraph.FindSources(nestedDepGraph)
+		err = depGraph.AddEdge(parent, source)
 		if err != nil {
-			return nil, nil, err
+			if err == graph.ErrEdgeCreatesCycle {
+				return nil, reporter.NewErrorEvent(
+					reporter.CircularDependencyExist,
+					nil,
+					"adding dependencies results in a cycle",
+				)
+			}
+			return nil, err
 		}
 
-		depGraph, err = pkgGraph.Union(depGraph, nestedDepGraph)
+		// Download the dependencies.
+		nested, err := c.downloadDeps(deppkg.ModFile.Dependencies, lockDeps, depGraph, source)
 		if err != nil {
-			return nil, nil, err
-		}
-
-		// make an edge between the source of all nested dep graph and main dep graph
-		for _, sourceOfNestedDepGraph := range sourcesOfNestedDepGraph {
-			err = depGraph.AddEdge(source, sourceOfNestedDepGraph)
-			if err != nil && err != graph.ErrEdgeAlreadyExists {
-				return nil, nil, err
-			}
+			return nil, err
 		}
 
 		// Update kcl.mod.
@@ -1256,7 +1244,7 @@ func (c *KpmClient) downloadDeps(deps pkg.Dependencies, lockDeps pkg.Dependencie
 		}
 	}
 
-	return &newDeps, depGraph, nil
+	return &newDeps, nil
 }
 
 // pullTarFromOci will pull a kcl package tar file from oci registry.

pkg/client/client_test.go

@@ -19,6 +19,7 @@ import (
 	"kcl-lang.io/kpm/pkg/git"
 	"kcl-lang.io/kpm/pkg/opt"
 	pkg "kcl-lang.io/kpm/pkg/package"
+	"kcl-lang.io/kpm/pkg/reporter"
 	"kcl-lang.io/kpm/pkg/runner"
 	"kcl-lang.io/kpm/pkg/utils"
 )
@@ -126,7 +127,7 @@ func TestDependencyGraph(t *testing.T) {
 	kclPkg, err := kpmcli.LoadPkgFromPath(testDir)
 	assert.Equal(t, err, nil)
 
-	depGraph, err := kpmcli.GetDependencyGraph(kclPkg)
+	_, depGraph, err := kpmcli.InitGraphAndDownloadDeps(kclPkg)
 	assert.Equal(t, err, nil)
 	adjMap, err := depGraph.AdjacencyMap()
 	assert.Equal(t, err, nil)
@@ -157,6 +158,34 @@ func TestDependencyGraph(t *testing.T) {
 	)
 }
 
+func TestCyclicDependency(t *testing.T) {
+	testDir := getTestDir("test_cyclic_dependency")
+	assert.Equal(t, utils.DirExists(filepath.Join(testDir, "aaa")), true)
+	assert.Equal(t, utils.DirExists(filepath.Join(testDir, "aaa/kcl.mod")), true)
+	assert.Equal(t, utils.DirExists(filepath.Join(testDir, "bbb")), true)
+	assert.Equal(t, utils.DirExists(filepath.Join(testDir, "bbb/kcl.mod")), true)
+
+	pkg_path := filepath.Join(testDir, "aaa")
+
+	kpmcli, err := NewKpmClient()
+	assert.Equal(t, err, nil)
+	kclPkg, err := kpmcli.LoadPkgFromPath(pkg_path)
+	assert.Equal(t, err, nil)
+
+	currentDir, err := os.Getwd()
+	assert.Equal(t, err, nil)
+	err = os.Chdir(pkg_path)
+	assert.Equal(t, err, nil)
+
+	_, _, err = kpmcli.InitGraphAndDownloadDeps(kclPkg)
+	assert.Equal(t, err, reporter.NewErrorEvent(
+		reporter.CircularDependencyExist, nil, "adding dependencies results in a cycle",
+	))
+
+	err = os.Chdir(currentDir)
+	assert.Equal(t, err, nil)
+}
+
 func TestInitEmptyPkg(t *testing.T) {
 	testDir := initTestDir("test_init_empty_mod")
 	kclPkg := pkg.NewKclPkg(&opt.InitOptions{Name: "test_name", InitPath: testDir})

pkg/client/test_data/test_cyclic_dependency/aaa/kcl.mod

@@ -0,0 +1,7 @@
+[package]
+name = "aaa"
+edition = "0.0.1"
+version = "0.0.1"
+
+[dependencies]
+bbb = { path = "../bbb" }

pkg/client/test_data/test_cyclic_dependency/aaa/kcl.mod.lock

@@ -0,0 +1,6 @@
+[dependencies]
+  [dependencies.bbb]
+    name = "bbb"
+    full_name = "bbb_"
+    sum = "Cgx5yepaNrS3/p4fK04RQWf1dEpG3j49ntFqzrJJsxU="
+    path = "../bbb"

pkg/client/test_data/test_cyclic_dependency/bbb/kcl.mod

@@ -0,0 +1,7 @@
+[package]
+name = "bbb"
+edition = "0.0.1"
+version = "0.0.1"
+
+[dependencies]
+aaa = { path = "../aaa" }

pkg/client/test_data/test_cyclic_dependency/bbb/kcl.mod.lock

@@ -0,0 +1,8 @@
+[dependencies]
+  [dependencies.aaa]
+    name = "aaa"
+    full_name = "aaa_0.0.1"
+    version = "0.0.1"
+    sum = "hgAM5wYFzXo83S+OiT0LaBDnHU7Q9vqq3BPXlTDr8TY="
+    path = "../aaa"
+

pkg/cmd/cmd_graph.go

@@ -62,7 +62,7 @@ func KpmGraph(c *cli.Context, kpmcli *client.KpmClient) error {
 		return err
 	}
 
-	depGraph, err := kpmcli.GetDependencyGraph(kclPkg)
+	_, depGraph, err := kpmcli.InitGraphAndDownloadDeps(kclPkg)
 	if err != nil {
 		return err
 	}

pkg/reporter/reporter.go

@@ -103,6 +103,7 @@ const (
 	AddItselfAsDep
 	PkgTagExists
 	DependencyNotFound
+	CircularDependencyExist
 	RemoveDep
 	AddDep
 	KclModNotFound