Re: [cmake-developers] Modules/GetPrequisites.cmake issues
On 03/08/2015 15:22, Brad King wrote: Hi Brad, On 07/30/2015 10:56 AM, Brad King wrote: Thanks! Applied: Those patches exposed a bug in FindMPI, so I fixed that and rebased the other changes on it: Glad my patch flushed out an unrelated defect. OTOH there are many other uses of execute_process() in the Modules that look like they might be suspect through not checking if the process returned a success status. It's not a blanket change as some do seem to be failsafe by checking for error output. Even a very simple command might fail unexpectedly due to access rights or similar. FindMPI: Drop unnecessary and incorrect use of GetPrerequisites http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=1c46b6ae GetPrerequisites: Add error checks for execute_process() calls http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=afb674ab GetPrerequisites: Optionally filter "objdump" output for speed http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=5d0a8b1a -Brad Regards Bill Somerville. -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] Modules/GetPrequisites.cmake issues
On 29/07/2015 14:37, Brad King wrote:
Hi Brad,
On 07/28/2015 06:02 PM, Bill Somerville wrote:
attached is a patch that addresses some issues recently discussed on the
users list.
...
If you're comfortable enough with Git, please split this part out into
a preceding patch with its own explanation in its commit message. That
will clarify which hunks in the patch belong to which change.
...
This can be simplified as:
execute_process(
COMMAND ${gp_cmd} ${gp_cmd_args} ${target}
${gp_cmd_maybe_filter}
RESULT_VARIABLE gp_rv
OUTPUT_VARIABLE gp_cmd_ov
ERROR_VARIABLE gp_ev
)
where ``gp_cmd_maybe_filter`` is initialized to empty and later
possibly set as:
if(gp_grep_cmd)
set(gp_cmd_maybe_filter
COMMAND ${gp_grep_cmd} "^[[:blank:]]*DLL Name: "
)
endif()
Revised patches attached.
Thanks,
-Brad
Regards
Bill Somerville.
>From 0332d20411f62d865b5a84d8e0f78f68a49d1a0b Mon Sep 17 00:00:00 2001
From: Bill Somerville
Date: Wed, 29 Jul 2015 17:05:51 +0100
Subject: [PATCH 1/2] Add error checks for execute_process() calls
Return status checks added to external command invocations so that
they do not fail silently producing incomplete install packages.
---
Modules/GetPrerequisites.cmake | 30 ++
1 file changed, 30 insertions(+)
diff --git a/Modules/GetPrerequisites.cmake b/Modules/GetPrerequisites.cmake
index 23d486e..a359d2c 100644
--- a/Modules/GetPrerequisites.cmake
+++ b/Modules/GetPrerequisites.cmake
@@ -229,9 +229,14 @@ function(is_file_executable file result_var)
if(file_cmd)
execute_process(COMMAND "${file_cmd}" "${file_full}"
+RESULT_VARIABLE file_rv
OUTPUT_VARIABLE file_ov
+ERROR_VARIABLE file_ev
OUTPUT_STRIP_TRAILING_WHITESPACE
)
+ if(NOT file_rv STREQUAL "0")
+message(FATAL_ERROR "${file_cmd} failed: ${file_rv}\n${file_ev}")
+ endif()
# Replace the name of the file in the output with a placeholder token
# (the string " _file_full_ ") so that just in case the path name of
@@ -543,11 +548,21 @@ function(gp_resolved_file_type original_file file exepath
dirs type_var)
if(CYGPATH_EXECUTABLE)
execute_process(COMMAND ${CYGPATH_EXECUTABLE} -W
+ RESULT_VARIABLE env_rv
OUTPUT_VARIABLE env_windir
+ ERROR_VARIABLE env_ev
OUTPUT_STRIP_TRAILING_WHITESPACE)
+ if(NOT env_rv STREQUAL "0")
+message(FATAL_ERROR "${CYGPATH_EXECUTABLE} -W failed:
${env_rv}\n${env_ev}")
+ endif()
execute_process(COMMAND ${CYGPATH_EXECUTABLE} -S
+ RESULT_VARIABLE env_rv
OUTPUT_VARIABLE env_sysdir
+ ERROR_VARIABLE env_ev
OUTPUT_STRIP_TRAILING_WHITESPACE)
+ if(NOT env_rv STREQUAL "0")
+message(FATAL_ERROR "${CYGPATH_EXECUTABLE} -S failed:
${env_rv}\n${env_ev}")
+ endif()
string(TOLOWER "${env_windir}" windir)
string(TOLOWER "${env_sysdir}" sysroot)
@@ -765,8 +780,18 @@ function(get_prerequisites target prerequisites_var
exclude_system recurse exepa
#
execute_process(
COMMAND ${gp_cmd} ${gp_cmd_args} ${target}
+RESULT_VARIABLE gp_rv
OUTPUT_VARIABLE gp_cmd_ov
+ERROR_VARIABLE gp_ev
)
+ if(NOT gp_rv STREQUAL "0")
+if(gp_tool STREQUAL "dumpbin")
+ # dumpbin error messages seem to go to stdout
+ message(FATAL_ERROR "${gp_cmd} failed: ${gp_rv}\n${gp_ev}\n${gp_cmd_ov}")
+else()
+ message(FATAL_ERROR "${gp_cmd} failed: ${gp_rv}\n${gp_ev}")
+endif()
+ endif()
if(gp_tool STREQUAL "ldd")
set(ENV{LD_LIBRARY_PATH} "${old_ld_env}")
@@ -791,8 +816,13 @@ function(get_prerequisites target prerequisites_var
exclude_system recurse exepa
if(gp_tool STREQUAL "otool")
execute_process(
COMMAND otool -D ${target}
+ RESULT_VARIABLE otool_rv
OUTPUT_VARIABLE gp_install_id_ov
+ ERROR_VARIABLE otool_ev
)
+if(NOT otool_rv STREQUAL "0")
+ message(FATAL_ERROR "otool -D failed: ${otool_rv}\n${otool_ev}")
+endif()
# second line is install name
string(REGEX REPLACE ".*:\n" "" gp_install_id "${gp_install_id_ov}")
if(gp_install_id)
--
1.9.5.msysgit.0
>From 85c1785af125f18581e69b492409339d074ec18e Mon Sep 17 00:00:00 2001
From: Bill Somerville
Date: Wed, 29 Jul 2015 17:36:38 +0100
Subject: [PATCH 2/2] Optional filter for dependency walker & use it for
objdump on MinGW
As dumpbin.exe is no longer reliable for gcc libraries on MinGW
because it crashes on ma
Re: [cmake-developers] Modules/GetPrequisites.cmake issues
On 29/07/2015 16:07, Bill Hoffman wrote: Hi Bill, On 7/29/2015 10:17 AM, Bill Somerville wrote: Is there a reason not to look for objdump before dumpbin? I was under the impression that dumpbin was selected first for non-MinGW Windows builds, I have no idea if objdump works with binaries produced by compilers other than GNU/CLang. This is why I commented about detecting MinGW, there are mentions of a MINGW CMake system variable but it doesn't seem to exist. Yes, that is the problem. This is a standalone script that is run at install time, so it does not have any information about the build toolchain. In retrospect it should have been passed more information about the build tool chain that was used. This is what we want: #dumpbin (Windows) #objdump (MinGW on Windows) #ldd (Linux/Unix) #otool (Mac OSX) There is already a documented override by setting the gp_tool variable, that's what we do in our project since we only support MinGW on Windows (at the moment). Trouble is dumpbin and objdump might both be in the PATH on Windows. You don't really know which one to use. I wonder if you found both if you could call something on each of them with one of the binaries, if you could figure out what tool would work better with it. You can't decide based on one executable since dumpbin works on some and not others, I believe the problem is with DLLs that export exactly zero symbols which is true of the GCC support libraries. It may be that if objdump is available it can always be used but I don't know that for sure, also it is still slower than dumpbin even with the patch I am proposing. -Bill Regards Bill Somerville. -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] Modules/GetPrequisites.cmake issues
On 29/07/2015 14:37, Brad King wrote:
Hi Brad,
On 07/28/2015 06:02 PM, Bill Somerville wrote:
...
As there doesn't seem to be a reliable way of detecting MinGW callers
of fixup_bundle() may have to set the variable gp_tool to "objdump" if
dumpbin.exe is installed on the build machine to stop it using
dumpbin.exe for library dependency walking.
Is there a reason not to look for objdump before dumpbin?
I was under the impression that dumpbin was selected first for non-MinGW
Windows builds, I have no idea if objdump works with binaries produced
by compilers other than GNU/CLang. This is why I commented about
detecting MinGW, there are mentions of a MINGW CMake system variable but
it doesn't seem to exist.
This patch also adds command status checking to the various
execute_process() invocations in GetPrerequisites.cmake so that they
don't fail silently producing invalid install packages.
If you're comfortable enough with Git, please split this part out into
a preceding patch with its own explanation in its commit message. That
will clarify which hunks in the patch belong to which change.
No problem with that, I will do that shortly.
+ if(gp_cmd_filter) # filter is for performance
+execute_process(
+ COMMAND ${gp_cmd} ${gp_cmd_args} ${target}
+ COMMAND ${gp_grep_cmd} ${gp_cmd_filter}
+ RESULT_VARIABLE gp_rv
+ OUTPUT_VARIABLE gp_cmd_ov
+ ERROR_VARIABLE gp_ev
+ )
+ else()
+execute_process(
+ COMMAND ${gp_cmd} ${gp_cmd_args} ${target}
+ RESULT_VARIABLE gp_rv
+ OUTPUT_VARIABLE gp_cmd_ov
+ ERROR_VARIABLE gp_ev
+ )
+ endif()
This can be simplified as:
execute_process(
COMMAND ${gp_cmd} ${gp_cmd_args} ${target}
${gp_cmd_maybe_filter}
RESULT_VARIABLE gp_rv
OUTPUT_VARIABLE gp_cmd_ov
ERROR_VARIABLE gp_ev
)
where ``gp_cmd_maybe_filter`` is initialized to empty and later
possibly set as:
if(gp_grep_cmd)
set(gp_cmd_maybe_filter
COMMAND ${gp_grep_cmd} "^[[:blank:]]*DLL Name: "
)
endif()
I tried that first but couldn't get it to work, maybe because I didn't
initialize it to empty, I will try again.
Thanks,
-Brad
Thanks for the review,
regards
Bill Somerville.
--
Powered by www.kitware.com
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Kitware offers various services to support the CMake community. For more
information on each offering, please visit:
CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers
[cmake-developers] Modules/GetPrequisites.cmake issues
Hi All,
attached is a patch that addresses some issues recently discussed on the
users list.
I'm no CMake expert so I don't know if I have done things in the best
way. One area I do not fully understand is how to detect MinGW, If I
could then objdump could be chosen as the preferred library dependency
walker over MS dumpbin.exe. See the patch description for details.
Regards
Bill Somerville.
From 203848c099026c23b5f70d395d0469887f099d23 Mon Sep 17 00:00:00 2001
From: Bill Somerville
Date: Tue, 28 Jul 2015 22:40:36 +0100
Subject: [PATCH] For MinGW pre-filter objdump output for performance
As dumpbin.exe is no longer reliable for gcc libraries on MinGW
because it crashes on many common libraries like libgcc_s and
libgfortran it is now necessary too resort to using objdump for DLL
dependency walking. Using objdump has a secondary problem in that it
generates a lot of output for large libraries and causes
fixup_bundle() to take many minutes to process what took fractions of
a second with dumpbin.exe /dependents.
This patch includes a grep pre-filter in the execute_process() command
pipeline to reduce this output to a minimum for a several orders of
magnitude speed up. If grep isn't available the full output is used.
As there doesn't seem to be a reliable way of detecting MinGW callers
of fixup_bundle() may have to set the variable gp_tool to "objdump" if
dumpbin.exe is installed on the build machine to stop it using
dumpbin.exe for library dependency walking.
This patch also adds command status checking to the various
execute_process() invocations in GetPrerequisites.cmake so that they
don't fail silently producing invalid install packages.
---
Modules/GetPrerequisites.cmake | 53 ++
1 file changed, 49 insertions(+), 4 deletions(-)
diff --git a/Modules/GetPrerequisites.cmake b/Modules/GetPrerequisites.cmake
index 23d486e..bbc1232 100644
--- a/Modules/GetPrerequisites.cmake
+++ b/Modules/GetPrerequisites.cmake
@@ -229,9 +229,14 @@ function(is_file_executable file result_var)
if(file_cmd)
execute_process(COMMAND "${file_cmd}" "${file_full}"
+RESULT_VARIABLE file_rv
OUTPUT_VARIABLE file_ov
+ERROR_VARIABLE file_ev
OUTPUT_STRIP_TRAILING_WHITESPACE
)
+ if(NOT file_rv STREQUAL "0")
+message(FATAL_ERROR "${file_cmd} failed: ${file_rv}\n${file_ev}")
+ endif()
# Replace the name of the file in the output with a placeholder token
# (the string " _file_full_ ") so that just in case the path name of
@@ -543,11 +548,21 @@ function(gp_resolved_file_type original_file file exepath
dirs type_var)
if(CYGPATH_EXECUTABLE)
execute_process(COMMAND ${CYGPATH_EXECUTABLE} -W
+ RESULT_VARIABLE env_rv
OUTPUT_VARIABLE env_windir
+ ERROR_VARIABLE env_ev
OUTPUT_STRIP_TRAILING_WHITESPACE)
+ if(NOT env_rv STREQUAL "0")
+message(FATAL_ERROR "${CYGPATH_EXECUTABLE} -W failed:
${env_rv}\n${env_ev}")
+ endif()
execute_process(COMMAND ${CYGPATH_EXECUTABLE} -S
+ RESULT_VARIABLE env_rv
OUTPUT_VARIABLE env_sysdir
+ ERROR_VARIABLE env_ev
OUTPUT_STRIP_TRAILING_WHITESPACE)
+ if(NOT env_rv STREQUAL "0")
+message(FATAL_ERROR "${CYGPATH_EXECUTABLE} -S failed:
${env_rv}\n${env_ev}")
+ endif()
string(TOLOWER "${env_windir}" windir)
string(TOLOWER "${env_sysdir}" sysroot)
@@ -709,6 +724,11 @@ function(get_prerequisites target prerequisites_var
exclude_system recurse exepa
set(gp_regex_error "")
set(gp_regex_fallback "")
set(gp_regex_cmp_count 1)
+# objdump generaates copious output so we create a grep filter to
pre-filter results
+find_program(gp_grep_cmd grep)
+if(gp_grep_cmd)
+ set(gp_cmd_filter "^[[:blank:]]*DLL Name: ")
+endif()
else()
message(STATUS "warning: gp_tool='${gp_tool}' is an unknown tool...")
message(STATUS "CMake function get_prerequisites needs more code to handle
'${gp_tool}'")
@@ -763,10 +783,30 @@ function(get_prerequisites target prerequisites_var
exclude_system recurse exepa
# Run gp_cmd on the target:
#
- execute_process(
-COMMAND ${gp_cmd} ${gp_cmd_args} ${target}
-OUTPUT_VARIABLE gp_cmd_ov
-)
+ if(gp_cmd_filter) # filter is for performance
+execute_process(
+ COMMAND ${gp_cmd} ${gp_cmd_args} ${target}
+ COMMAND ${gp_grep_cmd} ${gp_cmd_filter}
+ RESULT_VARIABLE gp_rv
+ OUTPUT_VARIAB
