Change in coreboot[master]: util/abuild: Cleanup the missing_arches check

[Raul Rangel (Code Review)]

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34241 )

Change subject: util/abuild: Cleanup the missing_arches check
......................................................................

util/abuild: Cleanup the missing_arches check

This change adds the following improvements:
* Easier to read.
* Checks to see if .xcompile is complete. I chose to abort the whole
  script since it would fail later anyway.
* Checks the make return code. This will catch if .xcompile is missing.

BUG=b:112267918
TEST=Modified my .xcompile and ran abuild and verified that
missing_arches got set correctly. Also deleted .xcompile and verified
there was a failure.

Change-Id: I7604d431f398fc0c80a857a0c7c21e164004cc99
Signed-off-by: Raul E Rangel 
---
M util/abuild/abuild
1 file changed, 17 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/34241/1

diff --git a/util/abuild/abuild b/util/abuild/abuild
index abfedba..3a297c3 100755
--- a/util/abuild/abuild
+++ b/util/abuild/abuild
@@ -462,10 +462,25 @@
    	return
    fi

+	local required_arches
+
    required_arches=$(grep -E "^CONFIG_ARCH_(BOOTBLOCK|R.MSTAGE|VERSTAGE)" "$TARGET/${BUILD_NAME}/config.build" | \
    		sed "s,^CONFIG_ARCH_[^_]*_([^=]*)=.*$,\1," |sort -u |tr 'A-Z\n\r' 'a-z  ')
-	# shellcheck disable=SC2016,SC2059
-	missing_arches=$(printf 'include .xcompile\nall: ; @echo $(foreach arch,'"$required_arches"',$(if $(filter $(arch),$(SUBARCH_SUPPORTED)),,$(arch)))' | $MAKE --no-print-directory -f -)
+
+	missing_arches="$($MAKE -f - REQUIRED_ARCHES="$required_arches" <<'EOF'
+include .xcompile
+.PHONY: missing_arches
+missing_arches:
+	$(if $(XCOMPILE_COMPLETE),,$(error .xcompile is invalid.))
+	@echo $(foreach arch,$(REQUIRED_ARCHES),$(if $(filter $(arch),$(SUBARCH_SUPPORTED)),,$(arch)))
+EOF
+)"
+	# shellcheck disable=SC2181
+	if [[ $? -ne 0 ]]; then
+		echo "Calculating missing_arches failed" >&2
+		exit 1
+	fi
+
    if [ -n "$missing_arches" ]; then
    	printf "skipping %s because we're missing compilers for (%s)\n" "$BUILD_NAME" "$missing_arches"
    	return

[Raul Rangel (Code Review)]

Hello Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34241

to look at the new patch set (#2).

Change subject: util/abuild: Cleanup the missing_arches check
......................................................................

util/abuild: Cleanup the missing_arches check

This change adds the following improvements:
* Easier to read.
* Checks to see if .xcompile is complete.
* Checks the make return code. This will catch if .xcompile is missing.

BUG=b:112267918
TEST=Modified my .xcompile and ran abuild and verified that
missing_arches got set correctly. Also deleted .xcompile and verified
there was a failure.

Change-Id: I7604d431f398fc0c80a857a0c7c21e164004cc99
Signed-off-by: Raul E Rangel 
---
M util/abuild/abuild
1 file changed, 18 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/34241/2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34241 )

Change subject: util/abuild: Cleanup the missing_arches check
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34241/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34241/2//COMMIT_MSG@7 
PS2, Line 7: Cleanup
The verb is spelled with a space.

Change in coreboot[master]: util/abuild: Clean up the missing_arches check

[Raul Rangel (Code Review)]

Hello Julius Werner, Paul Menzel, build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34241

to look at the new patch set (#3).

Change subject: util/abuild: Clean up the missing_arches check
......................................................................

util/abuild: Clean up the missing_arches check

This change adds the following improvements:
* Easier to read.
* Checks to see if .xcompile is complete.
* Checks the make return code. This will catch if .xcompile is missing.

BUG=b:112267918
TEST=Modified my .xcompile and ran abuild and verified that
missing_arches got set correctly. Also deleted .xcompile and verified
there was a failure.

Change-Id: I7604d431f398fc0c80a857a0c7c21e164004cc99
Signed-off-by: Raul E Rangel 
---
M util/abuild/abuild
1 file changed, 18 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/34241/3

Change in coreboot[master]: util/abuild: Clean up the missing_arches check

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34241 )

Change subject: util/abuild: Clean up the missing_arches check
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34241/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34241/2//COMMIT_MSG@7 
PS2, Line 7: Cleanup
> The verb is spelled with a space.

