core: reverse the argument order in check-host-cmake

PATCH New Carlos Santos 3 participants 10 messages May 7, 2017 – Jun 24, 2017

Share & tools

Carlos Santos

author

2017-05-07 04:32 UTC #0 lore raw

PATCH v2 1/4 core: reverse the argument order in check-host-cmake

Pass the minimal version before the program name. In a later change the script will become able to test a list of candidates.

Signed-off-by Carlos Santos <[email protected]>

---
 support/dependencies/check-host-cmake.mk | 2 +-
 support/dependencies/check-host-cmake.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

support/dependencies/check-host-cmake.mk +1 -1

index 8002278..914327f 100644
@@ -12,7 +12,7 @@ BR2_CMAKE_VERSION_MIN = 3.1
 
 BR2_CMAKE ?= cmake
 ifeq ($(call suitable-host-package,cmake,\
-	$(BR2_CMAKE) $(BR2_CMAKE_VERSION_MIN)),)
+	$(BR2_CMAKE_VERSION_MIN) $(BR2_CMAKE)),)
 BR2_CMAKE = $(HOST_DIR)/usr/bin/cmake
 BR2_CMAKE_HOST_DEPENDENCY = host-cmake
 endif

support/dependencies/check-host-cmake.sh +2 -2

index 9b63b06..73bd4ed 100755
@@ -1,7 +1,7 @@
 #!/bin/sh
 
-candidate="${1}"
-version_min="${2}"
+version_min="${1}"
+candidate="${2}"
 
 major_min="${version_min%.*}"
 minor_min="${version_min#*.}"

Signature · 1 line

2.7.4

Carlos Santos

author

2017-05-07 04:32 UTC #1 lore raw

PATCH v2 2/4 core: allow check-host-cmake.sh to try several candidates

This is useful on CentOS 7 whose "cmake" package provides cmake 2.8.12, which is too old, but the "cmake3" package (from EPEL) provides version 3.6.3, which is satisfactory. Examples:

    $ sh support/dependencies/check-host-cmake.sh 2.8 cmake cmake3
    /usr/bin/cmake

    $ sh support/dependencies/check-host-cmake.sh 3.1 cmake cmake3
    /usr/bin/cmake3

    $ sh support/dependencies/check-host-cmake.sh 3.8 cmake cmake3
    (nothing)

Signed-off-by Carlos Santos <[email protected]>

---
 support/dependencies/check-host-cmake.sh | 76 +++++++++++++++++---------------
 1 file changed, 41 insertions(+), 35 deletions(-)

support/dependencies/check-host-cmake.sh +41 ~~-35~~

index 73bd4ed..fadeae9 100755
@@ -1,39 +1,45 @@
 #!/bin/sh
 
-version_min="${1}"
-candidate="${2}"
-
-major_min="${version_min%.*}"
-minor_min="${version_min#*.}"
-
-cmake=`which ${candidate}`
-if [ ! -x "${cmake}" ]; then
-    # echo nothing: no suitable cmake found
-    exit 1
-fi
-
-# Extract version X.Y from versions in the form X.Y or X.Y.Z
-# with X, Y and Z numbers with one or more digits each, e.g.
-#   3.2     -> 3.2
-#   3.2.3   -> 3.2
-#   3.2.42  -> 3.2
-#   3.10    -> 3.10
-#   3.10.4  -> 3.10
-#   3.10.42 -> 3.10
-version="$(${cmake} --version \
-           |sed -r -e '/.* ([[:digit:]]+\.[[:digit:]]+).*$/!d;' \
-                   -e 's//\1/'
-          )"
-major="${version%.*}"
-minor="${version#*.}"
-
-if [ ${major} -gt ${major_min} ]; then
-    echo "${cmake}"
-else
-    if [ ${major} -eq ${major_min} -a ${minor} -ge ${minor_min} ]; then
+# prevent shift error
+[ $# -lt 2 ] && exit 1
+
+major_min="${1%.*}"
+minor_min="${1#*.}"
+
+shift
+
+for candidate; do
+
+    # Try to locate the candidate. Discard it if not located.
+    cmake=`which "${candidate}" 2>/dev/null`
+    [ -n "${cmake}" ] || continue
+
+    # Extract version X.Y from versions in the form X.Y or X.Y.Z
+    # with X, Y and Z numbers with one or more digits each, e.g.
+    #   3.2     -> 3.2
+    #   3.2.3   -> 3.2
+    #   3.2.42  -> 3.2
+    #   3.10    -> 3.10
+    #   3.10.4  -> 3.10
+    #   3.10.42 -> 3.10
+    # Discard the candidate if no version can be obtained
+    version="$(${cmake} --version \
+               |sed -r -e '/.* ([[:digit:]]+\.[[:digit:]]+).*$/!d;' \
+                       -e 's//\1/'
+              )"
+    [ -n "${version}" ] || continue
+
+    major="${version%.*}"
+    minor="${version#*.}"
+
+    if [ ${major} -gt ${major_min} ]; then
         echo "${cmake}"
