cmake: respect ${S} and ${B} patch problem

PATCH In review Miroslav Keš 3 participants 8 messages Jun 13, 2014 – Jun 24, 2014

Share & tools

Miroslav Keš

author

2014-06-13 16:34 UTC #0 lore raw

Hi!

I have found a problem related to the patch "respect ${S} and ${B}" on the cmake.bbclass (commit 43073569cb67d98c11aa71211d77b566b64f9145)

The cmake.bbclass now generates the cmake command using the ${S} variable as the path where the top most CMakeLists.txt should be found. This works OK as along as the CMakeLists.txt is in the top level directory of the package source tree.
But CMake doesn't require the directory tree to be structured that way. If the top level CMakeLists.txt is in a subdirectory of the package source tree AND the recipe needs to patch a file which is at a higher level the OE build is broken.

Example:

${WORKDIR}
- cmake
- Modules
- FindSomePackage.cmake
- Src
- CMakeLists.txt <- top level CMake file

For the cmake.bbclass to work correctly after the patch the S must be set to the ${WORKDIR}/Src.
But the ${S} is also used as the base directory for patching. In the example, the patch task creates directory ${S}/patches (i.e. ${WORKDIR}/Src/patches) and creates symbolic links to all relevant patches in that directory.
But those patches can only patch files under the ${S} subtree. The ${WORKDIR}/cmake/Modules/FindSomePackage.cmake becomes invisible for patches and the patch task ends up with error "No file to patch."

I would propose to return the OECMAKE_SOURCEPATH variable to the cmake.bbclass, pass it to the cmake command as the "path to the CMake file", and set its default value to ${S}

Regards

Mira

Signed-off-by Mira Kes <[email protected]>

meta/classes/cmake.bbclass +0 -2