I learned something new today.

Change in coreboot[master]: util/abuild: Clean up the missing_arches check

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34241 )

Change subject: util/abuild: Clean up the missing_arches check
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: util/abuild: Clean up the missing_arches check

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34241 )

Change subject: util/abuild: Clean up the missing_arches check
......................................................................

util/abuild: Clean up the missing_arches check

This change adds the following improvements:
* Easier to read.
* Checks to see if .xcompile is complete.
* Checks the make return code. This will catch if .xcompile is missing.

BUG=b:112267918
TEST=Modified my .xcompile and ran abuild and verified that
missing_arches got set correctly. Also deleted .xcompile and verified
there was a failure.

Change-Id: I7604d431f398fc0c80a857a0c7c21e164004cc99
Signed-off-by: Raul E Rangel 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34241
Tested-by: build bot (Jenkins) 
Reviewed-by: Martin Roth 
---
M util/abuild/abuild
1 file changed, 18 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Martin Roth: Looks good to me, approved

diff --git a/util/abuild/abuild b/util/abuild/abuild
index 03be0d4..ef4e46b 100755
--- a/util/abuild/abuild
+++ b/util/abuild/abuild
@@ -462,10 +462,26 @@
    	return
    fi

+	local required_arches
+
    required_arches=$(grep -E "^CONFIG_ARCH_(BOOTBLOCK|R.MSTAGE|VERSTAGE)" "$TARGET/${BUILD_NAME}/config.build" | \
    		sed "s,^CONFIG_ARCH_[^_]*_([^=]*)=.*$,\1," |sort -u |tr 'A-Z\n\r' 'a-z  ')
-	# shellcheck disable=SC2016,SC2059
-	missing_arches=$(printf 'include .xcompile\nall: ; @echo $(foreach arch,'"$required_arches"',$(if $(filter $(arch),$(SUBARCH_SUPPORTED)),,$(arch)))' | $MAKE --no-print-directory -f -)
+
+	missing_arches="$($MAKE --no-print-directory -f - \
+		REQUIRED_ARCHES="$required_arches" <<'EOF'
+include .xcompile
+.PHONY: missing_arches
+missing_arches:
+	$(if $(XCOMPILE_COMPLETE),,$(error .xcompile is invalid.))
+	@echo $(foreach arch,$(REQUIRED_ARCHES),$(if $(filter $(arch),$(SUBARCH_SUPPORTED)),,$(arch)))
+EOF
+)"
+	# shellcheck disable=SC2181
+	if [[ $? -ne 0 ]]; then
+		echo "Calculating missing_arches failed" >&2
+		exit 1
+	fi
+
    if [ -n "$missing_arches" ]; then
    	printf "skipping %s because we're missing compilers for (%s)\n" "$BUILD_NAME" "$missing_arches"
    	return

Change in coreboot[master]: util/abuild: Clean up the missing_arches check

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34241 )

Change subject: util/abuild: Clean up the missing_arches check
......................................................................

Patch Set 4:

With this patch abuild always fails for me with

/tmp/GmkR0sEo:1: .xcompile: No such file or directory
 make: *** No rule to make target '.xcompile'.  Stop.
 Calculating missing_arches failed

Was that intended? I normally only use abuild to test builds and never run 'make' directly in my coreboot checkout, so I don't have an .xcompile in there. Normally abuild just generates its own .xcompile in coreboot-builds/<board>/xcompile.build and uses that. Can't we make whatever this was trying to do do it that way too? I'd like abuild to work on its own in a clean checkout.

Change in coreboot[master]: util/abuild: Clean up the missing_arches check

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34241 )

Change subject: util/abuild: Clean up the missing_arches check
......................................................................

Patch Set 4:
> Patch Set 4:
>
> With this patch abuild always fails for me with
>
> /tmp/GmkR0sEo:1: .xcompile: No such file or directory
>  make: *** No rule to make target '.xcompile'.  Stop.
>  Calculating missing_arches failed
>
> Was that intended? I normally only use abuild to test builds and never run 'make' directly in my coreboot checkout, so I don't have an .xcompile in there. Normally abuild just generates its own .xcompile in coreboot-builds/<board>/xcompile.build and uses that. Can't we make whatever this was trying to do do it that way too? I'd like abuild to work on its own in a clean checkout.

ah, looks like when I tested it I had a stale .xcompile in my local directory. The reason it fails now is because I check the return code from make. Previously it would have been failing silently.

The correct fix is here: https://review.coreboot.org/c/coreboot/+/28101/6/util/abuild/abuild

In the short term we could remove the "Calculating missing_arches failed" check so it keeps silently failing...