LET/SET should block parallel commands that come after it (#3998)

Address #3997

CHANGELOG.md

@@ -8,6 +8,10 @@ All notable changes to [Earthly](https://github.com/earthly/earthly) will be doc
 
 - New experimental wildcard-based copy, e.g. `COPY ./services/*+artifact/* .` which would invoke `COPY` for `./services/foo+artifact`, and `./services/bar+artifact` (assuming two services foo and bar, both having a `artifact` target in their respective Earthfile). Enable with the `VERSION --wildcard-copy` feature flag. [#3966](https://github.com/earthly/earthly/issues/3966).
 
+### Fixed
+
+- Make `LET`/`SET` commands block parallel commands such as `BUILD` until the former are processed, similar to the behavior of `ARG`. [#3997](https://github.com/earthly/earthly/issues/3997)
+
 ## v0.8.7 - 2024-04-03
 
 ### Added

earthfile2llb/converter.go

@@ -1467,7 +1467,7 @@ func (c *Converter) UpdateArg(ctx context.Context, argKey string, argValue strin
 		pncvf = c.processNonConstantBuildArgFunc(ctx)
 	}
 
-	err := c.varCollection.UpdateVar(argKey, argValue, pncvf, isBase)
+	err := c.varCollection.UpdateVar(argKey, argValue, pncvf)
 	if err != nil {
 		return err
 	}

earthfile2llb/interpreter.go

@@ -113,9 +113,9 @@ func (i *Interpreter) handleTarget(ctx context.Context, t spec.Target) error {
 }
 
 func (i *Interpreter) handleBlock(ctx context.Context, b spec.Block) error {
-	prevWasArg := true // not exactly true, but makes the logic easier
+	prevWasArgLike := true // not exactly true, but makes the logic easier
 	for index, stmt := range b {
-		if i.parallelConversion && prevWasArg {
+		if i.parallelConversion && prevWasArgLike {
 			err := i.handleBlockParallel(ctx, b, index)
 			if err != nil {
 				return err
@@ -125,7 +125,8 @@ func (i *Interpreter) handleBlock(ctx context.Context, b spec.Block) error {
 		if err != nil {
 			return err
 		}
-		prevWasArg = (stmt.Command != nil && stmt.Command.Name == "ARG")
+		prevWasArgLike = i.isArgLike(stmt.Command)
+
 	}
 	return nil
 }
@@ -141,7 +142,7 @@ func (i *Interpreter) handleBlockParallel(ctx context.Context, b spec.Block, sta
 		stmt := b[index]
 		if stmt.Command != nil {
 			switch stmt.Command.Name {
-			case command.Arg, command.Locally, command.From, command.FromDockerfile:
+			case command.Arg, command.Locally, command.From, command.FromDockerfile, command.Let, command.Set:
 				// Cannot do any further parallel builds - these commands need to be
 				// executed to ensure that they don't impact the outcome. As such,
 				// commands following these cannot be executed preemptively.
@@ -2158,6 +2159,20 @@ func (i *Interpreter) expandArgs(ctx context.Context, word string, keepPlusEscap
 	return unescapeSlashPlus(ret), nil
 }
 
+// isArgLike returns true if the command is ARG/LET/SET
+func (i *Interpreter) isArgLike(cmd *spec.Command) bool {
+	if cmd == nil {
+		return false
+	}
+
+	switch cmd.Name {
+	case command.Arg, command.Let, command.Set:
+		return true
+	}
+
+	return false
+}
+
 func escapeSlashPlus(str string) string {
 	// TODO: This is not entirely correct in a string like "\\\\+".
 	return strings.ReplaceAll(str, "\\+", "\\\\+")

tests/Earthfile

@@ -147,6 +147,7 @@ ga-no-qemu-group5:
     BUILD +test-aws-flag-envs
     BUILD +test-aws-flag-configs
     BUILD +test-aws-flag-none
+    BUILD +no-let-set-block-parallel
 
     # Forcing the implicit global wait/end block, causes some tests, which rely
     # on the ability to have two different targets issue the same SAVE IMAGE tag name
@@ -1699,16 +1700,20 @@ test-visited-upfront-hash-collection:
   DO +RUN_EARTHLY \
       --earthfile=visited-upfront-hash-collection.earth \
       --target=+parallel
-  RUN order=$(cat earthly.output | grep -Eo "### [[:digit:]]+"  | cut -d' ' -f2- | tr '\n' ' ' | tr -d '[:space:]'); \
-      echo "order: $order"; \
-      # This regex matches successions of number trios like: 222111444333555, happening when the builds didn't run in parallel
-      match=$(echo $order | sed -nr '/^([[:digit:]])\1\1([[:digit:]])\2\2([[:digit:]])\3\3([[:digit:]])\4\4([[:digit:]])\5\5$/p'); \
-      if [[ $match ]]; then \
-        echo "no parallelization observed"; \
-        exit 1; \
-      else \
-         echo "parallelization checked"; \
-      fi
+  DO +TEST_PARALLELIZATION
+  DO +RUN_EARTHLY \
+      --earthfile=visited-upfront-hash-collection.earth \
+      --target=+parallel-dynamic-arg
+  DO +TEST_PARALLELIZATION
+  DO +RUN_EARTHLY \
+      --earthfile=visited-upfront-hash-collection.earth \
+      --target=+parallel-dynamic-let
+  DO +TEST_PARALLELIZATION
+  DO +RUN_EARTHLY \
+      --earthfile=visited-upfront-hash-collection.earth \
+      --target=+parallel-dynamic-set
+  DO +TEST_PARALLELIZATION
+
 
 test-exec-stats:
   DO +RUN_EARTHLY \
@@ -1760,6 +1765,19 @@ earthly --config \$earthly_config +test 2>&1 | tee output.txt;
     DO +RUN_EARTHLY --exec_cmd=/tmp/run-earthly-with-bad-ssh-cipher
     RUN cat output.txt | acbgrep "Unknown cipher type 'badcipher'"
 
+TEST_PARALLELIZATION:
+  FUNCTION
+  RUN order=$(cat earthly.output | grep -Eo "### [[:digit:]]+"  | cut -d' ' -f2- | tr '\n' ' ' | tr -d '[:space:]'); \
+      echo "order: $order"; \
+      # This regex matches successions of number trios like: 222111444333555, happening when the builds didn't run in parallel
+      match=$(echo $order | sed -nr '/^([[:digit:]])\1\1([[:digit:]])\2\2([[:digit:]])\3\3([[:digit:]])\4\4([[:digit:]])\5\5$/p'); \
+      if [[ $match ]]; then \
+        echo "no parallelization observed"; \
+        exit 1; \
+      else \
+         echo "parallelization checked"; \
+      fi
+
 RUN_EARTHLY:
     FUNCTION
     ARG earthfile=

tests/visited-upfront-hash-collection.earth

@@ -9,6 +9,31 @@ parallel:
   BUILD +test-executor --INDEX=4
   BUILD +test-executor --INDEX=5
 
+parallel-dynamic-arg:
+  ARG target=+test-executor
+  BUILD $target --INDEX=1
+  BUILD $target --INDEX=2
+  BUILD $target --INDEX=3
+  BUILD $target --INDEX=4
+  BUILD $target --INDEX=5
+
+parallel-dynamic-let:
+  LET target=+test-executor
+  BUILD $target --INDEX=1
+  BUILD $target --INDEX=2
+  BUILD $target --INDEX=3
+  BUILD $target --INDEX=4
+  BUILD $target --INDEX=5
+
+parallel-dynamic-set:
+  LET target=""
+  SET target=+test-executor
+  BUILD $target --INDEX=1
+  BUILD $target --INDEX=2
+  BUILD $target --INDEX=3
+  BUILD $target --INDEX=4
+  BUILD $target --INDEX=5
+
 test-executor:
   ARG INDEX
   RUN --no-cache for i in $(seq 1 3); do  \

variables/collection.go

@@ -387,7 +387,7 @@ func (c *Collection) DeclareEnv(name string, value string) {
 // value of the variable, regardless of where the value was previously defined.
 //
 // It returns ErrVarNotFound if the variable was not found.
-func (c *Collection) UpdateVar(name, value string, pncvf ProcessNonConstantVariableFunc, isBase bool) (retErr error) {
+func (c *Collection) UpdateVar(name, value string, pncvf ProcessNonConstantVariableFunc) (retErr error) {
 	defer func() {
 		if retErr == nil {
 			c.effectiveCache = nil