Change in flashrom[master]: Makefile: Fix building on AArch64 NixOS

[Pyry Kontio (Code Review)]

Pyry Kontio has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Makefile: Fix building on AArch64 NixOS

Signed-off-by: Pyry Kontio 
Change-Id: Iaa4477a71e758cf9ecad2c22f3b77bc6508a3510
---
M Makefile
1 file changed, 8 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/43140/1

diff --git a/Makefile b/Makefile
index 803529f..6a2abd1 100644
--- a/Makefile
+++ b/Makefile
@@ -106,7 +106,8 @@
 # IMPORTANT: The following line must be placed before TARGET_OS is ever used
 # (of course), but should come after any lines setting CC because the line
 # below uses CC itself.
-override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null | grep -v '^#' | grep '"' | cut -f 2 -d'"'))
+override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null \
+    | grep -v '^#' | grep '"' | tail -1 | cut -f 2 -d'"'))

ifeq ($(TARGET_OS), Darwin)
 override CPPFLAGS += -I/opt/local/include -I/usr/local/include
@@ -460,8 +461,10 @@
 # IMPORTANT: The following line must be placed before ARCH is ever used
 # (of course), but should come after any lines setting CC because the line
 # below uses CC itself.
-override ARCH := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E archtest.c 2>/dev/null | grep -v '^#' | grep '"' | cut -f 2 -d'"'))
-override ENDIAN := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E endiantest.c 2>/dev/null | grep -v '^#'))
+override ARCH := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E archtest.c 2>/dev/null \
+    | grep -v '^#' | grep '"' | tail -1 | cut -f 2 -d'"'))
+override ENDIAN := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E endiantest.c 2>/dev/null \
+    | grep -v '^#'))

# Disable the internal programmer on unsupported architectures (everything but x86 and mipsel)
 ifneq ($(ARCH)-little, $(filter $(ARCH),x86 mips)-$(ENDIAN))
@@ -1253,12 +1256,12 @@
    @printf "Target arch is "
    @# FreeBSD wc will output extraneous whitespace.
    @echo $(ARCH)|wc -w|grep -q '^[[:blank:]]*1[[:blank:]]*$$' ||	\
-		( echo "unknown. Aborting."; exit 1)
+		( echo "unknown ("$(ARCH)"). Aborting."; exit 1)
    @printf "%s\n" '$(ARCH)'
    @printf "Target OS is "
    @# FreeBSD wc will output extraneous whitespace.
    @echo $(TARGET_OS)|wc -w|grep -q '^[[:blank:]]*1[[:blank:]]*$$' ||	\
-		( echo "unknown. Aborting."; exit 1)
+		( echo "unknown ("$(TARGET_OS)"). Aborting."; exit 1)
    @printf "%s\n" '$(TARGET_OS)'
 ifeq ($(TARGET_OS), libpayload)
    @$(CC) --version 2>&1 | grep -q coreboot || \

[Paul Menzel (Code Review)]

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

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 1:

(1 comment)

Welcome to flashrom!

https://review.coreboot.org/c/flashrom/+/43140/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/43140/1//COMMIT_MSG@8 
PS1, Line 8: 
Please describe the problem, you are seeing in NixOS.

[Pyry Kontio (Code Review)]

Pyry Kontio has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/flashrom/+/43140/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/43140/1//COMMIT_MSG@8 
PS1, Line 8:
> Please describe the problem, you are seeing in NixOS.

The problem was originally described and resolved here: https://github.com/flashrom/flashrom/issues/125

Just to make sure; do you want me to add a short description to the commit message or describe it here?

Here's an amended draft commit:

commit e4483e165efb017912eeef0a1c15a91e2ed33ac1 (HEAD -> upstream_master)
Author: Pyry Kontio 
Date:   Mon Jul 6 12:57:35 2020 +0900

Makefile: Fix building on AArch64 NixOS

The parsing of the output of archtest.c produced an unexpected
    value on AArch64 NixOS. For example, the make variable ARCH was set to:

```
    bit outside of fd_set selected
    arm
    ```

