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