Skip to contentSkip to conversation

cmake.bbclass: use `cmake --build` to build & install

PATCH In review Cody P Schafer 2 participants 6 messages May 1, 2017 – May 2, 2017

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 cmake.bbclass: use `cmake --build` to build & install

Rather than presuming make is the generator, use cmake's generic cmake --build feature (which knows to call the appropriate generator).

Both DESTDIR and VERBOSE still behave as intended when used as environment variables instead of make variable-arguments.

This makes it more straight forward for others to select other cmake generators (many folks have been reaching for ninja lately).

---
 meta/classes/cmake.bbclass | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
meta/classes/cmake.bbclass +2 -4
index 12df617ad8..05ec50d385 100644
@@ -135,13 +135,11 @@ cmake_do_configure() {
 
 do_compile[progress] = "percent"
 cmake_do_compile()  {
-	cd ${B}
-	base_do_compile VERBOSE=1
+	VERBOSE=1 cmake --build '${B}'
 }
 
 cmake_do_install() {
-	cd ${B}
-	oe_runmake 'DESTDIR=${D}' install
+	DESTDIR='${D}' cmake --build '${B}' --target install
 }
 
 EXPORT_FUNCTIONS do_configure do_compile do_install do_generate_toolchain_file
Signature · 1 line
2.12.2
#1 lore raw
Quoted 19 lines · Cody P Schafer

Rather than presuming make is the generator, use cmake's generic cmake --build feature (which knows to call the appropriate generator).

Both DESTDIR and VERBOSE still behave as intended when used as environment variables instead of make variable-arguments.

This makes it more straight forward for others to select other cmake generators (many folks have been reaching for ninja lately).

---
 meta/classes/cmake.bbclass | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
meta/classes/cmake.bbclass +0 -0
index 12df617ad8..05ec50d385 100644

meta/classes/cmake.bbclass cmake_do_configure() { · line 138

Quoted 4 lines
meta/classes/cmake.bbclass +0 -1
@@ -135,13 +135,11 @@ cmake_do_configure() {
 
 do_compile[progress] = "percent"
 cmake_do_compile()  {
-       cd ${B}

Removing the redundant "cd ${B}" should perhaps be a separate commit?

meta/classes/cmake.bbclass cmake_do_configure() { · line 138

Quoted 2 lines
meta/classes/cmake.bbclass +1 -1
-       base_do_compile VERBOSE=1
+       VERBOSE=1 cmake --build '${B}'

This looks like it will cause parallel make and EXTRA_OEMAKE options to be ignored?

Quoted 17 lines
meta/classes/cmake.bbclass +1 -3
 }
 
 cmake_do_install() {
-       cd ${B}
-       oe_runmake 'DESTDIR=${D}' install
+       DESTDIR='${D}' cmake --build '${B}' --target install
 }
 
 EXPORT_FUNCTIONS do_configure do_compile do_install do_generate_toolchain_file
--

2.12.2

--
_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core

#2 lore raw
Quoted 5 lines
Quoted 3 lines

do_compile[progress] = "percent"
cmake_do_compile() {
- cd ${B}

Removing the redundant "cd ${B}" should perhaps be a separate commit?

I'm not really seeing that as useful, change right now can be viewed as "moves the ${B} from being in cd to being part of the command line for cmake --build". Is fairly straight forward because cmake requires a path be specified (where make makes it optional).

Quoted 6 lines

meta/classes/cmake.bbclass cmake_do_configure() { · line 138

Quoted 3 lines
meta/classes/cmake.bbclass +1 -1
-       base_do_compile VERBOSE=1
+       VERBOSE=1 cmake --build '${B}'

This looks like it will cause parallel make and EXTRA_OEMAKE options to be ignored?

That is a good point. I don't think EXTRA_OEMAKE should matter here too much as configuration for cmake-based builds all is read at configure time, but the ${PARALLEL_MAKE} flag is definitely an issue.

Doing this could work:

    VERBOSE=1 cmake --build '${B}' -- ${EXTRA_OEMAKE}

(And similar for install)

And for the case of -jN like flags, make & ninja happen to be compatible (they share a few other flags, like -l and -n.

Another alternative is passing ${PARALLEL_MAKE} (and ${PARALLEL_MAKEINST}) instead of directly using ${EXTRA_OEMAKE}.

Also: I took a look at what else run_oemake is doing, and it contains some output printing (bbnotes the make command & dies on make failure), which could be added directly around the cmake --build invocations instead.

#3 lore raw
Quoted 10 lines · Cody P Schafer
Quoted 5 lines
Quoted 3 lines

do_compile[progress] = "percent"
cmake_do_compile() {
- cd ${B}

Removing the redundant "cd ${B}" should perhaps be a separate commit?

I'm not really seeing that as useful, change right now can be viewed as "moves the ${B} from being in cd to being part of the command line for cmake --build". Is fairly straight forward because cmake requires a path be specified (where make makes it optional).

If ${B} is the default directory for both these tasks then "cd ${B}" is redundant and can be removed. If it is not the default directory, then removing it will potentially break do_compile_append() and do_install_append() functions which rely on running in ${B}. That logic doesn't depend on whether or not "cmake --build" takes care of changing directory on it's own, so if "cd ${B}" is redundant, then removing it should be standalone commit.

Quoted 27 lines
Quoted 6 lines

meta/classes/cmake.bbclass cmake_do_configure() { · line 138

Quoted 3 lines
meta/classes/cmake.bbclass +1 -1
-       base_do_compile VERBOSE=1
+       VERBOSE=1 cmake --build '${B}'

This looks like it will cause parallel make and EXTRA_OEMAKE options to be ignored?

That is a good point. I don't think EXTRA_OEMAKE should matter here too much as configuration for cmake-based builds all is read at configure time, but the ${PARALLEL_MAKE} flag is definitely an issue.

Doing this could work:

    VERBOSE=1 cmake --build '${B}' -- ${EXTRA_OEMAKE}

(And similar for install)

And for the case of -jN like flags, make & ninja happen to be compatible (they share a few other flags, like -l and -n.

Another alternative is passing ${PARALLEL_MAKE} (and ${PARALLEL_MAKEINST}) instead of directly using ${EXTRA_OEMAKE}.

Also: I took a look at what else run_oemake is doing, and it contains some output printing (bbnotes the make command & dies on make failure), which could be added directly around the cmake --build invocations instead.

#4 lore raw
Quoted 12 lines · Andre McCurdy
Quoted 10 lines · Cody P Schafer
Quoted 5 lines
Quoted 3 lines

do_compile[progress] = "percent"
cmake_do_compile() {
- cd ${B}

Removing the redundant "cd ${B}" should perhaps be a separate commit?

I'm not really seeing that as useful, change right now can be viewed as "moves the ${B} from being in cd to being part of the command line for cmake --build". Is fairly straight forward because cmake requires a path be specified (where make makes it optional).

Quoted 7 lines

If ${B} is the default directory for both these tasks then "cd ${B}" is redundant and can be removed. If it is not the default directory, then removing it will potentially break do_compile_append() and do_install_append() functions which rely on running in ${B}. That logic doesn't depend on whether or not "cmake --build" takes care of changing directory on it's own, so if "cd ${B}" is redundant, then removing it should be standalone commit.

    meta/classes/base.bbclass
    307:do_configure[dirs] = "${B}"
    326:do_compile[dirs] = "${B}"

${B} is the default dir for do_compile & do_configure.

Are we concerned about prepend scripts changing the directory to something not-${B} & appends implicitly relying on ${B} being the directory?

It seems that prepends/appends should avoid changing directories &/or avoid depending on a specific directory. Many (but not all) of the appends I've seen are pretty explicitly about the dirs they need (Using ${B}, ${S}, and ${WORKDIR} as needed instead of assuming the dir). Is there a policy on what the right thing to do is w.r.t. cwd changes within tasks & specifically within appends/prepends to tasks?

#5 lore raw
Quoted 25 lines · Cody P Schafer
Quoted 12 lines · Andre McCurdy
Quoted 10 lines · Cody P Schafer
Quoted 5 lines
Quoted 3 lines

do_compile[progress] = "percent"
cmake_do_compile() {
- cd ${B}

Removing the redundant "cd ${B}" should perhaps be a separate commit?

I'm not really seeing that as useful, change right now can be viewed as "moves the ${B} from being in cd to being part of the command line for cmake --build". Is fairly straight forward because cmake requires a path be specified (where make makes it optional).

Quoted 7 lines

If ${B} is the default directory for both these tasks then "cd ${B}" is redundant and can be removed. If it is not the default directory, then removing it will potentially break do_compile_append() and do_install_append() functions which rely on running in ${B}. That logic doesn't depend on whether or not "cmake --build" takes care of changing directory on it's own, so if "cd ${B}" is redundant, then removing it should be standalone commit.

    meta/classes/base.bbclass
    307:do_configure[dirs] = "${B}"
    326:do_compile[dirs] = "${B}"
Quoted 5 lines

${B} is the default dir for do_compile & do_configure.

Are we concerned about prepend scripts changing the directory to something not-${B} & appends implicitly relying on ${B} being the directory?

No, no concerns about that. The point is simply that you appear to have identified two redundant "cd ${B}" commands in cmake.bbclass and removing them could/should be a standalone commit rather than being mixed up with the "cmake --build" change. If every patch only does one thing then it makes the review process a little easier.

Quoted 6 lines

It seems that prepends/appends should avoid changing directories &/or avoid depending on a specific directory. Many (but not all) of the appends I've seen are pretty explicitly about the dirs they need (Using ${B}, ${S}, and ${WORKDIR} as needed instead of assuming the dir). Is there a policy on what the right thing to do is w.r.t. cwd changes within tasks & specifically within appends/prepends to tasks?