-    else
-        # echo nothing: no suitable cmake found
-        exit 1
+        exit
+    elif [ ${major} -eq ${major_min} -a ${minor} -ge ${minor_min} ]; then
+        echo "${cmake}"
+        exit
     fi
-fi
+done
+
+# echo nothing: no suitable cmake found
+exit 1

Signature · 1 line

2.7.4

Yann E. MORIN

2017-05-07 09:47 UTC #2 lore raw

PATCH v2 2/4 core: allow check-host-cmake.sh to try several candidates

Carlos, All,

On 2017-05-07 01:32 -0300, Carlos Santos spake thusly:

Quoted 7 lines

This is useful on CentOS 7 whose "cmake" package provides cmake 2.8.12, which is too old, but the "cmake3" package (from EPEL) provides version 3.6.3, which is satisfactory. Examples:
    $ sh support/dependencies/check-host-cmake.sh 2.8 cmake cmake3
    /usr/bin/cmake

Quoted 7 lines

    $ sh support/dependencies/check-host-cmake.sh 3.1 cmake cmake3
    /usr/bin/cmake3

    $ sh support/dependencies/check-host-cmake.sh 3.8 cmake cmake3
    (nothing)

Signed-off-by Carlos Santos <[email protected]>

Acked-by "Yann E. MORIN" <[email protected]>

However, there is a small nit-pick I'd like to point out...

[--SNIP--]

Quoted 10 lines

support/dependencies/check-host-cmake.sh +10 -0

+for candidate; do
+
+    # Try to locate the candidate. Discard it if not located.
+    cmake=`which "${candidate}" 2>/dev/null`
+    [ -n "${cmake}" ] || continue
+
+    # Extract version X.Y from versions in the form X.Y or X.Y.Z
+    # with X, Y and Z numbers with one or more digits each, e.g.
+    #   3.2     -> 3.2
+    #   3.2.3   -> 3.2

support/dependencies/check-host-cmake.sh line 30

Quoted 10 lines

support/dependencies/check-host-cmake.sh +10 -0

+    #   3.2.42  -> 3.2
+    #   3.10    -> 3.10
+    #   3.10.4  -> 3.10
+    #   3.10.42 -> 3.10
+    # Discard the candidate if no version can be obtained
+    version="$(${cmake} --version \
+               |sed -r -e '/.* ([[:digit:]]+\.[[:digit:]]+).*$/!d;' \
+                       -e 's//\1/'
+              )"
+    [ -n "${version}" ] || continue

... here: the check that version is not empty is new, and semantically it looks like it should have been in a spearate patch (prossibly before that one).

Unless it is the act of testing multiple candidates that introduces a case where the version is empty, but I fil to see how that would be.

In any case, don't re-spin just for that.

Regards,
Yann E. MORIN.

Quoted 21 lines

support/dependencies/check-host-cmake.sh +12 -4

+    major="${version%.*}"
+    minor="${version#*.}"
+
+    if [ ${major} -gt ${major_min} ]; then
         echo "${cmake}"
