Merge pull request #15 from yaklabco/feat-cycle-detection

Author: preminger

CHANGELOG.md

@@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- `--dryrun` mode.
+
+- Automated detection of circular dependencies among stavefile targets.
+
 - `CHANGELOG.md`! (And first formally-versioned release of Stave.)
 
 [unreleased]: https://github.com/yaklabco/stave/compare/v0.1.0...HEAD

TODO.md

@@ -26,3 +26,4 @@
 - [x] Parallelize tests w/locking mechanism to prevent parallel tests in same testdata subdir
 - [x] Bring in CI from `goctx` with the fancy caching
 - [x] Validate that every target is run in its own Goroutine
+- [x] Dependency cycle detection

pkg/st/cycle_detector.go

@@ -0,0 +1,101 @@
+package st
+
+import (
+	"log/slog"
+	"runtime"
+	"strings"
+	"sync"
+
+	"github.com/samber/lo"
+	"github.com/yaklabco/stave/pkg/toposort"
+)
+
+const (
+	// maxStackDepthToCheck defines the maximum stack depth for runtime caller inspection.
+	maxStackDepthToCheck = 64
+)
+
+var (
+	depsByID      = make(map[string]toposort.TopoSortable) //nolint:gochecknoglobals // Part of a mutexed pattern.
+	depsByIDMutex sync.RWMutex                             //nolint:gochecknoglobals // Part of a mutexed pattern.
+)
+
+func firstExternalCaller() *runtime.Frame {
+	thisProgCtr, _, _, ok := runtime.Caller(0)
+	if !ok {
+		return nil
+	}
+	thisFunc := runtime.FuncForPC(thisProgCtr)
+	pkgPrefix := getPackagePath(thisFunc)
+
+	// runtime.Callers (0), firstExternalCaller (1), the function calling firstExternalCaller (2)
+	const skip = 2
+	progCtrsAboveUs := make([]uintptr, maxStackDepthToCheck)
+	nProgCtrsAboveUs := runtime.Callers(skip, progCtrsAboveUs)
+	frames := runtime.CallersFrames(progCtrsAboveUs[:nProgCtrsAboveUs])
+
+	for {
+		frame, more := frames.Next()
+
+		// frame.Function is the fully-qualified name:
+		//   "mypkg.myFunc"
+		//   "otherpkg.DoThing"
+		//   "github.com/me/foo/bar.Baz"
+		slog.Debug("checking the frame", slog.String("function", frame.Function), slog.String("pkg_prefix", pkgPrefix))
+		if !strings.HasPrefix(frame.Function, pkgPrefix) {
+			return &frame
+		}
+
+		if !more {
+			break
+		}
+	}
+
+	return nil
+}
+
+func getPackagePath(thisFunc *runtime.Func) string {
+	pkgPrefix := thisFunc.Name()
+	lastSlash := strings.LastIndex(pkgPrefix, "/")
+	lastDot := strings.LastIndex(pkgPrefix, ".")
+	if lastDot > lastSlash {
+		pkgPrefix = pkgPrefix[:lastDot] // e.g. "github.com/me/project/mypkg"
+	}
+	return pkgPrefix
+}
+
+func checkForCycle(funcs []Fn) error {
+	callerFrame := firstExternalCaller()
+	if callerFrame == nil {
+		slog.Warn("could not determine caller, skipping circular-dependency check")
+		return nil
+	}
+
+	callerID := callerFrame.File + ":" + callerFrame.Function
+	slog.Debug("checking for cycle", slog.String("caller_id", callerID))
+
+	funcIDs := make([]string, 0, len(funcs))
+	for _, theFunc := range funcs {
+		theFuncObj := theFunc.Underlying()
+		theFile, _ := theFuncObj.FileLine(0)
+		theFuncID := theFile + ":" + theFuncObj.Name()
+		slog.Debug("adding dependency", slog.String("func_id", theFuncID))
+		funcIDs = append(funcIDs, theFuncID)
+	}
+
+	depsByIDMutex.Lock()
+	defer depsByIDMutex.Unlock()
+	depsByID[callerID] = depsNode{tpID: callerID, dependencyTPIDs: funcIDs}
+
+	_, err := toposort.Sort(lo.Values(depsByID), true)
+
+	return err
+}
+
+type depsNode struct {
+	tpID            string
+	dependencyTPIDs []string
+}
+
+func (n depsNode) TPID() string              { return n.tpID }
+func (n depsNode) DependencyTPIDs() []string { return n.dependencyTPIDs }

