Auto-skip: ensure we account for +base (#3903)

The auto-skip code did not account for the "base" target. Hence, the
change to root-level `ARG`s were not breaking the cache.

earthly/earthly#3895

ast/Earthfile

@@ -1,6 +1,6 @@
 VERSION 0.8
 
-FROM github.com/earthly/earthly+base
+FROM ../+base
 
 # deps downloads and caches all dependencies for the ast package. When called
 # directly, go.mod and go.sum will be updated locally.

inputgraph/loader.go

@@ -41,21 +41,20 @@ type Stats struct {
 }
 
 type loader struct {
-	target           domain.Target
-	visited          map[string]struct{}
-	hasher           *hasher.Hasher
-	importsProcessed bool
-	hashCache        map[string][]byte
-	stats            *Stats
-	primaryTarget    bool
-	conslog          conslogging.ConsoleLogger
-	varCollection    *variables.Collection
-	features         *features.Features
-	isBaseTarget     bool
-	ci               bool
-	builtinArgs      variables.DefaultArgs
-	overridingVars   *variables.Scope
-	globalImports    map[string]domain.ImportTrackerVal
+	target         domain.Target
+	visited        map[string]struct{}
+	hasher         *hasher.Hasher
+	baseProcessed  bool
+	hashCache      map[string][]byte
+	stats          *Stats
+	primaryTarget  bool
+	conslog        conslogging.ConsoleLogger
+	varCollection  *variables.Collection
+	features       *features.Features
+	ci             bool
+	builtinArgs    variables.DefaultArgs
+	overridingVars *variables.Scope
+	globalImports  map[string]domain.ImportTrackerVal
 }
 
 func newLoader(ctx context.Context, opt HashOpt) *loader {
@@ -67,7 +66,6 @@ func newLoader(ctx context.Context, opt HashOpt) *loader {
 		target:         opt.Target,
 		visited:        map[string]struct{}{},
 		hasher:         h,
-		isBaseTarget:   opt.Target.Target == "base",
 		ci:             opt.CI,
 		builtinArgs:    opt.BuiltinArgs,
 		overridingVars: opt.OverridingVars,
@@ -375,7 +373,7 @@ func (l *loader) handleCommand(ctx context.Context, cmd spec.Command) error {
 	case command.Copy:
 		return l.handleCopy(ctx, cmd)
 	case command.Arg:
-		return l.handleArg(ctx, cmd)
+		return l.handleArg(ctx, cmd, false)
 	case command.FromDockerfile:
 		return l.handleFromDockerfile(ctx, cmd)
 	case command.Import:
@@ -387,14 +385,14 @@ func (l *loader) handleCommand(ctx context.Context, cmd spec.Command) error {
 	}
 }
 
-func (l *loader) handleImport(ctx context.Context, cmd spec.Command, global bool) error {
+func (l *loader) handleImport(ctx context.Context, cmd spec.Command, isBase bool) error {
 
 	var alias string
 	if len(cmd.Args) == 3 {
 		alias = cmd.Args[2]
 	}
 
-	err := l.varCollection.Imports().Add(cmd.Args[0], alias, global, false, false)
+	err := l.varCollection.Imports().Add(cmd.Args[0], alias, isBase, false, false)
 	if err != nil {
 		return wrapError(err, cmd.SourceLocation, "failed to add import")
 	}
@@ -421,8 +419,8 @@ func (l *loader) handleFromDockerfile(ctx context.Context, cmd spec.Command) err
 	return nil
 }
 
-func (l *loader) handleArg(ctx context.Context, cmd spec.Command) error {
-	opts, key, valueOrNil, err := flagutil.ParseArgArgs(ctx, cmd, l.isBaseTarget, l.features.ExplicitGlobal)
+func (l *loader) handleArg(ctx context.Context, cmd spec.Command, isBase bool) error {
+	opts, key, valueOrNil, err := flagutil.ParseArgArgs(ctx, cmd, isBase, l.features.ExplicitGlobal)
 	if err != nil {
 		return wrapError(err, cmd.SourceLocation, "failed to parse args")
 	}
@@ -821,7 +819,6 @@ func (l *loader) forTarget(ctx context.Context, target domain.Target, args []str
 		target:         target,
 		visited:        visited,
 		hasher:         hasher.New(),
-		isBaseTarget:   target.Target == "base",
 		ci:             l.ci,
 		builtinArgs:    l.builtinArgs,
 		overridingVars: overriding,
@@ -831,7 +828,7 @@ func (l *loader) forTarget(ctx context.Context, target domain.Target, args []str
 	}
 
 	if target.IsLocalInternal() {
-		ret.importsProcessed = true
+		ret.baseProcessed = true
 		ret.globalImports = l.varCollection.Imports().Global()
 	}
 
@@ -957,37 +954,46 @@ func (l *loader) load(ctx context.Context) ([]byte, error) {
 		l.hashVersion(*ef.Version)
 	}
 
-	// Ensure all IMPORT commands are processed.
-	if !l.importsProcessed {
+	// Ensure all "base" target commands are processed once.
+	if !l.baseProcessed {
 		for _, stmt := range ef.BaseRecipe {
-			if stmt.Command != nil && stmt.Command.Name == command.Import {
-				if err := l.handleImport(ctx, *stmt.Command, true); err != nil {
-					return nil, err
-				}
+			var err error
+			switch {
+			case stmt.Command == nil:
+				break
+			case stmt.Command.Name == command.Import:
+				err = l.handleImport(ctx, *stmt.Command, true)
+			case stmt.Command.Name == command.Arg:
+				err = l.handleArg(ctx, *stmt.Command, true)
+			case stmt.Command.Name == command.From:
+				err = l.handleFrom(ctx, *stmt.Command)
+			}
+			if err != nil {
+				return nil, err
 			}
 		}
 	}
 
-	var block spec.Block
+	isBase := l.target.Target == "base"
+
+	// Since "base" is always processed above, there's not need to revisit it here.
+	if !isBase {
+		var block spec.Block
 
-	if l.target.Target == "base" {
-		block = ef.BaseRecipe
-	} else {
 		for _, t := range ef.Targets {
 			if t.Name == l.target.Target {
 				block = t.Recipe
 				break
 			}
 		}
-	}
 
-	if block == nil {
-		return nil, fmt.Errorf("target %q not found", l.target.Target)
-	}
+		if block == nil {
+			return nil, fmt.Errorf("target %q not found", l.target.Target)
+		}
 
-	err = l.loadBlock(ctx, block)
-	if err != nil {
-		return nil, err
+		if err := l.loadBlock(ctx, block); err != nil {
+			return nil, err
+		}
 	}
 
 	v := l.hasher.GetHash()

tests/autoskip/Earthfile

@@ -33,6 +33,7 @@ test-all:
     BUILD +test-build-flag
     BUILD +test-flag-conflict
     BUILD +test-init-failure
+    BUILD +test-arg-change
 
 test-files:
     RUN echo hello > my-file
@@ -277,6 +278,18 @@ test-init-failure:
                                      --earthfile=build-flag.earth --target=+basic \
                                      --output_contains="Failed to initialize auto-skip database"
 
+test-arg-change:
+    # These 2 distinct versions are pinned to ensure a version argument change
+    # triggers auto-skip cache-busting.
+    ARG V1="3.18.6"
+    ARG V2="3.19.1"
+    RUN acbtest "$V1" != "$V2" # Ensure that the values are kept distinct.
+    COPY arg-change.earth Earthfile
+    RUN sed -i "s/<VERSION>/$V1/g" Earthfile
+    DO --pass-args tests+RUN_EARTHLY --extra_args="--auto-skip-db-path=test.db" --target=+expand-args-from --output_contains="VERSION_ID=$V1"
+    RUN sed -i "s/$V1/$V2/g" Earthfile
+    DO --pass-args tests+RUN_EARTHLY --extra_args="--auto-skip-db-path=test.db" --target=+expand-args-from --output_contains="VERSION_ID=$V2"
+
 RUN_EARTHLY_ARGS:
     FUNCTION
     ARG extra_args

tests/autoskip/arg-change.earth

@@ -0,0 +1,11 @@
+VERSION --build-auto-skip 0.8
+
+# Note: '<VERSION>' is replaced in tests/autoskip/Earthfile.
+ARG VERSION=<VERSION>
+FROM alpine:$VERSION
+
+parent:
+    RUN cat /etc/os-release
+
+expand-args-from:
+    BUILD --auto-skip +parent

util/deltautil/Earthfile

@@ -1,6 +1,6 @@
 VERSION 0.8
 
-FROM github.com/earthly/earthly+base
+FROM ../../+base
 
 # deps downloads and caches all dependencies for the deltautil package. When
 # called directly, go.mod and go.sum will be updated locally.