From 95ec19f2bef2f04e80a68694180a127473a8dcef Mon Sep 17 00:00:00 2001 From: Valentin Maerten Date: Sat, 18 Jul 2026 23:28:14 +0200 Subject: [PATCH] refactor: centralize fingerprint method resolution in a Fingerprinter The decision "is this task up to date?" was spread over five call sites: the method-selection idiom (task method falling back to the Taskfile method) was copied verbatim in RunTask, Status and ToEditorOutput, and the sources checker was constructed separately in statusOnError and in compiledTask for the CHECKSUM/TIMESTAMP variable injection. Introduce a Fingerprinter in internal/fingerprint that owns method resolution and checker construction behind a small interface (UpToDate, OnError, Kind, SourceValue), and route all five call sites through it. It is built on the fly from the Executor's current state because exported fields like Dry may legitimately be mutated between runs. This also fixes the fingerprint variable ignoring a method set at the Taskfile level: compiledTask picked the checker from the task's own method only, so with a Taskfile-level "method: timestamp" the injected variable was computed by the checksum checker while the up-to-date check used timestamp. The variable now follows the same resolution as the up-to-date check, and is no longer injected when the effective method is "none". --- CHANGELOG.md | 5 + executor.go | 13 ++ help.go | 13 +- internal/fingerprint/fingerprinter.go | 179 ++++++++++++++++++ .../{task_test.go => fingerprinter_test.go} | 7 +- internal/fingerprint/task.go | 132 ------------- status.go | 27 +-- task.go | 13 +- task_test.go | 51 +++++ testdata/method_taskfile_none/Taskfile.yml | 10 + testdata/method_taskfile_none/source.txt | 1 + .../method_taskfile_timestamp/Taskfile.yml | 10 + testdata/method_taskfile_timestamp/source.txt | 1 + variables.go | 16 +- 14 files changed, 281 insertions(+), 197 deletions(-) create mode 100644 internal/fingerprint/fingerprinter.go rename internal/fingerprint/{task_test.go => fingerprinter_test.go} (97%) delete mode 100644 internal/fingerprint/task.go create mode 100644 testdata/method_taskfile_none/Taskfile.yml create mode 100644 testdata/method_taskfile_none/source.txt create mode 100644 testdata/method_taskfile_timestamp/Taskfile.yml create mode 100644 testdata/method_taskfile_timestamp/source.txt diff --git a/CHANGELOG.md b/CHANGELOG.md index bc921400c5..0e1c1006c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +- Fixed the fingerprint variable (`{{.CHECKSUM}}`/`{{.TIMESTAMP}}`) ignoring a + `method:` set at the Taskfile level: the variable now follows the same method + resolution as the up-to-date check, and is no longer injected when the + effective method is `none` (#2924 by @vmaerten). + - Considerably improve performance of fingerprinting on large repositories (monorepos). Fingerprinting is up to 86% faster and make up to 70% fewer memory allocations on the more advanced scenarios. Benchmarks were added as diff --git a/executor.go b/executor.go index 783f18ed0d..210350fea0 100644 --- a/executor.go +++ b/executor.go @@ -10,6 +10,7 @@ import ( "github.com/puzpuzpuz/xsync/v4" "github.com/sajari/fuzzy" + "github.com/go-task/task/v3/internal/fingerprint" "github.com/go-task/task/v3/internal/logger" "github.com/go-task/task/v3/internal/output" "github.com/go-task/task/v3/internal/sort" @@ -122,6 +123,18 @@ func (e *Executor) Options(opts ...ExecutorOption) { } } +// fingerprinter returns a [fingerprint.Fingerprinter] reflecting the +// Executor's current state. It is built on the fly rather than once in Setup +// because fields like Dry may be mutated between runs. +func (e *Executor) fingerprinter() *fingerprint.Fingerprinter { + return fingerprint.NewFingerprinter( + e.Taskfile.Method, + e.TempDir.Fingerprint, + e.Dry, + e.Logger, + ) +} + // WithDir sets the working directory of the [Executor]. By default, the // directory is set to the user's current working directory. func WithDir(dir string) ExecutorOption { diff --git a/help.go b/help.go index 9998bd38ad..c6399363eb 100644 --- a/help.go +++ b/help.go @@ -12,7 +12,6 @@ import ( "golang.org/x/sync/errgroup" "github.com/go-task/task/v3/internal/editors" - "github.com/go-task/task/v3/internal/fingerprint" "github.com/go-task/task/v3/internal/logger" "github.com/go-task/task/v3/internal/sort" "github.com/go-task/task/v3/taskfile/ast" @@ -151,17 +150,7 @@ func (e *Executor) ToEditorOutput(tasks []*ast.Task, noStatus bool, nested bool) return nil } - // Get the fingerprinting method to use - method := e.Taskfile.Method - if tasks[i].Method != "" { - method = tasks[i].Method - } - upToDate, err := fingerprint.IsTaskUpToDate(context.Background(), tasks[i], - fingerprint.WithMethod(method), - fingerprint.WithTempDir(e.TempDir.Fingerprint), - fingerprint.WithDry(e.Dry), - fingerprint.WithLogger(e.Logger), - ) + upToDate, err := e.fingerprinter().UpToDate(context.Background(), tasks[i]) if err != nil { return err } diff --git a/internal/fingerprint/fingerprinter.go b/internal/fingerprint/fingerprinter.go new file mode 100644 index 0000000000..76ac1f751b --- /dev/null +++ b/internal/fingerprint/fingerprinter.go @@ -0,0 +1,179 @@ +package fingerprint + +import ( + "context" + + "github.com/go-task/task/v3/internal/logger" + "github.com/go-task/task/v3/taskfile/ast" +) + +type ( + // A FingerprinterOption is a functional option for a [Fingerprinter]. + FingerprinterOption func(*Fingerprinter) + + // A Fingerprinter answers whether a task is up-to-date. It owns the + // resolution of the fingerprinting method (the task's method, falling back + // to the default) and the construction of the underlying checkers, so that + // every caller gets the same answer for the same task. + Fingerprinter struct { + defaultMethod string + tempDir string + dry bool + logger *logger.Logger + statusChecker StatusCheckable + sourcesChecker SourcesCheckable + } +) + +// WithStatusChecker allows a custom [StatusCheckable] to be used instead of +// the default one. +func WithStatusChecker(checker StatusCheckable) FingerprinterOption { + return func(f *Fingerprinter) { + f.statusChecker = checker + } +} + +// WithSourcesChecker allows a custom [SourcesCheckable] to be used instead of +// the one selected by the resolved fingerprinting method. +func WithSourcesChecker(checker SourcesCheckable) FingerprinterOption { + return func(f *Fingerprinter) { + f.sourcesChecker = checker + } +} + +// NewFingerprinter creates a new [Fingerprinter]. The defaultMethod is used +// for tasks that don't declare a method of their own. +func NewFingerprinter( + defaultMethod string, + tempDir string, + dry bool, + logger *logger.Logger, + opts ...FingerprinterOption, +) *Fingerprinter { + f := &Fingerprinter{ + defaultMethod: defaultMethod, + tempDir: tempDir, + dry: dry, + logger: logger, + } + for _, opt := range opts { + opt(f) + } + return f +} + +func (f *Fingerprinter) resolveMethod(t *ast.Task) string { + if t.Method != "" { + return t.Method + } + return f.defaultMethod +} + +// Kind returns the kind of fingerprint variable ("checksum", "timestamp" or +// "none") produced by the method resolved for the given task. Unknown methods +// fall back to "checksum"; they are only rejected by [NewSourcesChecker]. +func (f *Fingerprinter) Kind(t *ast.Task) string { + if f.sourcesChecker != nil { + return f.sourcesChecker.Kind() + } + switch method := f.resolveMethod(t); method { + case "timestamp", "none": + return method + default: + return "checksum" + } +} + +// SourceValue returns the value of the fingerprint variable (CHECKSUM or +// TIMESTAMP) for the given task. It is potentially expensive, so callers +// should only invoke it when the task actually references the variable. +func (f *Fingerprinter) SourceValue(t *ast.Task) (any, error) { + sourcesChecker, err := f.resolveSourcesChecker(f.Kind(t)) + if err != nil { + return nil, err + } + return sourcesChecker.Value(t) +} + +// UpToDate reports whether the given task is up-to-date, considering both its +// status commands and its sources. +// +// | Status up-to-date | Sources up-to-date | Task is up-to-date | +// | ----------------- | ------------------ | ------------------ | +// | not set | not set | false | +// | not set | true | true | +// | not set | false | false | +// | true | not set | true | +// | true | true | true | +// | true | false | false | +// | false | not set | false | +// | false | true | false | +// | false | false | false | +func (f *Fingerprinter) UpToDate(ctx context.Context, t *ast.Task) (bool, error) { + var statusUpToDate bool + var sourcesUpToDate bool + + statusChecker := f.statusChecker + if statusChecker == nil { + statusChecker = NewStatusChecker(f.logger) + } + sourcesChecker, err := f.resolveSourcesChecker(f.resolveMethod(t)) + if err != nil { + return false, err + } + + statusIsSet := len(t.Status) != 0 + sourcesIsSet := len(t.Sources) != 0 + + // If status is set, check if it is up-to-date + if statusIsSet { + statusUpToDate, err = statusChecker.IsUpToDate(ctx, t) + if err != nil { + return false, err + } + } + + // If sources is set, check if they are up-to-date + if sourcesIsSet { + sourcesUpToDate, err = sourcesChecker.IsUpToDate(t) + if err != nil { + return false, err + } + } + + // If both status and sources are set, the task is up-to-date if both are up-to-date + if statusIsSet && sourcesIsSet { + return statusUpToDate && sourcesUpToDate, nil + } + + // If only status is set, the task is up-to-date if the status is up-to-date + if statusIsSet { + return statusUpToDate, nil + } + + // If only sources is set, the task is up-to-date if the sources are up-to-date + if sourcesIsSet { + return sourcesUpToDate, nil + } + + // If no status or sources are set, the task should always run + // i.e. it is never considered "up-to-date" + return false, nil +} + +// OnError gives the sources checker resolved for the given task a chance to +// clean up after a failed run. +func (f *Fingerprinter) OnError(t *ast.Task) error { + sourcesChecker, err := f.resolveSourcesChecker(f.resolveMethod(t)) + if err != nil { + return err + } + return sourcesChecker.OnError(t) +} + +func (f *Fingerprinter) resolveSourcesChecker(method string) (SourcesCheckable, error) { + if f.sourcesChecker != nil { + return f.sourcesChecker, nil + } + return NewSourcesChecker(method, f.tempDir, f.dry) +} diff --git a/internal/fingerprint/task_test.go b/internal/fingerprint/fingerprinter_test.go similarity index 97% rename from internal/fingerprint/task_test.go rename to internal/fingerprint/fingerprinter_test.go index 3452b19c91..eb123910dc 100644 --- a/internal/fingerprint/task_test.go +++ b/internal/fingerprint/fingerprinter_test.go @@ -23,7 +23,7 @@ import ( // | false | not set | false | // | false | true | false | // | false | false | false | -func TestIsTaskUpToDate(t *testing.T) { +func TestFingerprinterUpToDate(t *testing.T) { t.Parallel() tests := []struct { @@ -162,12 +162,11 @@ func TestIsTaskUpToDate(t *testing.T) { tt.setupMockSourcesChecker(mockSourcesChecker) } - result, err := IsTaskUpToDate( - t.Context(), - tt.task, + f := NewFingerprinter("checksum", "", false, nil, WithStatusChecker(mockStatusChecker), WithSourcesChecker(mockSourcesChecker), ) + result, err := f.UpToDate(t.Context(), tt.task) require.NoError(t, err) assert.Equal(t, tt.expected, result) }) diff --git a/internal/fingerprint/task.go b/internal/fingerprint/task.go deleted file mode 100644 index 2b48e114c9..0000000000 --- a/internal/fingerprint/task.go +++ /dev/null @@ -1,132 +0,0 @@ -package fingerprint - -import ( - "context" - - "github.com/go-task/task/v3/internal/logger" - "github.com/go-task/task/v3/taskfile/ast" -) - -type ( - CheckerOption func(*CheckerConfig) - CheckerConfig struct { - method string - dry bool - tempDir string - logger *logger.Logger - statusChecker StatusCheckable - sourcesChecker SourcesCheckable - } -) - -func WithMethod(method string) CheckerOption { - return func(config *CheckerConfig) { - config.method = method - } -} - -func WithDry(dry bool) CheckerOption { - return func(config *CheckerConfig) { - config.dry = dry - } -} - -func WithTempDir(tempDir string) CheckerOption { - return func(config *CheckerConfig) { - config.tempDir = tempDir - } -} - -func WithLogger(logger *logger.Logger) CheckerOption { - return func(config *CheckerConfig) { - config.logger = logger - } -} - -func WithStatusChecker(checker StatusCheckable) CheckerOption { - return func(config *CheckerConfig) { - config.statusChecker = checker - } -} - -func WithSourcesChecker(checker SourcesCheckable) CheckerOption { - return func(config *CheckerConfig) { - config.sourcesChecker = checker - } -} - -func IsTaskUpToDate( - ctx context.Context, - t *ast.Task, - opts ...CheckerOption, -) (bool, error) { - var statusUpToDate bool - var sourcesUpToDate bool - var err error - - // Default config - config := &CheckerConfig{ - method: "none", - tempDir: "", - dry: false, - logger: nil, - statusChecker: nil, - sourcesChecker: nil, - } - - // Apply functional options - for _, opt := range opts { - opt(config) - } - - // If no status checker was given, set up the default one - if config.statusChecker == nil { - config.statusChecker = NewStatusChecker(config.logger) - } - - // If no sources checker was given, set up the default one - if config.sourcesChecker == nil { - config.sourcesChecker, err = NewSourcesChecker(config.method, config.tempDir, config.dry) - if err != nil { - return false, err - } - } - - statusIsSet := len(t.Status) != 0 - sourcesIsSet := len(t.Sources) != 0 - - // If status is set, check if it is up-to-date - if statusIsSet { - statusUpToDate, err = config.statusChecker.IsUpToDate(ctx, t) - if err != nil { - return false, err - } - } - - // If sources is set, check if they are up-to-date - if sourcesIsSet { - sourcesUpToDate, err = config.sourcesChecker.IsUpToDate(t) - if err != nil { - return false, err - } - } - - // If both status and sources are set, the task is up-to-date if both are up-to-date - if statusIsSet && sourcesIsSet { - return statusUpToDate && sourcesUpToDate, nil - } - - // If only status is set, the task is up-to-date if the status is up-to-date - if statusIsSet { - return statusUpToDate, nil - } - - // If only sources is set, the task is up-to-date if the sources are up-to-date - if sourcesIsSet { - return sourcesUpToDate, nil - } - - // If no status or sources are set, the task should always run - // i.e. it is never considered "up-to-date" - return false, nil -} diff --git a/status.go b/status.go index ae40f5ba5f..21fe861bb7 100644 --- a/status.go +++ b/status.go @@ -4,33 +4,18 @@ import ( "context" "fmt" - "github.com/go-task/task/v3/internal/fingerprint" "github.com/go-task/task/v3/taskfile/ast" ) // Status returns an error if any the of given tasks is not up-to-date func (e *Executor) Status(ctx context.Context, calls ...*Call) error { for _, call := range calls { - - // Compile the task t, err := e.CompiledTask(call) if err != nil { return err } - // Get the fingerprinting method to use - method := e.Taskfile.Method - if t.Method != "" { - method = t.Method - } - - // Check if the task is up-to-date - isUpToDate, err := fingerprint.IsTaskUpToDate(ctx, t, - fingerprint.WithMethod(method), - fingerprint.WithTempDir(e.TempDir.Fingerprint), - fingerprint.WithDry(e.Dry), - fingerprint.WithLogger(e.Logger), - ) + isUpToDate, err := e.fingerprinter().UpToDate(ctx, t) if err != nil { return err } @@ -42,13 +27,5 @@ func (e *Executor) Status(ctx context.Context, calls ...*Call) error { } func (e *Executor) statusOnError(t *ast.Task) error { - method := t.Method - if method == "" { - method = e.Taskfile.Method - } - checker, err := fingerprint.NewSourcesChecker(method, e.TempDir.Fingerprint, e.Dry) - if err != nil { - return err - } - return checker.OnError(t) + return e.fingerprinter().OnError(t) } diff --git a/task.go b/task.go index 98d340c976..482dc5cef7 100644 --- a/task.go +++ b/task.go @@ -15,7 +15,6 @@ import ( "github.com/go-task/task/v3/errors" "github.com/go-task/task/v3/internal/env" "github.com/go-task/task/v3/internal/execext" - "github.com/go-task/task/v3/internal/fingerprint" "github.com/go-task/task/v3/internal/logger" "github.com/go-task/task/v3/internal/output" "github.com/go-task/task/v3/internal/slicesext" @@ -221,17 +220,7 @@ func (e *Executor) RunTask(ctx context.Context, call *Call) error { return err } - // Get the fingerprinting method to use - method := e.Taskfile.Method - if t.Method != "" { - method = t.Method - } - upToDate, err := fingerprint.IsTaskUpToDate(ctx, t, - fingerprint.WithMethod(method), - fingerprint.WithTempDir(e.TempDir.Fingerprint), - fingerprint.WithDry(e.Dry), - fingerprint.WithLogger(e.Logger), - ) + upToDate, err := e.fingerprinter().UpToDate(ctx, t) if err != nil { return err } diff --git a/task_test.go b/task_test.go index b56930e77e..7425358f62 100644 --- a/task_test.go +++ b/task_test.go @@ -653,6 +653,57 @@ func TestStatusChecksumMissingGenerated(t *testing.T) { // nolint:paralleltest / require.NoError(t, err, "generated.txt should be recreated after third run") } +// TestFingerprintVarMethodInheritedFromTaskfile asserts that the fingerprint +// variable injected into a task follows the Taskfile-level method when the +// task doesn't declare one, like the up-to-date check does. +func TestFingerprintVarMethodInheritedFromTaskfile(t *testing.T) { + t.Parallel() + + const dir = "testdata/method_taskfile_timestamp" + _ = os.RemoveAll(filepathext.SmartJoin(dir, ".task")) + + var buff bytes.Buffer + e := task.NewExecutor( + task.WithDir(dir), + task.WithStdout(&buff), + task.WithStderr(&buff), + task.WithTempDir(task.TempDir{ + Remote: filepathext.SmartJoin(dir, ".task"), + Fingerprint: filepathext.SmartJoin(dir, ".task"), + }), + ) + require.NoError(t, e.Setup()) + + require.NoError(t, e.Run(t.Context(), &task.Call{Task: "build"})) + assert.Contains(t, buff.String(), "ts=") + assert.NotContains(t, buff.String(), "", "TIMESTAMP should be injected when method is inherited from the Taskfile") +} + +// TestFingerprintVarMethodNone asserts that no fingerprint variable is +// injected when the effective method is "none", including when it is +// inherited from the Taskfile level. +func TestFingerprintVarMethodNone(t *testing.T) { + t.Parallel() + + const dir = "testdata/method_taskfile_none" + _ = os.RemoveAll(filepathext.SmartJoin(dir, ".task")) + + var buff bytes.Buffer + e := task.NewExecutor( + task.WithDir(dir), + task.WithStdout(&buff), + task.WithStderr(&buff), + task.WithTempDir(task.TempDir{ + Remote: filepathext.SmartJoin(dir, ".task"), + Fingerprint: filepathext.SmartJoin(dir, ".task"), + }), + ) + require.NoError(t, e.Setup()) + + require.NoError(t, e.Run(t.Context(), &task.Call{Task: "build"})) + assert.Contains(t, buff.String(), "cs=\n", "CHECKSUM should not be injected when the effective method is none") +} + func writeFile(t *testing.T, dir, name, content string) { t.Helper() require.NoError(t, os.WriteFile(filepathext.SmartJoin(dir, name), []byte(content), 0o644)) diff --git a/testdata/method_taskfile_none/Taskfile.yml b/testdata/method_taskfile_none/Taskfile.yml new file mode 100644 index 0000000000..e212720cfa --- /dev/null +++ b/testdata/method_taskfile_none/Taskfile.yml @@ -0,0 +1,10 @@ +version: '3' + +method: none + +tasks: + build: + cmds: + - echo "cs={{.CHECKSUM}}" + sources: + - ./source.txt diff --git a/testdata/method_taskfile_none/source.txt b/testdata/method_taskfile_none/source.txt new file mode 100644 index 0000000000..5a18cd2fbf --- /dev/null +++ b/testdata/method_taskfile_none/source.txt @@ -0,0 +1 @@ +source diff --git a/testdata/method_taskfile_timestamp/Taskfile.yml b/testdata/method_taskfile_timestamp/Taskfile.yml new file mode 100644 index 0000000000..dc0f139e5d --- /dev/null +++ b/testdata/method_taskfile_timestamp/Taskfile.yml @@ -0,0 +1,10 @@ +version: '3' + +method: timestamp + +tasks: + build: + cmds: + - echo "ts={{.TIMESTAMP}}" + sources: + - ./source.txt diff --git a/testdata/method_taskfile_timestamp/source.txt b/testdata/method_taskfile_timestamp/source.txt new file mode 100644 index 0000000000..5a18cd2fbf --- /dev/null +++ b/testdata/method_taskfile_timestamp/source.txt @@ -0,0 +1 @@ +source diff --git a/variables.go b/variables.go index 900f87c0d9..a72cbffe64 100644 --- a/variables.go +++ b/variables.go @@ -208,21 +208,13 @@ func (e *Executor) compiledTask(call *Call, evaluateShVars bool) (*ast.Task, err } } - if len(origTask.Sources) > 0 && origTask.Method != "none" { - var checker fingerprint.SourcesCheckable - - if origTask.Method == "timestamp" { - checker = fingerprint.NewTimestampChecker(e.TempDir.Fingerprint, e.Dry) - } else { - checker = fingerprint.NewChecksumChecker(e.TempDir.Fingerprint, e.Dry) - } - - if origTask.ReferencesFingerprintVar(checker.Kind()) { - value, err := checker.Value(&new) + if kind := e.fingerprinter().Kind(&new); len(origTask.Sources) > 0 && kind != "none" { + if origTask.ReferencesFingerprintVar(kind) { + value, err := e.fingerprinter().SourceValue(&new) if err != nil { return nil, err } - vars.Set(strings.ToUpper(checker.Kind()), ast.Var{Live: value}) + vars.Set(strings.ToUpper(kind), ast.Var{Live: value}) // Adding new variables, requires us to refresh the templaters // cache of the the values manually