-    else
-        # echo nothing: no suitable cmake found
-        exit 1
+        exit
+    elif [ ${major} -eq ${major_min} -a ${minor} -ge ${minor_min} ]; then
+        echo "${cmake}"
+        exit
     fi
-fi
+done
+
+# echo nothing: no suitable cmake found
+exit 1

Signature · 1 line

2.7.4

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Carlos Santos

author

2017-05-07 15:28 UTC #3 lore raw

PATCH v2 2/4 core: allow check-host-cmake.sh to try several candidates

Quoted 7 lines

From: "Yann E. MORIN" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org
Sent: Sunday, May 7, 2017 6:47:06 AM
Subject: Re: [PATCH v2 2/4] core: allow check-host-cmake.sh to try several candidates
Carlos, All,

---8<---

Quoted 7 lines

support/dependencies/check-host-cmake.sh line 30

Quoted 6 lines

support/dependencies/check-host-cmake.sh +6 -0

+    # Discard the candidate if no version can be obtained
+    version="$(${cmake} --version \
+               |sed -r -e '/.* ([[:digit:]]+\.[[:digit:]]+).*$/!d;' \
+                       -e 's//\1/'
+              )"
+    [ -n "${version}" ] || continue

Quoted 6 lines

... here: the check that version is not empty is new, and semantically it looks like it should have been in a spearate patch (prossibly before that one).
Unless it is the act of testing multiple candidates that introduces a case where the version is empty, but I fil to see how that would be.

