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

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

[Miroslav Keš]

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 <miroslav.kes@gmail.com>

diff --git a/meta/classes/cmake.bbclass b/meta/classes/cmake.bbclass
index c9c15f3..95a1cbf 100644
--- a/meta/classes/cmake.bbclass
+++ b/meta/classes/cmake.bbclass
@@ -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} \

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

[Burton, Ross]

On 13 June 2014 17:33, Miroslav Keš <miroslav.kes@gmail.com> wrote:
> +    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

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

[Miroslav Keš]

On 06/13/14 18:38, Burton, Ross wrote:
> On 13 June 2014 17:33, Miroslav Keš <miroslav.kes@gmail.com> wrote:
>> +    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.

You are right.

> 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
> Ross

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

[Miroslav Keš]

On 06/13/14 18:38, Burton, Ross wrote:
> On 13 June 2014 17:33, Miroslav Keš <miroslav.kes@gmail.com> wrote:
>> +    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

Here is the updated patch:

Signed-off-by: Mira Kes <miroslav.kes@gmail.com>

diff --git a/meta/classes/cmake.bbclass b/meta/classes/cmake.bbclass
index c9c15f3..f762792 100644
--- a/meta/classes/cmake.bbclass
+++ b/meta/classes/cmake.bbclass
@@ -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} \

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

[Saul Wold]

On 06/17/2014 01:20 PM, Miroslav Keš wrote:
> On 06/13/14 18:38, Burton, Ross wrote:
>> On 13 June 2014 17:33, Miroslav Keš <miroslav.kes@gmail.com> wrote:
>>> +    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
>
> Here is the updated patch:
>

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

> Signed-off-by: Mira Kes <miroslav.kes@gmail.com>
>
> diff --git a/meta/classes/cmake.bbclass b/meta/classes/cmake.bbclass
> index c9c15f3..f762792 100644
> --- a/meta/classes/cmake.bbclass
> +++ b/meta/classes/cmake.bbclass
> @@ -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} \
>
>

[PATCH v2] cmake: respect ${S} and ${B} patch problem

[Miroslav Keš]

> 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}


On 06/17/14 22:36, Saul Wold wrote:
>
> 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 <miroslav.kes@gmail.com>
---
 meta/classes/cmake.bbclass | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/meta/classes/cmake.bbclass b/meta/classes/cmake.bbclass
index c9c15f3..f762792 100644
--- a/meta/classes/cmake.bbclass
+++ b/meta/classes/cmake.bbclass
@@ -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} \
-- 
1.8.5.5

Re: [PATCH v2] cmake: respect ${S} and ${B} patch problem

[Burton, Ross]

On 19 June 2014 20:39, Miroslav Keš <miroslav.kes@gmail.com> wrote:
> 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

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

[Burton, Ross]

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

Ross

On 17 June 2014 21:20, Miroslav Keš <miroslav.kes@gmail.com> wrote:
> On 06/13/14 18:38, Burton, Ross wrote:
>> On 13 June 2014 17:33, Miroslav Keš <miroslav.kes@gmail.com> wrote:
>>> +    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
>
> Here is the updated patch:
>
> Signed-off-by: Mira Kes <miroslav.kes@gmail.com>
>
> diff --git a/meta/classes/cmake.bbclass b/meta/classes/cmake.bbclass
> index c9c15f3..f762792 100644
> --- a/meta/classes/cmake.bbclass
> +++ b/meta/classes/cmake.bbclass
> @@ -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} \
>
>