This made the arch and OS checks fail.
    This commit addresses this case, making the parsing more robust.

Signed-off-by: Pyry Kontio 
    Change-Id: Iaa4477a71e758cf9ecad2c22f3b77bc6508a3510

Am I expected to force-push, replacing this commit?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/c/flashrom/+/43140/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/43140/1//COMMIT_MSG@8 
PS1, Line 8:
> The problem was originally described and resolved here: https://github. […]

You don't need to force-push, but pushing an amended commit to gerrit with the same Change-Id line will become a new "patchset" (version) of this change.

https://review.coreboot.org/c/flashrom/+/43140/1/Makefile 
File Makefile:

https://review.coreboot.org/c/flashrom/+/43140/1/Makefile@110 
PS1, Line 110: grep '"'
As mentioned on the GitHub issue, I'd prefer to improve the parsing step. Start by ignoring lines with "__attribute__", and maybe add a check with "wc" to exit early if more than one word is detected?

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/flashrom/+/43140/1/Makefile 
File Makefile:

https://review.coreboot.org/c/flashrom/+/43140/1/Makefile@110 
PS1, Line 110: grep '"'
> As mentioned on the GitHub issue, I'd prefer to improve the parsing step. […]

`gcc -E` on archtest.c, endianness.c, and os.h should all produce the result on the last line, so perhaps we can skip complicated grep'ing and just use `tail` and `cut` to extract the information we need?

[Pyry Kontio (Code Review)]

Hello build bot (Jenkins), David Hendricks, Angel Pons,

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

https://review.coreboot.org/c/flashrom/+/43140

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

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Makefile: Fix building on AArch64 NixOS

The parsing of the output of archtest.c produced an unexpected
value on AArch64 NixOS. For example, the make variable ARCH was set to:

```
bit outside of fd_set selected
arm
```

This made the arch and OS checks fail.
This commit addresses this case, making the parsing more robust.

Signed-off-by: Pyry Kontio 
Change-Id: Iaa4477a71e758cf9ecad2c22f3b77bc6508a3510
---
M Makefile
1 file changed, 8 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/43140/2

[Pyry Kontio (Code Review)]

Pyry Kontio has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 2:

Indeed, archtest.c, endianness.c and os.c all produce the result on the last line. I think simpler the better, so I agree with David. I updated the parsing bits.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 2: Code-Review+1

Something seems to be wrong, Jenkins says: syntax error.

[Pyry Kontio (Code Review)]

Pyry Kontio has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review+1
>
> Something seems to be wrong, Jenkins says: syntax error.

There was a latent bug with lack of escaping in debug_shell. Sent https://review.coreboot.org/c/flashrom/+/44104 to fix it. I thought it would be added to this thread as an additional commit, but apparently it created a new thread. (I know hardly anything about Gerrit; sorry about that.)

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2: Code-Review+1
> >
> > Something seems to be wrong, Jenkins says: syntax error.
>
> There was a latent bug with lack of escaping in debug_shell. Sent https://review.coreboot.org/c/flashrom/+/44104 to fix it. I thought it would be added to this thread as an additional commit, but apparently it created a new thread. (I know hardly anything about Gerrit; sorry about that.)

You should amend this commit. If you don't have it on your local repo anymore, there's a download button with multiple options in the Gerrit UI.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2: Code-Review+1
> > >
> > > Something seems to be wrong, Jenkins says: syntax error.
> >
> > There was a latent bug with lack of escaping in debug_shell. Sent https://review.coreboot.org/c/flashrom/+/44104 to fix it. I thought it would be added to this thread as an additional commit, but apparently it created a new thread. (I know hardly anything about Gerrit; sorry about that.)
>
> You should amend this commit. If you don't have it on your local repo anymore, there's a download button with multiple options in the Gerrit UI.

