From 63f70091e387921849a076d78002ed401beba064 Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Thu, 13 Apr 2023 14:48:26 +0800 Subject: [PATCH] Address feedback. --- cli/integration_tests/dry_json/monorepo.t | 4 +-- .../dry_json/single_package.t | 2 +- .../dry_json/single_package_no_config.t | 2 +- .../dry_json/single_package_with_deps.t | 4 +-- cli/integration_tests/run_summary/error.t | 2 +- cli/internal/graph/graph.go | 15 ++++++-- cli/internal/run/dry_run.go | 6 +--- cli/internal/run/global_hash.go | 9 +++-- cli/internal/run/real_run.go | 6 +--- cli/internal/run/run.go | 7 ++++ cli/internal/taskhash/taskhash.go | 35 +++++++++---------- 11 files changed, 53 insertions(+), 39 deletions(-) diff --git a/cli/integration_tests/dry_json/monorepo.t b/cli/integration_tests/dry_json/monorepo.t index 895e709a5d919..72335ddc38f27 100644 --- a/cli/integration_tests/dry_json/monorepo.t +++ b/cli/integration_tests/dry_json/monorepo.t @@ -121,7 +121,7 @@ Setup }, "expandedOutputs": [], "framework": "", - "envMode": "infer", + "envMode": "loose", "environmentVariables": { "configured": [], "inferred": [], @@ -170,7 +170,7 @@ Setup }, "expandedOutputs": [], "framework": "", - "envMode": "infer", + "envMode": "loose", "environmentVariables": { "configured": [ "NODE_ENV=" diff --git a/cli/integration_tests/dry_json/single_package.t b/cli/integration_tests/dry_json/single_package.t index 5594cdfd5e653..57a727763f0b1 100644 --- a/cli/integration_tests/dry_json/single_package.t +++ b/cli/integration_tests/dry_json/single_package.t @@ -69,7 +69,7 @@ Setup }, "expandedOutputs": [], "framework": "\u003cNO FRAMEWORK DETECTED\u003e", - "envMode": "infer", + "envMode": "loose", "environmentVariables": { "configured": [], "inferred": [], diff --git a/cli/integration_tests/dry_json/single_package_no_config.t b/cli/integration_tests/dry_json/single_package_no_config.t index d7939a7078cf8..525605e7a518b 100644 --- a/cli/integration_tests/dry_json/single_package_no_config.t +++ b/cli/integration_tests/dry_json/single_package_no_config.t @@ -60,7 +60,7 @@ Setup }, "expandedOutputs": [], "framework": "\u003cNO FRAMEWORK DETECTED\u003e", - "envMode": "infer", + "envMode": "loose", "environmentVariables": { "configured": [], "inferred": [], diff --git a/cli/integration_tests/dry_json/single_package_with_deps.t b/cli/integration_tests/dry_json/single_package_with_deps.t index 5d9432010ca0d..a673af24dd40d 100644 --- a/cli/integration_tests/dry_json/single_package_with_deps.t +++ b/cli/integration_tests/dry_json/single_package_with_deps.t @@ -80,7 +80,7 @@ Setup }, "expandedOutputs": [], "framework": "\u003cNO FRAMEWORK DETECTED\u003e", - "envMode": "infer", + "envMode": "loose", "environmentVariables": { "configured": [], "inferred": [], @@ -128,7 +128,7 @@ Setup }, "expandedOutputs": [], "framework": "\u003cNO FRAMEWORK DETECTED\u003e", - "envMode": "infer", + "envMode": "loose", "environmentVariables": { "configured": [], "inferred": [], diff --git a/cli/integration_tests/run_summary/error.t b/cli/integration_tests/run_summary/error.t index 2b91f87963186..51dbeed0f18cc 100644 --- a/cli/integration_tests/run_summary/error.t +++ b/cli/integration_tests/run_summary/error.t @@ -61,7 +61,7 @@ Validate that we got a full task summary for the failed task with an error in .e }, "expandedOutputs": [], "framework": "", - "envMode": "infer", + "envMode": "loose", "environmentVariables": { "configured": [], "inferred": [], diff --git a/cli/internal/graph/graph.go b/cli/internal/graph/graph.go index 3137a748b46d6..480dec9ad38db 100644 --- a/cli/internal/graph/graph.go +++ b/cli/internal/graph/graph.go @@ -80,8 +80,18 @@ func (g *CompleteGraph) GetPackageTaskVisitor( // Task env mode is only independent when global env mode is `infer`. taskEnvMode := globalEnvMode - if taskEnvMode == util.Infer && taskDefinition.PassthroughEnv != nil { - taskEnvMode = util.Strict + useOldTaskHashable := false + if taskEnvMode == util.Infer { + if taskDefinition.PassthroughEnv != nil { + taskEnvMode = util.Strict + } else { + // If we're in infer mode we have just detected non-usage of strict env vars. + // Since we haven't stabilized this we don't want to break their cache. + useOldTaskHashable = true + + // But our old behavior's actual meaning of this state is `loose`. + taskEnvMode = util.Loose + } } // TODO: maybe we can remove this PackageTask struct at some point @@ -103,6 +113,7 @@ func (g *CompleteGraph) GetPackageTaskVisitor( taskGraph.DownEdges(taskID), logger, passThruArgs, + useOldTaskHashable, ) // Not being able to construct the task hash is a hard error diff --git a/cli/internal/run/dry_run.go b/cli/internal/run/dry_run.go index 679153a57bc7a..067f3b9d4073c 100644 --- a/cli/internal/run/dry_run.go +++ b/cli/internal/run/dry_run.go @@ -28,6 +28,7 @@ func DryRun( _ *taskhash.Tracker, // unused, but keep here for parity with RealRun method signature turboCache cache.Cache, turboJSON *fs.TurboJSON, + globalEnvMode util.EnvMode, base *cmdutil.CmdBase, summary runsummary.Meta, ) error { @@ -35,11 +36,6 @@ func DryRun( taskSummaries := []*runsummary.TaskSummary{} - globalEnvMode := rs.Opts.runOpts.EnvMode - if globalEnvMode == util.Infer && turboJSON.GlobalPassthroughEnv != nil { - globalEnvMode = util.Strict - } - mu := sync.Mutex{} execFunc := func(ctx gocontext.Context, packageTask *nodes.PackageTask, taskSummary *runsummary.TaskSummary) error { // Assign some fallbacks if they were missing diff --git a/cli/internal/run/global_hash.go b/cli/internal/run/global_hash.go index 6ad386a5695b9..2ebf6427eba15 100644 --- a/cli/internal/run/global_hash.go +++ b/cli/internal/run/global_hash.go @@ -72,9 +72,14 @@ func calculateGlobalHashFromHashable(full GlobalHashable) (string, error) { // Remove the passthroughs from hash consideration if we're explicitly loose. full.envVarPassthroughs = nil return fs.HashObject(full) - default: - // When we aren't in infer or loose mode we can hash the whole object as is. + case util.Strict: + // Collapse `nil` and `[]` in strict mode. + if full.envVarPassthroughs == nil { + full.envVarPassthroughs = make([]string, 0) + } return fs.HashObject(full) + default: + panic("unimplemented environment mode") } } diff --git a/cli/internal/run/real_run.go b/cli/internal/run/real_run.go index 3e1d5e66f2044..120cfd836facc 100644 --- a/cli/internal/run/real_run.go +++ b/cli/internal/run/real_run.go @@ -42,6 +42,7 @@ func RealRun( taskHashTracker *taskhash.Tracker, turboCache cache.Cache, turboJSON *fs.TurboJSON, + globalEnvMode util.EnvMode, packagesInScope []string, base *cmdutil.CmdBase, runSummary runsummary.Meta, @@ -72,11 +73,6 @@ func RealRun( runCache := runcache.New(turboCache, base.RepoRoot, rs.Opts.runcacheOpts, colorCache) - globalEnvMode := rs.Opts.runOpts.EnvMode - if globalEnvMode == util.Infer && turboJSON.GlobalPassthroughEnv != nil { - globalEnvMode = util.Strict - } - ec := &execContext{ colorCache: colorCache, runSummary: runSummary, diff --git a/cli/internal/run/run.go b/cli/internal/run/run.go index 500d728f4dfa6..71bd4eede38d3 100644 --- a/cli/internal/run/run.go +++ b/cli/internal/run/run.go @@ -354,6 +354,11 @@ func (r *run) run(ctx gocontext.Context, targets []string) error { } } + globalEnvMode := rs.Opts.runOpts.EnvMode + if globalEnvMode == util.Infer && turboJSON.GlobalPassthroughEnv != nil { + globalEnvMode = util.Strict + } + // RunSummary contains information that is statically analyzable about // the tasks that we expect to run based on the user command. summary := runsummary.NewRunSummary( @@ -386,6 +391,7 @@ func (r *run) run(ctx gocontext.Context, targets []string) error { taskHashTracker, turboCache, turboJSON, + globalEnvMode, r.base, summary, ) @@ -400,6 +406,7 @@ func (r *run) run(ctx gocontext.Context, targets []string) error { taskHashTracker, turboCache, turboJSON, + globalEnvMode, packagesInScope, r.base, summary, diff --git a/cli/internal/taskhash/taskhash.go b/cli/internal/taskhash/taskhash.go index 0082eb468dd03..33989eaed2a8e 100644 --- a/cli/internal/taskhash/taskhash.go +++ b/cli/internal/taskhash/taskhash.go @@ -304,20 +304,9 @@ type oldTaskHashable struct { } // calculateTaskHashFromHashable returns a hash string from the taskHashable -func calculateTaskHashFromHashable(full *taskHashable) (string, error) { - switch full.envMode { - case util.Infer: - if full.passthroughEnv != nil { - // In infer mode, if there is any passThru config (even if it is an empty array) - // we'll hash the whole object, so we can detect changes to that config - // Further, resolve the envMode to the concrete value. - full.envMode = util.Strict - return fs.HashObject(full) - } - - // If we're in infer mode, and there is no global pass through config, - // we can use the old anonymous struct. this will be true for everyone not using the strict env - // feature, and we don't want to break their cache. +func calculateTaskHashFromHashable(full *taskHashable, useOldTaskHashable bool) (string, error) { + // The user is not using the strict environment variables feature. + if useOldTaskHashable { return fs.HashObject(&oldTaskHashable{ packageDir: full.packageDir, hashOfFiles: full.hashOfFiles, @@ -329,13 +318,23 @@ func calculateTaskHashFromHashable(full *taskHashable) (string, error) { globalHash: full.globalHash, taskDependencyHashes: full.taskDependencyHashes, }) + } + + switch full.envMode { case util.Loose: // Remove the passthroughs from hash consideration if we're explicitly loose. full.passthroughEnv = nil return fs.HashObject(full) - default: - // When we aren't in infer or loose mode we can hash the whole object as is. + case util.Strict: + // Collapse `nil` and `[]` in strict mode. + if full.passthroughEnv == nil { + full.passthroughEnv = make([]string, 0) + } return fs.HashObject(full) + case util.Infer: + panic("task inferred status should have already been resolved") + default: + panic("unimplemented environment mode") } } @@ -370,7 +369,7 @@ func (th *Tracker) calculateDependencyHashes(dependencySet dag.Set) ([]string, e // CalculateTaskHash calculates the hash for package-task combination. It is threadsafe, provided // that it has previously been called on its task-graph dependencies. File hashes must be calculated // first. -func (th *Tracker) CalculateTaskHash(packageTask *nodes.PackageTask, dependencySet dag.Set, logger hclog.Logger, args []string) (string, error) { +func (th *Tracker) CalculateTaskHash(packageTask *nodes.PackageTask, dependencySet dag.Set, logger hclog.Logger, args []string, useOldTaskHashable bool) (string, error) { pfs := specFromPackageTask(packageTask) pkgFileHashKey := pfs.ToKey() @@ -416,7 +415,7 @@ func (th *Tracker) CalculateTaskHash(packageTask *nodes.PackageTask, dependencyS hashableEnvPairs: hashableEnvPairs, globalHash: th.globalHash, taskDependencyHashes: taskDependencyHashes, - }) + }, useOldTaskHashable) if err != nil { return "", fmt.Errorf("failed to hash task %v: %v", packageTask.TaskID, hash) }