pkg/st/deps.go

@@ -137,6 +137,11 @@ func checkFns(fns []interface{}) []Fn {
 
 		funcs[iFunc] = F(theFunc)
 	}
+
+	if err := checkForCycle(funcs); err != nil {
+		panic(fmt.Errorf("checking for cycles in dependency graph: %w", err))
+	}
+
 	return funcs
 }
 
@@ -175,7 +180,11 @@ func changeExit(oldExitCode, newExitCode int) int {
 
 // funcName returns the unique name for the function.
 func funcName(i interface{}) string {
-	return runtime.FuncForPC(reflect.ValueOf(i).Pointer()).Name()
+	return funcObj(i).Name()
+}
+
+func funcObj(i interface{}) *runtime.Func {
+	return runtime.FuncForPC(reflect.ValueOf(i).Pointer())
 }
 
 func displayName(name string) string {

pkg/st/fn.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"runtime"
 )
 
 // Fn represents a function that can be run with st.Deps. Package, Name, and ID must combine to
@@ -23,6 +24,9 @@ type Fn interface {
 
 	// Run should run the function.
 	Run(ctx context.Context) error
+
+	// Underlying should return the original, wrapped function object.
+	Underlying() *runtime.Func
 }
 
 // F takes a function that is compatible as a stave target, and any args that need to be passed to
@@ -78,13 +82,15 @@ func F(target interface{}, args ...interface{}) Fn {
 			}
 			return nil
 		},
+		underlying: funcObj(target),
 	}
 }
 
 type fn struct {
-	name string
-	id   string
-	f    func(ctx context.Context) error
+	name       string
+	id         string
+	f          func(ctx context.Context) error
+	underlying *runtime.Func
 }
 
 // Name returns the fully qualified name of the function.
@@ -102,6 +108,11 @@ func (f fn) Run(ctx context.Context) error {
 	return f.f(ctx)
 }
 