Or, if you have both commits on your local repo, just squash the changes into a single commit.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > > > Patch Set 2: Code-Review+1
> > > >
> > > > Something seems to be wrong, Jenkins says: syntax error.
> > >
> > > There was a latent bug with lack of escaping in debug_shell. Sent https://review.coreboot.org/c/flashrom/+/44104 to fix it. I thought it would be added to this thread as an additional commit, but apparently it created a new thread. (I know hardly anything about Gerrit; sorry about that.)
> >
> > You should amend this commit. If you don't have it on your local repo anymore, there's a download button with multiple options in the Gerrit UI.
>
> Or, if you have both commits on your local repo, just squash the changes into a single commit.

Last thing: the `Change-Id` line in the commit message is what Gerrit uses to associate commits to changes (like this one). If you squash, make sure the resulting commit message only contains one Change-Id line (the one from this change).

[Pyry Kontio (Code Review)]

Pyry Kontio has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > > > Patch Set 2:
> > > > > Patch Set 2: Code-Review+1
> > > > >
> > > > > Something seems to be wrong, Jenkins says: syntax error.
> > > >
> > > > There was a latent bug with lack of escaping in debug_shell. Sent https://review.coreboot.org/c/flashrom/+/44104 to fix it. I thought it would be added to this thread as an additional commit, but apparently it created a new thread. (I know hardly anything about Gerrit; sorry about that.)
> > >
> > > You should amend this commit. If you don't have it on your local repo anymore, there's a download button with multiple options in the Gerrit UI.
> >
> > Or, if you have both commits on your local repo, just squash the changes into a single commit.
>
> Last thing: the `Change-Id` line in the commit message is what Gerrit uses to associate commits to changes (like this one). If you squash, make sure the resulting commit message only contains one Change-Id line (the one from this change).

Ah, sorry! I figured that the escaping bugfix was kind of independent from these changes, that it would be cleaner to introduce as a separate commit. I'll squash them.

[Pyry Kontio (Code Review)]

Hello build bot (Jenkins), David Hendricks, Angel Pons,

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

https://review.coreboot.org/c/flashrom/+/43140

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

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Makefile: Fix building on AArch64 NixOS

The parsing of the output of archtest.c produced an unexpected
value on AArch64 NixOS. For example, the make variable ARCH was set to:

```
bit outside of fd_set selected
arm
```

This made the arch and OS checks fail.

This commit simplifies the parsing, making it more robust.

The C files archtest.c, endiantest.c and os.h used to set the
TARGET_OS, ARCH and ENDIAN variables, respectively, output
the result of the test as the final line, so just extracting
the final line and removing double quoting is enough.

This commit also fixes a bug with debug_shell lacking escaping
single quotes, which prevented using the single quote in the
debug_shell calls. It used to work by accident before this fix;
the line in the call happened to contain a balanced pair of double
quotes and lacked other characters that needed escaping, which
didn't break the debug_shell, but this was accidental and very
brittle.

Signed-off-by: Pyry Kontio 
Change-Id: Iaa4477a71e758cf9ecad2c22f3b77bc6508a3510
---
M Makefile
1 file changed, 10 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/43140/3

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 3: Code-Review+2

[Pyry Kontio (Code Review)]

Pyry Kontio has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 4:

Rebased against the newest master. It would be nice to get this merged soon.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/flashrom/+/43140/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/43140/1//COMMIT_MSG@8 
PS1, Line 8:
> You don't need to force-push, but pushing an amended commit to gerrit with the same Change-Id line w […]

Done

https://review.coreboot.org/c/flashrom/+/43140/1/Makefile 
File Makefile:

https://review.coreboot.org/c/flashrom/+/43140/1/Makefile@110 
PS1, Line 110: grep '"'
> `gcc -E` on archtest.c, endianness.c, and os. […]

Done

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/43140 )

Change subject: Makefile: Fix building on AArch64 NixOS
......................................................................

Makefile: Fix building on AArch64 NixOS

The parsing of the output of archtest.c produced an unexpected
value on AArch64 NixOS. For example, the make variable ARCH was set to:

```
bit outside of fd_set selected
arm
```

This made the arch and OS checks fail.

This commit simplifies the parsing, making it more robust.