The problem here is that if $version is empty both $major and $minor become empty too and the test below emits an annoying error message. Example (commenting the '[ -n "${version}" ] || continue' line):

    $ sh check-host-cmake.sh 3.1 python
    Python 2.7.12
    check-host-cmake.sh: 35: [: -gt: unexpected operator
    check-host-cmake.sh: 38: [: -eq: unexpected operator

Raising even more the paranoia level, the script is also fragile in other aspects, e.g

    $ bash --version
    GNU bash, version 4.3.46(1)-release (x86_64-pc-linux-gnu)
    ---8<---
    $  sh check-host-cmake.sh 3.1 bash
    /bin/bash
    $ cat --version
    cat (GNU coreutils) 8.25
    ---8<---
    $ sh check-host-cmake.sh 3.1 cat
    /bin/cat

It is so fragile that I'm tempted to replace the regex by a stricter one. Example:

    $ cmake --version | sed -n -r -e 's/^cmake version ([[:digit:]]+\.[[:digit:]]+).*$/\1/p'
    2.8
    $ cmake3 --version | sed -n -r -e 's/^cmake version ([[:digit:]]+\.[[:digit:]]+).*$/\1/p'
    3.6

Signature · 4 lines

Carlos Santos (Casantos) - DATACOM, P&D
?The greatest triumph that modern PR can offer is the transcendent 
success of having your words and actions judged by your reputation, 
rather than the other way about.? ? Christopher Hitchens

Yann E. MORIN

2017-05-07 16:11 UTC #4 lore raw

PATCH v2 2/4 core: allow check-host-cmake.sh to try several candidates

Carlos, All,

On 2017-05-07 12:28 -0300, Carlos Santos spake thusly:

Quoted 22 lines

Quoted 7 lines
From: "Yann E. MORIN" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org
Sent: Sunday, May 7, 2017 6:47:06 AM
Subject: Re: [PATCH v2 2/4] core: allow check-host-cmake.sh to try several candidates
Carlos, All,
---8<---
Quoted 7 lines
support/dependencies/check-host-cmake.sh line 30
Quoted 6 lines
support/dependencies/check-host-cmake.sh +6 -0
+    # Discard the candidate if no version can be obtained
+    version="$(${cmake} --version \
+               |sed -r -e '/.* ([[:digit:]]+\.[[:digit:]]+).*$/!d;' \
+                       -e 's//\1/'
+              )"
+    [ -n "${version}" ] || continue
Quoted 6 lines
... here: the check that version is not empty is new, and semantically it looks like it should have been in a spearate patch (prossibly before that one).
Unless it is the act of testing multiple candidates that introduces a case where the version is empty, but I fil to see how that would be.

Quoted 8 lines

The problem here is that if $version is empty both $major and $minor become empty too and the test below emits an annoying error message. Example (commenting the '[ -n "${version}" ] || continue' line):
    $ sh check-host-cmake.sh 3.1 python
    Python 2.7.12
    check-host-cmake.sh: 35: [: -gt: unexpected operator
    check-host-cmake.sh: 38: [: -eq: unexpected operator

Well, this can only happen in one condition: the 'cmake' (or 'cmake3') executable is in fact not CMake, which would be an excessively stupid situation...

Quoted 13 lines

Raising even more the paranoia level, the script is also fragile in other aspects, e.g

    $ bash --version
    GNU bash, version 4.3.46(1)-release (x86_64-pc-linux-gnu)
    ---8<---
    $  sh check-host-cmake.sh 3.1 bash
    /bin/bash
    $ cat --version
    cat (GNU coreutils) 8.25
    ---8<---
    $ sh check-host-cmake.sh 3.1 cat
    /bin/cat

YUes, if you call the script manually, which it is not intended to be. Remeber that the script is only called if the user did not explicitly pass BR2_CMAKE, and its arguments are always the minimum version and the candidates, which are always 'cmake' and 'cmake3'.

Quoted 7 lines

It is so fragile that I'm tempted to replace the regex by a stricter one. Example:

    $ cmake --version | sed -n -r -e 's/^cmake version ([[:digit:]]+\.[[:digit:]]+).*$/\1/p'
    2.8
    $ cmake3 --version | sed -n -r -e 's/^cmake version ([[:digit:]]+\.[[:digit:]]+).*$/\1/p'
    3.6

I'm not against having a stricter regex, but remember what you are trying to protect against... And I personally don't think the cases you point at would ever happen, unless one of the candidates is in fact not CMake.

Regards,
Yann E. MORIN.

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Carlos Santos

author

2017-05-08 17:14 UTC #5 lore raw

PATCH v2 2/4 core: allow check-host-cmake.sh to try several candidates

Quoted 6 lines

From: "Yann E. MORIN" <[email protected]>
To: "Carlos Santos" <[email protected]>
Cc: buildroot at buildroot.org
Sent: Sunday, May 7, 2017 1:11:32 PM
Subject: Re: [PATCH v2 2/4] core: allow check-host-cmake.sh to try several candidates

Quoted 8 lines

Quoted 4 lines
    $ sh check-host-cmake.sh 3.1 python
    Python 2.7.12
    check-host-cmake.sh: 35: [: -gt: unexpected operator
    check-host-cmake.sh: 38: [: -eq: unexpected operator
Well, this can only happen in one condition: the 'cmake' (or 'cmake3') executable is in fact not CMake, which would be an excessively stupid situation...

---8<---

Quoted 5 lines

I'm not against having a stricter regex, but remember what you are trying to protect against... And I personally don't think the cases you point at would ever happen, unless one of the candidates is in fact not CMake.

OK, let's keep the paranoia at its current level. :-)

Signature · 4 lines

Carlos Santos (Casantos) - DATACOM, P&D
?The greatest triumph that modern PR can offer is the transcendent 
success of having your words and actions judged by your reputation, 
rather than the other way about.? ? Christopher Hitchens

Carlos Santos

author

2017-05-07 04:32 UTC #6 lore raw

PATCH v2 3/4 core: allow having a list of "cmake" candidates

In reply to Carlos Santos

Add the BR2_CMAKE_CANDIDATES variable, containing a list of candidates to check and use as BR2_CMAKE, if possible.

This allows using "cmake3" on CentOS 7, whose default cmake corresponds to version 2.8.12. Example:

    $ make BR2_CMAKE_CANDIDATES="cmake cmake3"

Signed-off-by Carlos Santos <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

---
 support/dependencies/check-host-cmake.mk | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

support/dependencies/check-host-cmake.mk +4 -3

index 914327f..ef080ff 100644
@@ -10,9 +10,10 @@
 #
 BR2_CMAKE_VERSION_MIN = 3.1
 
-BR2_CMAKE ?= cmake
-ifeq ($(call suitable-host-package,cmake,\
-	$(BR2_CMAKE_VERSION_MIN) $(BR2_CMAKE)),)
+BR2_CMAKE_CANDIDATES ?= cmake
+BR2_CMAKE ?= $(call suitable-host-package,cmake,\
+	$(BR2_CMAKE_VERSION_MIN) $(BR2_CMAKE_CANDIDATES))
+ifeq ($(BR2_CMAKE),)
 BR2_CMAKE = $(HOST_DIR)/usr/bin/cmake
 BR2_CMAKE_HOST_DEPENDENCY = host-cmake
 endif

Signature · 1 line

2.7.4

Carlos Santos

author

2017-05-07 04:32 UTC #7 lore raw

PATCH v2 4/4 core: add "cmake3" to the list of cmake candidates

In reply to Carlos Santos

This is useful on CentOS 7, whose "cmake" utility corresponds to version 2.8.12, which is too old for Buildroot.

Signed-off-by Carlos Santos <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

---
 support/dependencies/check-host-cmake.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

support/dependencies/check-host-cmake.mk +1 -1

index ef080ff..07b064c 100644
@@ -10,7 +10,7 @@
 #
 BR2_CMAKE_VERSION_MIN = 3.1
 
-BR2_CMAKE_CANDIDATES ?= cmake
+BR2_CMAKE_CANDIDATES ?= cmake cmake3
 BR2_CMAKE ?= $(call suitable-host-package,cmake,\
 	$(BR2_CMAKE_VERSION_MIN) $(BR2_CMAKE_CANDIDATES))
 ifeq ($(BR2_CMAKE),)

Signature · 1 line

2.7.4

Yann E. MORIN

2017-05-07 09:40 UTC #8 lore raw

PATCH v2 1/4 core: reverse the argument order in check-host-cmake

In reply to Carlos Santos

Carlos, All,

On 2017-05-07 01:32 -0300, Carlos Santos spake thusly:

Quoted 4 lines

Pass the minimal version before the program name. In a later change the script will become able to test a list of candidates.
Signed-off-by Carlos Santos <[email protected]>

Acked-by "Yann E. MORIN" <[email protected]>

Regards,
Yann E. MORIN.

Quoted 35 lines

---
 support/dependencies/check-host-cmake.mk | 2 +-
 support/dependencies/check-host-cmake.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

support/dependencies/check-host-cmake.mk +1 -1

index 8002278..914327f 100644
@@ -12,7 +12,7 @@ BR2_CMAKE_VERSION_MIN = 3.1
 
 BR2_CMAKE ?= cmake
 ifeq ($(call suitable-host-package,cmake,\
-	$(BR2_CMAKE) $(BR2_CMAKE_VERSION_MIN)),)
+	$(BR2_CMAKE_VERSION_MIN) $(BR2_CMAKE)),)
 BR2_CMAKE = $(HOST_DIR)/usr/bin/cmake
 BR2_CMAKE_HOST_DEPENDENCY = host-cmake
 endif

support/dependencies/check-host-cmake.sh +2 -2

index 9b63b06..73bd4ed 100755
@@ -1,7 +1,7 @@
 #!/bin/sh
 
-candidate="${1}"
-version_min="${2}"
+version_min="${1}"
+candidate="${2}"
 
 major_min="${version_min%.*}"
 minor_min="${version_min#*.}"

Signature · 1 line

2.7.4

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Thomas Petazzoni

2017-06-24 19:46 UTC #9 lore raw

PATCH v2 1/4 core: reverse the argument order in check-host-cmake

In reply to Carlos Santos

Hello,

Quoted 8 lines · Carlos Santos

Pass the minimal version before the program name. In a later change the script will become able to test a list of candidates.
Signed-off-by Carlos Santos <[email protected]>
---
 support/dependencies/check-host-cmake.mk | 2 +-
 support/dependencies/check-host-cmake.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Thanks, series applied to master! And thanks to Yann for reviewing those patches.

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com