+// Underlying returns the original, wrapped function object.
+func (f fn) Underlying() *runtime.Func {
+	return f.underlying
+}
+
 func checkF(target interface{}, args []interface{}) (bool, bool, error) {
 	theType := reflect.TypeOf(target)
 	if theType == nil || theType.Kind() != reflect.Func {

pkg/stave/cyclic_dependencies_test.go

@@ -0,0 +1,35 @@
+package stave
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestCyclicDependencyDetection verifies proper detection of cyclic dependencies.
+func TestCyclicDependencyDetection(t *testing.T) {
+	t.Parallel()
+	testDataDir := "./testdata/cyclic_dependencies"
+	mu := mutexByDir(testDataDir)
+	mu.Lock()
+	defer mu.Unlock()
+
+	ctx := t.Context()
+
+	stdout := &bytes.Buffer{}
+	stderr := &bytes.Buffer{}
+
+	err := Run(RunParams{
+		BaseCtx: ctx,
+		Dir:     testDataDir,
+		Stdout:  stdout,
+		Stderr:  stderr,
+		Args:    []string{"Step1"},
+	})
+	require.Error(t, err)
+
+	expected := "circular dependency detected"
+	assert.Contains(t, stderr.String(), expected)
+}

pkg/stave/main.go

@@ -30,6 +30,7 @@ import (
 	"github.com/yaklabco/stave/internal/parse"
 	"github.com/yaklabco/stave/pkg/sh"
 	"github.com/yaklabco/stave/pkg/st"
+	"github.com/yaklabco/stave/pkg/stave/prettylog"
 )
 
 const (
@@ -84,7 +85,10 @@ func (i RunParams) UsesStavefiles() bool {
 // function to allow it to be used from other programs, specifically so you can
 // go run a simple file that run's stave's Run.
 func Run(params RunParams) error {
-	logHandler := setupLogger(params)
+	if params.WriterForLogger == nil {
+		params.WriterForLogger = params.Stderr
+	}
+	logHandler := prettylog.SetupPrettyLogger(params.WriterForLogger)
 
 	if params.Debug {
 		logHandler.SetLevel(cblog.DebugLevel)
@@ -124,24 +128,6 @@ func Run(params RunParams) error {
 	return stave(ctx, params)
 }
 
-func setupLogger(params RunParams) *cblog.Logger {
-	if params.WriterForLogger == nil {
-		params.WriterForLogger = params.Stderr
-	}
-
-	logHandler := cblog.NewWithOptions(
-		params.WriterForLogger,
-		cblog.Options{
-			Level:           cblog.WarnLevel, // Setting this to lowest possible value, since slog will handle the actual filtering.
-			ReportTimestamp: true,
-			ReportCaller:    true,
-		},
-	)
-	logger := slog.New(logHandler)
-	slog.SetDefault(logger)
-	return logHandler
-}
-
 func stave(ctx context.Context, params RunParams) error {
 	files, err := Stavefiles(params.Dir, params.GOOS, params.GOARCH, params.UsesStavefiles())
 	if err != nil {

pkg/stave/main_test.go

@@ -601,8 +601,10 @@ func TestVerbose(t *testing.T) {
 	err = Run(runParams)
 	require.NoError(t, err, "stderr was: %s", stderr.String())
 
-	expectedRegexp := `Running target: TestVerbose\n.*hi!\n`
-	assert.Regexp(t, expectedRegexp, stderr.String())
+	expectedOutRegexp := `\bhi!\n`
+	assert.Regexp(t, expectedOutRegexp, stdout.String())
+	expectedErrRegexp := `\bRunning target: TestVerbose\b`
+	assert.Regexp(t, expectedErrRegexp, stderr.String())
 }
 
 func TestList(t *testing.T) {
@@ -1021,11 +1023,13 @@ func TestMultipleTargets(t *testing.T) {
 
 	err := Run(runParams)
 	require.NoError(t, err, "stderr was: %s", stderr.String())
-	expectedRegexp := `Running target: TestVerbose\n.*hi!\nRunning target: ReturnsNilError\n`
-	assert.Regexp(t, expectedRegexp, stderr.String())
+	expectedOutRegexp := `\bhi!`
+	assert.Regexp(t, expectedOutRegexp, stdout.String())
+	expectedErrRegexp := `Running target: TestVerbose\n(.*\n)*Running target: ReturnsNilError\n`
+	assert.Regexp(t, expectedErrRegexp, stderr.String())
 
 	expectedOutStr := "stuff\n"
-	assert.Equal(t, expectedOutStr, stdout.String())
+	assert.Contains(t, stdout.String(), expectedOutStr)
 }
 
 func TestFirstTargetFails(t *testing.T) {
@@ -1364,7 +1368,7 @@ func TestCompiledFlags(t *testing.T) {
 	err = run(stdout, stderr, name, "-v", "testverbose")
 	require.NoError(t, err, "stderr was: %s", stderr.String())
 	want = hiExclam
-	assert.Contains(t, stderr.String(), want)
+	assert.Contains(t, stdout.String(), want)
 
 	// pass list flag -l
 	err = run(stdout, stderr, name, "-l")
@@ -1441,7 +1445,7 @@ func TestCompiledEnvironmentVars(t *testing.T) {
 	err = run(stdout, stderr, name, st.VerboseEnv+"=1", "testverbose")
 	require.NoError(t, err, "stderr was: %s", stderr.String())
 	want = hiExclam
-	assert.Contains(t, stderr.String(), want)
+	assert.Contains(t, stdout.String(), want)
 
 	err = run(stdout, stderr, name, "STAVEFILE_LIST=1")
 	require.NoError(t, err, "stderr was: %s", stderr.String())

pkg/stave/prettylog/prettylog.go

@@ -0,0 +1,23 @@
+package prettylog
+
+import (
+	"io"
+	"log/slog"
+
+	"github.com/charmbracelet/log"
+)
+
+func SetupPrettyLogger(writerForLogger io.Writer) *log.Logger {
+	logHandler := log.NewWithOptions(
+		writerForLogger,
+		log.Options{
+			Level:           log.InfoLevel, // Setting this to lowest possible value, since slog will handle the actual filtering.
+			ReportTimestamp: true,
+			ReportCaller:    true,
+		},
+	)
+	logger := slog.New(logHandler)
+	slog.SetDefault(logger)
+
+	return logHandler
+}

pkg/stave/stavefile_embedder.go

@@ -2,5 +2,5 @@ package stave
 
 import _ "embed"
 
-//go:embed templates/stavefile_tmpl.go
+//go:embed templates/initial_stavefile.go
 var staveTpl string