The C files archtest.c, endiantest.c and os.h used to set the
TARGET_OS, ARCH and ENDIAN variables, respectively, output
the result of the test as the final line, so just extracting
the final line and removing double quoting is enough.

This commit also fixes a bug with debug_shell lacking escaping
single quotes, which prevented using the single quote in the
debug_shell calls. It used to work by accident before this fix;
the line in the call happened to contain a balanced pair of double
quotes and lacked other characters that needed escaping, which
didn't break the debug_shell, but this was accidental and very
brittle.

Signed-off-by: Pyry Kontio 
Change-Id: Iaa4477a71e758cf9ecad2c22f3b77bc6508a3510
Reviewed-on: https://review.coreboot.org/c/flashrom/+/43140
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M Makefile
1 file changed, 10 insertions(+), 6 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/Makefile b/Makefile
index f3f7717..e475cbd 100644
--- a/Makefile
+++ b/Makefile
@@ -83,7 +83,8 @@

# Provide an easy way to execute a command, print its output to stdout and capture any error message on stderr
 # in the build details file together with the original stdout output.
-debug_shell = $(shell export LC_ALL=C ; { echo 'exec: export LC_ALL=C ; { $(1) ; }' >&2;  { $(1) ; } | tee -a $(BUILD_DETAILS_FILE) ; echo >&2 ; } 2>>$(BUILD_DETAILS_FILE))
+debug_shell = $(shell export LC_ALL=C ; { echo 'exec: export LC_ALL=C ; { $(subst ',''',$(1)) ; }' >&2; \
+    { $(1) ; } | tee -a $(BUILD_DETAILS_FILE) ; echo >&2 ; } 2>>$(BUILD_DETAILS_FILE))

###############################################################################
 # General OS-specific settings.
@@ -106,7 +107,8 @@
 # IMPORTANT: The following line must be placed before TARGET_OS is ever used
 # (of course), but should come after any lines setting CC because the line
 # below uses CC itself.
-override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null | grep -v '^#' | grep '"' | cut -f 2 -d'"'))
+override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null \
+    | tail -1 | cut -f 2 -d'"'))

ifeq ($(TARGET_OS), Darwin)
 override CPPFLAGS += -I/opt/local/include -I/usr/local/include
@@ -490,8 +492,10 @@
 # IMPORTANT: The following line must be placed before ARCH is ever used
 # (of course), but should come after any lines setting CC because the line
 # below uses CC itself.
-override ARCH := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E archtest.c 2>/dev/null | grep -v '^#' | grep '"' | cut -f 2 -d'"'))
-override ENDIAN := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E endiantest.c 2>/dev/null | grep -v '^#'))
+override ARCH := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E archtest.c 2>/dev/null \
+    | tail -1 | cut -f 2 -d'"'))
+override ENDIAN := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E endiantest.c 2>/dev/null \
+    | tail -1))

# Disable the internal programmer on unsupported architectures (everything but x86 and mipsel)
 ifneq ($(ARCH)-little, $(filter $(ARCH),x86 mips)-$(ENDIAN))
@@ -1299,12 +1303,12 @@
    @printf "Target arch is "
    @# FreeBSD wc will output extraneous whitespace.
    @echo $(ARCH)|wc -w|grep -q '^[[:blank:]]*1[[:blank:]]*$$' ||	\
-		( echo "unknown. Aborting."; exit 1)
+		( echo "unknown ("$(ARCH)"). Aborting."; exit 1)
    @printf "%s\n" '$(ARCH)'
    @printf "Target OS is "
    @# FreeBSD wc will output extraneous whitespace.
    @echo $(TARGET_OS)|wc -w|grep -q '^[[:blank:]]*1[[:blank:]]*$$' ||	\
-		( echo "unknown. Aborting."; exit 1)
+		( echo "unknown ("$(TARGET_OS)"). Aborting."; exit 1)
    @printf "%s\n" '$(TARGET_OS)'
 ifeq ($(TARGET_OS), libpayload)
    @$(CC) --version 2>&1 | grep -q coreboot || \