index c9c15f3..95a1cbf 100644
@@ -65,8 +65,12 @@ EOF
 addtask generate_toolchain_file after do_patch before do_configure
 
 cmake_do_configure() {
-    if [ "${OECMAKE_BUILDPATH}" -o "${OECMAKE_SOURCEPATH}" ]; then
-        bbnote "cmake.bbclass no longer uses OECMAKE_SOURCEPATH and

OECMAKE_BUILDPATH. The default behaviour is now out-of-tree builds with B=WORKDIR/build."
+ if [ "${OECMAKE_BUILDPATH}" ]; then
+ bbnote "cmake.bbclass no longer uses OECMAKE_BUILDPATH. The
default behaviour is now out-of-tree builds with B=WORKDIR/build."
+ fi
+
+ if [ -z "${OECMAKE_SOURCEPATH}" ]; then
+ OECMAKE_SOURCEPATH="${S}"

     fi
 
     if [ "${S}" != "${B}" ]; then

@@ -84,7 +88,7 @@ cmake_do_configure() {

     cmake \
       ${OECMAKE_SITEFILE} \
-      ${S} \
+      ${OECMAKE_SOURCEPATH} \
       -DCMAKE_INSTALL_PREFIX:PATH=${prefix} \
       -DCMAKE_INSTALL_BINDIR:PATH=${bindir} \
       -DCMAKE_INSTALL_SBINDIR:PATH=${sbindir} \

Burton, Ross

2014-06-13 16:38 UTC #1 lore raw

Review cmake_do_configure() { · line 91

Quoted 11 lines · Miroslav Keš

(quoted diff) +3 -1

+    if [ -z "${OECMAKE_SOURCEPATH}" ]; then
+        OECMAKE_SOURCEPATH="${S}"
     fi
 
     if [ "${S}" != "${B}" ]; then

@@ -84,7 +88,7 @@ cmake_do_configure() {
 
     cmake \
       ${OECMAKE_SITEFILE} \
-      ${S} \
+      ${OECMAKE_SOURCEPATH} \

A better idiom that's more self-documenting would be to set OECMAKE_SOURCEPATH ?= "${S}" at the top-level.

Would it be sensible to give that variable a different name as it refers specifically to the location of the cmake file, and not the rest of the source?

Ross

Miroslav Keš

author

2014-06-13 16:51 UTC #2 lore raw

Quoted 8 lines · Ross

Quoted 7 lines · Miroslav Keš

(quoted diff) +2 -0

+    if [ -z "${OECMAKE_SOURCEPATH}" ]; then
+        OECMAKE_SOURCEPATH="${S}"
     fi
 
     if [ "${S}" != "${B}" ]; then

@@ -84,7 +88,7 @@ cmake_do_configure() {

Quoted 6 lines

Review cmake_do_configure() { · line 91
Quoted 4 lines
(quoted diff) +1 -1
     cmake \
       ${OECMAKE_SITEFILE} \
-      ${S} \
+      ${OECMAKE_SOURCEPATH} \
A better idiom that's more self-documenting would be to set OECMAKE_SOURCEPATH ?= "${S}" at the top-level.

You are right.

Quoted 3 lines

Would it be sensible to give that variable a different name as it refers specifically to the location of the cmake file, and not the rest of the source?

I was thinking about a better name too.
The reason why I stayed with the OECMAKE_SOURCEPATH is that the cmake man page says:

USAGE

cmake [options] <path-to-source>

And that's the directory that I want to set. So I thought that it would be understandable for people familiar with cmake.

Mira

Quoted 1 line

Ross

Miroslav Keš

author

2014-06-17 20:20 UTC #3 lore raw

In reply to Burton, Ross

Quoted 15 lines · Ross

Review cmake_do_configure() { · line 91
Quoted 11 lines · Miroslav Keš
(quoted diff) +3 -1
+    if [ -z "${OECMAKE_SOURCEPATH}" ]; then
+        OECMAKE_SOURCEPATH="${S}"
     fi
 
     if [ "${S}" != "${B}" ]; then
@@ -84,7 +88,7 @@ cmake_do_configure() {
 
     cmake \
       ${OECMAKE_SITEFILE} \
-      ${S} \
+      ${OECMAKE_SOURCEPATH} \
A better idiom that's more self-documenting would be to set OECMAKE_SOURCEPATH ?= "${S}" at the top-level.

Quoted 5 lines

Would it be sensible to give that variable a different name as it refers specifically to the location of the cmake file, and not the rest of the source?
Ross

Here is the updated patch:

Signed-off-by Mira Kes <[email protected]>

meta/classes/cmake.bbclass +6 -3

index c9c15f3..f762792 100644
@@ -23,6 +23,9 @@ OECMAKE_RPATH ?= ""
 OECMAKE_PERLNATIVE_DIR ??= ""
 OECMAKE_EXTRA_ROOT_PATH ?= ""
 
+# Path to the CMake file to process.
+OECMAKE_SOURCEPATH ?= "${S}"
+
 cmake_do_generate_toolchain_file() {
        cat > ${WORKDIR}/toolchain.cmake <<EOF
 # CMake system name must be something like "Linux".

@@ -65,8 +68,8 @@ EOF
 addtask generate_toolchain_file after do_patch before do_configure
 
 cmake_do_configure() {
-       if [ "${OECMAKE_BUILDPATH}" -o "${OECMAKE_SOURCEPATH}" ]; then
-               bbnote "cmake.bbclass no longer uses OECMAKE_SOURCEPATH and OECMAKE_BUILDPATH.  The default behaviour is now out-of-tree builds with B=WORKDIR/build."
+       if [ "${OECMAKE_BUILDPATH}" ]; then
+               bbnote "cmake.bbclass no longer uses OECMAKE_BUILDPATH.  The default behaviour is now out-of-tree builds with B=WORKDIR/build."
        fi
 
        if [ "${S}" != "${B}" ]; then

@@ -84,7 +87,7 @@ cmake_do_configure() {
 
        cmake \
          ${OECMAKE_SITEFILE} \
-         ${S} \
+         ${OECMAKE_SOURCEPATH} \
          -DCMAKE_INSTALL_PREFIX:PATH=${prefix} \
          -DCMAKE_INSTALL_BINDIR:PATH=${bindir} \
          -DCMAKE_INSTALL_SBINDIR:PATH=${sbindir} \

Saul Wold

2014-06-17 20:36 UTC #4 lore raw

Quoted 16 lines · Miroslav Keš

Quoted 15 lines · Ross
meta/classes/cmake.bbclass cmake_do_configure() { · line 91
Quoted 11 lines · Miroslav Keš
meta/classes/cmake.bbclass +3 -1
+    if [ -z "${OECMAKE_SOURCEPATH}" ]; then
+        OECMAKE_SOURCEPATH="${S}"
      fi
 
      if [ "${S}" != "${B}" ]; then
@@ -84,7 +88,7 @@ cmake_do_configure() {
 
      cmake \
        ${OECMAKE_SITEFILE} \
-      ${S} \
+      ${OECMAKE_SOURCEPATH} \
A better idiom that's more self-documenting would be to set OECMAKE_SOURCEPATH ?= "${S}" at the top-level.

Quoted 8 lines

Quoted 5 lines
Would it be sensible to give that variable a different name as it refers specifically to the location of the cmake file, and not the rest of the source?
Ross
Here is the updated patch:

Can you send this as a proper v2 patch please.
Thanks
Sau!

Quoted 38 lines

Signed-off-by Mira Kes <[email protected]>

meta/classes/cmake.bbclass +6 -3

index c9c15f3..f762792 100644
@@ -23,6 +23,9 @@ OECMAKE_RPATH ?= ""
  OECMAKE_PERLNATIVE_DIR ??= ""
  OECMAKE_EXTRA_ROOT_PATH ?= ""
 
+# Path to the CMake file to process.
+OECMAKE_SOURCEPATH ?= "${S}"
+
  cmake_do_generate_toolchain_file() {
         cat > ${WORKDIR}/toolchain.cmake <<EOF
  # CMake system name must be something like "Linux".

@@ -65,8 +68,8 @@ EOF
  addtask generate_toolchain_file after do_patch before do_configure
 
  cmake_do_configure() {
-       if [ "${OECMAKE_BUILDPATH}" -o "${OECMAKE_SOURCEPATH}" ]; then
-               bbnote "cmake.bbclass no longer uses OECMAKE_SOURCEPATH and OECMAKE_BUILDPATH.  The default behaviour is now out-of-tree builds with B=WORKDIR/build."
+       if [ "${OECMAKE_BUILDPATH}" ]; then
+               bbnote "cmake.bbclass no longer uses OECMAKE_BUILDPATH.  The default behaviour is now out-of-tree builds with B=WORKDIR/build."
         fi
 
         if [ "${S}" != "${B}" ]; then

@@ -84,7 +87,7 @@ cmake_do_configure() {
 
         cmake \
           ${OECMAKE_SITEFILE} \
-         ${S} \
+         ${OECMAKE_SOURCEPATH} \
           -DCMAKE_INSTALL_PREFIX:PATH=${prefix} \
           -DCMAKE_INSTALL_BINDIR:PATH=${bindir} \
           -DCMAKE_INSTALL_SBINDIR:PATH=${sbindir} \

Miroslav Keš

author

2014-06-19 19:39 UTC #5 lore raw

PATCH v2 1/1 cmake: respect ${S} and ${B} patch problem

Quoted 12 lines

The cmake.bbclass currently generates the cmake command using the ${S} variable as the path where the top most CMakeLists.txt should be found. This works OK as along as the CMakeLists.txt is in the top level directory of the package source tree.
But CMake doesn't require the directory tree to be structured that way. If the top level CMakeLists.txt is in a subdirectory of the package source tree AND the recipe needs to patch a file which is at a higher level the OE build is broken.
I would propose to return the OECMAKE_SOURCEPATH variable to the cmake.bbclass, pass it to the cmake command as the "path to the CMake file", and set its default value to ${S}

Quoted 4 lines · Saul Wold

Can you send this as a proper v2 patch please.
Thanks
Sau!

I'm doing it first time in my life. Hoping I understood the instructions correctly.

Mira

Signed-off-by Miroslav Kes <[email protected]>

---
 meta/classes/cmake.bbclass | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

meta/classes/cmake.bbclass +6 -3

index c9c15f3..f762792 100644
@@ -23,6 +23,9 @@ OECMAKE_RPATH ?= ""
 OECMAKE_PERLNATIVE_DIR ??= ""
 OECMAKE_EXTRA_ROOT_PATH ?= ""
 
+# Path to the CMake file to process.
+OECMAKE_SOURCEPATH ?= "${S}"
+
 cmake_do_generate_toolchain_file() {
     cat > ${WORKDIR}/toolchain.cmake <<EOF
 # CMake system name must be something like "Linux".

@@ -65,8 +68,8 @@ EOF
 addtask generate_toolchain_file after do_patch before do_configure
 
 cmake_do_configure() {
-    if [ "${OECMAKE_BUILDPATH}" -o "${OECMAKE_SOURCEPATH}" ]; then
-        bbnote "cmake.bbclass no longer uses OECMAKE_SOURCEPATH and OECMAKE_BUILDPATH.  The default behaviour is now out-of-tree builds with B=WORKDIR/build."
+    if [ "${OECMAKE_BUILDPATH}" ]; then
+        bbnote "cmake.bbclass no longer uses OECMAKE_BUILDPATH.  The default behaviour is now out-of-tree builds with B=WORKDIR/build."
     fi
 
     if [ "${S}" != "${B}" ]; then

@@ -84,7 +87,7 @@ cmake_do_configure() {
 
     cmake \
       ${OECMAKE_SITEFILE} \
-      ${S} \
+      ${OECMAKE_SOURCEPATH} \
       -DCMAKE_INSTALL_PREFIX:PATH=${prefix} \
       -DCMAKE_INSTALL_BINDIR:PATH=${bindir} \
       -DCMAKE_INSTALL_SBINDIR:PATH=${sbindir} \

Signature · 1 line

1.8.5.5

Burton, Ross

2014-06-24 20:24 UTC #6 lore raw

Quoted 1 line · Miroslav Keš

I'm doing it first time in my life. Hoping I understood the instructions correctly.

The best way to send the patch would be something like:

git send-email --to oe-core HEAD^ --subject-prefix=PATCH][V2

(send to oe-core the current commit, marking the subject with PATCH and V2)

However the patch as attached doesn't apply to master for some reason, so as it's so small I re-created it and have just submitted it. Ross

Burton, Ross

2014-06-17 21:22 UTC #7 lore raw

In reply to Miroslav Keš

Apart from what Saul said, this looks good to me.

Ross

Quoted 62 lines · Miroslav Keš

Quoted 15 lines · Ross

meta/classes/cmake.bbclass cmake_do_configure() { · line 91
Quoted 11 lines · Miroslav Keš
meta/classes/cmake.bbclass +3 -1
+    if [ -z "${OECMAKE_SOURCEPATH}" ]; then
+        OECMAKE_SOURCEPATH="${S}"
     fi
 
     if [ "${S}" != "${B}" ]; then
@@ -84,7 +88,7 @@ cmake_do_configure() {
 
     cmake \
       ${OECMAKE_SITEFILE} \
-      ${S} \
+      ${OECMAKE_SOURCEPATH} \
A better idiom that's more self-documenting would be to set OECMAKE_SOURCEPATH ?= "${S}" at the top-level.

Quoted 5 lines

Would it be sensible to give that variable a different name as it refers specifically to the location of the cmake file, and not the rest of the source?
Ross

Here is the updated patch:

Signed-off-by Mira Kes <[email protected]>

meta/classes/cmake.bbclass +6 -3

index c9c15f3..f762792 100644
@@ -23,6 +23,9 @@ OECMAKE_RPATH ?= ""
 OECMAKE_PERLNATIVE_DIR ??= ""
 OECMAKE_EXTRA_ROOT_PATH ?= ""
 
+# Path to the CMake file to process.
+OECMAKE_SOURCEPATH ?= "${S}"
+
 cmake_do_generate_toolchain_file() {
        cat > ${WORKDIR}/toolchain.cmake <<EOF
 # CMake system name must be something like "Linux".

@@ -65,8 +68,8 @@ EOF
 addtask generate_toolchain_file after do_patch before do_configure
 
 cmake_do_configure() {
-       if [ "${OECMAKE_BUILDPATH}" -o "${OECMAKE_SOURCEPATH}" ]; then
-               bbnote "cmake.bbclass no longer uses OECMAKE_SOURCEPATH and OECMAKE_BUILDPATH.  The default behaviour is now out-of-tree builds with B=WORKDIR/build."
+       if [ "${OECMAKE_BUILDPATH}" ]; then
+               bbnote "cmake.bbclass no longer uses OECMAKE_BUILDPATH.  The default behaviour is now out-of-tree builds with B=WORKDIR/build."
        fi
 
        if [ "${S}" != "${B}" ]; then

@@ -84,7 +87,7 @@ cmake_do_configure() {
 
        cmake \
          ${OECMAKE_SITEFILE} \
-         ${S} \
+         ${OECMAKE_SOURCEPATH} \
          -DCMAKE_INSTALL_PREFIX:PATH=${prefix} \
          -DCMAKE_INSTALL_BINDIR:PATH=${bindir} \
          -DCMAKE_INSTALL_SBINDIR:PATH=${sbindir} \