Skip to content

Commit

Permalink
Address feedback.
Browse files Browse the repository at this point in the history
  • Loading branch information
nathanhammond committed Apr 13, 2023
1 parent fc23ab3 commit 63f7009
Show file tree
Hide file tree
Showing 11 changed files with 53 additions and 39 deletions.
4 changes: 2 additions & 2 deletions cli/integration_tests/dry_json/monorepo.t
Expand Up @@ -121,7 +121,7 @@ Setup
},
"expandedOutputs": [],
"framework": "<NO FRAMEWORK DETECTED>",
"envMode": "infer",
"envMode": "loose",
"environmentVariables": {
"configured": [],
"inferred": [],
Expand Down Expand Up @@ -170,7 +170,7 @@ Setup
},
"expandedOutputs": [],
"framework": "<NO FRAMEWORK DETECTED>",
"envMode": "infer",
"envMode": "loose",
"environmentVariables": {
"configured": [
"NODE_ENV="
Expand Down
2 changes: 1 addition & 1 deletion cli/integration_tests/dry_json/single_package.t
Expand Up @@ -69,7 +69,7 @@ Setup
},
"expandedOutputs": [],
"framework": "\u003cNO FRAMEWORK DETECTED\u003e",
"envMode": "infer",
"envMode": "loose",
"environmentVariables": {
"configured": [],
"inferred": [],
Expand Down
2 changes: 1 addition & 1 deletion cli/integration_tests/dry_json/single_package_no_config.t
Expand Up @@ -60,7 +60,7 @@ Setup
},
"expandedOutputs": [],
"framework": "\u003cNO FRAMEWORK DETECTED\u003e",
"envMode": "infer",
"envMode": "loose",
"environmentVariables": {
"configured": [],
"inferred": [],
Expand Down
4 changes: 2 additions & 2 deletions cli/integration_tests/dry_json/single_package_with_deps.t
Expand Up @@ -80,7 +80,7 @@ Setup
},
"expandedOutputs": [],
"framework": "\u003cNO FRAMEWORK DETECTED\u003e",
"envMode": "infer",
"envMode": "loose",
"environmentVariables": {
"configured": [],
"inferred": [],
Expand Down Expand Up @@ -128,7 +128,7 @@ Setup
},
"expandedOutputs": [],
"framework": "\u003cNO FRAMEWORK DETECTED\u003e",
"envMode": "infer",
"envMode": "loose",
"environmentVariables": {
"configured": [],
"inferred": [],
Expand Down
2 changes: 1 addition & 1 deletion cli/integration_tests/run_summary/error.t
Expand Up @@ -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": [],
Expand Down
15 changes: 13 additions & 2 deletions cli/internal/graph/graph.go
Expand Up @@ -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
Expand All @@ -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
Expand Down
6 changes: 1 addition & 5 deletions cli/internal/run/dry_run.go
Expand Up @@ -28,18 +28,14 @@ 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 {
defer turboCache.Shutdown()

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
Expand Down
9 changes: 7 additions & 2 deletions cli/internal/run/global_hash.go
Expand Up @@ -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")
}
}

Expand Down
6 changes: 1 addition & 5 deletions cli/internal/run/real_run.go
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions cli/internal/run/run.go
Expand Up @@ -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(
Expand Down Expand Up @@ -386,6 +391,7 @@ func (r *run) run(ctx gocontext.Context, targets []string) error {
taskHashTracker,
turboCache,
turboJSON,
globalEnvMode,
r.base,
summary,
)
Expand All @@ -400,6 +406,7 @@ func (r *run) run(ctx gocontext.Context, targets []string) error {
taskHashTracker,
turboCache,
turboJSON,
globalEnvMode,
packagesInScope,
r.base,
summary,
Expand Down
35 changes: 17 additions & 18 deletions cli/internal/taskhash/taskhash.go
Expand Up @@ -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,
Expand All @@ -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")
}
}

Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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)
}
Expand Down

0 comments on commit 63f7009

Please sign in to comment.