List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:February 4 2011 9:48am
Subject:review of bug#42969
View as plain text  
Hello Joerg,

Here is a review. Please forgive my relative cmake ignorance.

> #At file:///MySQL/REPO/V55/bug42969-5.5/ based on
> revid:jorgen.loland@stripped
> 
>  3209 Joerg Bruehe	2011-01-31
>       Fix bug#42969    Please add a MANIFEST to each build
>       
>       With this change, there will be new files "INFO_SRC"
>       and "INFO_BIN", which describe the source and the
>       binaries.
>       They will be contained in all packages:
>       - in "tar.gz" and derived packages, in "docs/",
>       - in RPMs, in "/usr/share/doc/packages/MySQL-server".

In the future we may also want to ask the server directly about this 
info, like with
SHOW VARIABLES LIKE "SOURCE_REVISION_ID",
which would show the bzr revision id. So maybe there will be in the 
future, at build time, a (cmake?) script parsing INFO_SRC/INFO_BIN and 
putting the result into some .h file which itself will be used in 
compilation.

>       "INFO_SRC" is also part of a source tarball.
>       It gives the version as exact as possible, preferably
>       by calling "bzr version-info" on the source tree.
>       If that is not possible, it just contains the three
>       level version number.
>       
>       "INFO_BIN" contains some info when and where the
>       binaries were built, the options given to the compiler,
>       and the flags controlling the included features.
>       
>       (At the time of this writing, January 2011,
>       cmake 2.6.4 does not provide some of this desired
>       information in the extent promised by its documentation.)
>      @ CMakeLists.txt
>         For the new files describing the source and the build
>         ("INFO_SRC" and "INFO_BIN"), we need a new file
>         "cmake/info_macros.cmake.in" with the build rules.
>         
>         1) This file must be configured with the current variables.
>         
>         2) "INFO_SRC" can be created during the cmake phase.
>         
>         3) "INFO_BIN" must be created during the make phase
>            only, because it contains information from files
>         which will be written at the end of the cmake phase only.
>         Therefore, it must be a custom target which is included
>         in all "make" targets.
>         
>         4) The resulting version files must be included in packages.
>      @ cmake/info_bin.cmake
>         This is the file to create "INFO_BIN",
>         by calling the "CREATE_INFO_BIN()" macro.
>         
>         It must be a separate file, so that the macro
>         definitions can be included in other cmake scripts
>         without that file inclusion causing a side effect,
>         the macro call.
>         That call would modify the source tree which should
>         be trated read-only.
>      @ cmake/info_macros.cmake.in
>         This new file contains the macros to create the
>         "INFO_*" files during various steps of the build,
>         the calls will be at other places.
>         
>         1) For source: If running from a BZR tree, always create
>            (update) "INFO_SRC" by running "bzr version-info".
>            Outside a BZR tree, try to take it from exported
>            sources, and create it only if missing, in that
>            case put the three level version number into it.
>         
>         2) "INFO_BIN" contains (if provided)
>            - date/time and host name of the build host,
>            - information about the platform,
>              (2.6.4 does not provide it, the manual is wrong),
>            - information about the C and CXX compiler
>              and the options given to them (Unix only),
>            - the feature flags as reported by "cmake -L".
>      @ cmake/make_dist.cmake.in
>         Create a "VERSION_src" file during "make dist".
>         
>         In case it already exists from a preceding "cmake" run
>         or tree export (which is quite likely), a new
>         "make dist" must not modify it.
>      @ support-files/mysql.spec.sh
>         Add "INFO_SRC" and "INFO_BIN" to the RPM contents.
> 
>     added:
>       cmake/info_bin.cmake
>       cmake/info_macros.cmake.in
>     modified:
>       CMakeLists.txt
>       cmake/make_dist.cmake.in
>       support-files/mysql.spec.sh
> === modified file 'CMakeLists.txt'
> --- a/CMakeLists.txt	2010-12-15 10:30:09 +0000
> +++ b/CMakeLists.txt	2011-01-31 19:32:10 +0000
> @@ -1,4 +1,4 @@
> -# Copyright (c) 2006, 2010, Oracle and/or its affiliates. All rights reserved.
> +# Copyright (c) 2006, 2011, Oracle and/or its affiliates. All rights reserved.
>  # 
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -317,6 +317,19 @@ CONFIGURE_FILE(${CMAKE_SOURCE_DIR}/inclu
>                 ${CMAKE_BINARY_DIR}/include/mysql_version.h )
>  CONFIGURE_FILE(${CMAKE_SOURCE_DIR}/sql/sql_builtin.cc.in
>      ${CMAKE_BINARY_DIR}/sql/sql_builtin.cc)
> +CONFIGURE_FILE(
> +    ${CMAKE_SOURCE_DIR}/cmake/info_macros.cmake.in
> ${CMAKE_BINARY_DIR}/info_macros.cmake @ONLY)

What is @ONLY for?

> +
> +# Handle the "INFO_*" files.
> +INCLUDE(${CMAKE_BINARY_DIR}/info_macros.cmake)
> +# Source: This can be done during the cmake phase, all information is
> +# available.
> +CREATE_INFO_SRC(${CMAKE_BINARY_DIR}/Docs)

If in my tree I do a build (cmake + make), then bzr pull, then "make" 
again, the macro above will not be run, and INFO_SRC will still contain 
the old, now out-of-date, bzr revision information?

> +# Build flags: This must be postponed to the make phase.
> +ADD_CUSTOM_TARGET(INFO_BIN ALL
> +  COMMAND ${CMAKE_COMMAND} -P ${CMAKE_SOURCE_DIR}/cmake/info_bin.cmake
> +  WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
> +)
>  
>  # Packaging
>  IF(WIN32)
> @@ -344,6 +357,7 @@ IF(NOT INSTALL_LAYOUT MATCHES "RPM")
>    OPTIONAL
>    )
>    INSTALL(FILES README DESTINATION ${INSTALL_DOCREADMEDIR} COMPONENT Readme)
> +  INSTALL(FILES ${CMAKE_BINARY_DIR}/Docs/INFO_SRC ${CMAKE_BINARY_DIR}/Docs/INFO_BIN
> DESTINATION ${INSTALL_DOCDIR})

I don't understand: INFO_BIN is created only during the "make" phase, 
but is already used above (in the cmake phase) by the INSTALL command?

>    IF(UNIX)
>      INSTALL(FILES Docs/INSTALL-BINARY DESTINATION ${INSTALL_DOCREADMEDIR} COMPONENT
> Readme)
>    ENDIF()
> 
> === added file 'cmake/info_bin.cmake'
> --- a/cmake/info_bin.cmake	1970-01-01 00:00:00 +0000
> +++ b/cmake/info_bin.cmake	2011-01-31 19:32:10 +0000
> @@ -0,0 +1,30 @@
> +# Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
> +# 
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; version 2 of the License.
> +# 
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +# 
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> +
> +
> +# The sole purpose of this cmake control file is to create the "INFO_BIN" file.
> +
> +# By having a separate cmake file for this, it is ensured this happens
> +# only in the build (Unix: "make") phase, not when cmake runs.
> +# This, in turn, avoids creating stuff in the source directory -
> +# it should get into the binary directory only.
> +
> +
> +# Get the macros which the "INFO_*" files.
> +INCLUDE(${CMAKE_BINARY_DIR}/info_macros.cmake)
> +
> +# Here is where the action is.
> +CREATE_INFO_BIN()
> +
> 
> === added file 'cmake/info_macros.cmake.in'
> --- a/cmake/info_macros.cmake.in	1970-01-01 00:00:00 +0000
> +++ b/cmake/info_macros.cmake.in	2011-01-31 19:32:10 +0000
> @@ -0,0 +1,119 @@
> +# Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
> +# 
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; version 2 of the License.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA 
> +
> +
> +# Handle/create the "INFO_*" files describing a MySQL (server) binary.
> +# This is part of the fix for bug#42969.
> +
> +SET(VERSION "@VERSION@")
> +SET(CMAKE_SOURCE_DIR "@CMAKE_SOURCE_DIR@")
> +SET(CMAKE_BINARY_DIR "@CMAKE_BINARY_DIR@")
> +SET(BZR_EXECUTABLE "@BZR_EXECUTABLE@")
> + 
> + 
> +# Create an "INFO_SRC" file with information about the source (only).
> +# We use "bzr version-info", if possible, and the "VERSION" contents.
> +#
> +# Outside development (BZR tree), the "INFO_SRC" file will not be modified
> +# provided it exists (from "make dist" or a source tarball creation).
> +
> +MACRO(CREATE_INFO_SRC target_dir)
> +  SET(INFO_SRC "${target_dir}/INFO_SRC")
> +
> +  IF(EXISTS ${CMAKE_SOURCE_DIR}/.bzr)
> +    # Sources are in a BZR repository: Always update.
> +    EXECUTE_PROCESS(
> +      COMMAND ${BZR_EXECUTABLE} version-info ${CMAKE_SOURCE_DIR}
> +      OUTPUT_VARIABLE VERSION_INFO
> +      RESULT_VARIABLE RESULT
> +    )
> +    FILE(WRITE ${INFO_SRC} "${VERSION_INFO}\n")
> +    FILE(APPEND ${INFO_SRC} "\nResult ${RESULT}\n")
> +    # For better readability ...
> +    FILE(APPEND ${INFO_SRC} "\nMySQL source ${VERSION}\n")
> +  ELSEIF(EXISTS ${INFO_SRC})
> +    # Outside a BZR tree, there is no need to change an existing "INFO_SRC",
> +    # it cannot be improved.
> +  ELSEIF(EXISTS ${CMAKE_SOURCE_DIR}/Docs/INFO_SRC)
> +    # If we are building from a source distribution, it also contains "INFO_SRC".
> +    # Similar, the export used for a release build already has the file.
> +    FILE(READ ${CMAKE_SOURCE_DIR}/Docs/INFO_SRC SOURCE_INFO)
> +    FILE(WRITE ${INFO_SRC} "${SOURCE_INFO}\n")
> +  ELSEIF(EXISTS ${CMAKE_SOURCE_DIR}/INFO_SRC)
> +    # This is not the proper location, but who knows ...

Maybe this is an over-paranoid case which can be ignored (deleted)?

> +    FILE(READ ${CMAKE_SOURCE_DIR}/INFO_SRC SOURCE_INFO)
> +    FILE(WRITE ${INFO_SRC} "${SOURCE_INFO}\n")
> +  ELSE()
> +    # This is a fall-back.
> +    FILE(WRITE ${INFO_SRC} "\nMySQL source ${VERSION}\n")
> +  ENDIF()
> +ENDMACRO(CREATE_INFO_SRC)

I would add a test with mysql-test-run. The .test file would use the 
"perl" command, to verify that the INFO_SRC and INFO_BIN are there and 
that their content looks sound (for example, "bzr version-info" should 
look like
revision-id:.*@.*
date: 2.*
etc
).

> +
> +
> +# This is for the "real" build, must be run again with each cmake run
> +# to make sure we report the current flags (not those of some previous run).
> +
> +MACRO(CREATE_INFO_BIN)
> +  SET(INFO_BIN "Docs/INFO_BIN")
> +
> +  FILE(WRITE ${INFO_BIN} "===== Information about the build process: =====\n")
> +  IF (WIN32)
> +    EXECUTE_PROCESS(COMMAND date /c /T OUTPUT_VARIABLE TMP_DATE)
> +  ELSEIF(UNIX)
> +    EXECUTE_PROCESS(COMMAND date "+%Y-%m-%d %H:%M:%S" OUTPUT_VARIABLE TMP_DATE)
> +  ELSE()
> +    SET(TMP_DATE "No date command known")
> +  ENDIF()
> +  SITE_NAME(HOSTNAME)
> +  FILE(APPEND ${INFO_BIN} "Build was run at ${TMP_DATE} on host '${HOSTNAME}'\n\n")
> +
> +  # According to the cmake docs, these variables should always be set.
> +  # However, they are empty in my tests, using cmake 2.6.4 on Linux, various Unix,
> and Windows.
> +  # Still, include this code, so we will profit if a build environment does provide
> that info.
> +  IF(${CMAKE_HOST_SYSTEM})
> +    FILE(APPEND ${INFO_BIN} "Build was done on  ${CMAKE_HOST_SYSTEM} using
> ${CMAKE_HOST_SYSTEM_PROCESSOR}\n")
> +  ENDIF()
> +  IF(${CMAKE_SYSTEM})
> +    FILE(APPEND ${INFO_BIN} "Build was done for ${CMAKE_SYSTEM} using
> ${CMAKE_SYSTEM_PROCESSOR}\n")
> +  ENDIF()
> +
> +  # ${CMAKE_VERSION} doesn't work in 2.6.0, use the separate components.
> +  FILE(APPEND ${INFO_BIN} "Build was done using cmake
> ${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION}.${CMAKE_PATCH_VERSION} \n\n")
> +
> +  IF (WIN32)
> +    FILE(APPEND ${INFO_BIN} "===== Compiler / generator used: =====\n")
> +    FILE(APPEND ${INFO_BIN} ${CMAKE_GENERATOR} "\n\n")
> +  ELSEIF(UNIX)
> +    FILE(APPEND ${INFO_BIN} "===== Compiler flags used (from the 'sql/'
> subdirectory): =====\n")
> +    IF(EXISTS sql/CMakeFiles/sql.dir/flags.make)
> +      EXECUTE_PROCESS(COMMAND egrep "^# compile|^C_|^CXX_"
> sql/CMakeFiles/sql.dir/flags.make OUTPUT_VARIABLE COMPILE_FLAGS)

Does egrep exist on all our Unix machines?

> +      FILE(APPEND ${INFO_BIN} ${COMPILE_FLAGS} "\n")
> +    ELSE()
> +      FILE(APPEND ${INFO_BIN} "File 'sql/CMakeFiles/sql.dir/flags.make' is not yet
> found.\n\n")
> +    ENDIF()
> +  ENDIF()
> +
> +  FILE(APPEND ${INFO_BIN} "===== Feature flags used: =====\n")
> +  IF(EXISTS ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> +    # Attention: "-N" prevents cmake from entering a recursion, and it must be a
> separate flag from "-L".
> +    EXECUTE_PROCESS(COMMAND ${CMAKE_COMMAND} -N -L ${CMAKE_BINARY_DIR}
> OUTPUT_VARIABLE FEATURE_FLAGS)
> +    FILE(APPEND ${INFO_BIN} ${FEATURE_FLAGS} "\n\n")
> +  ELSE()
> +    FILE(APPEND ${INFO_BIN} "File 'CMakeCache.txt' is not yet found.\n\n")
> +  ENDIF()
> +
> +  FILE(APPEND ${INFO_BIN} "===== EOF =====\n")
> +ENDMACRO(CREATE_INFO_BIN)
> +
> 
> === modified file 'cmake/make_dist.cmake.in'
> --- a/cmake/make_dist.cmake.in	2010-11-20 14:47:50 +0000
> +++ b/cmake/make_dist.cmake.in	2011-01-31 19:32:10 +0000
> @@ -1,4 +1,4 @@
> -# Copyright (C) 2009 Sun Microsystems, Inc
> +# Copyright (c) 2009, 2011, Oracle and/or its affiliates. All rights reserved.
>  # 
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -106,6 +106,12 @@ IF(MYSQL_DOCS_LOCATION)
>    EXECUTE_PROCESS(COMMAND ${CMAKE_COMMAND} -E copy_directory
> "${MYSQL_DOCS_LOCATION}" "${PACKAGE_DIR}")
>  ENDIF()
>  
> +# Ensure there is an "INFO_SRC" file.
> +INCLUDE(${CMAKE_BINARY_DIR}/info_macros.cmake)
> +IF(NOT EXISTS ${PACKAGE_DIR}/Docs/INFO_SRC)

in which case can this happen?

> +  CREATE_INFO_SRC(${PACKAGE_DIR}/Docs)
> +ENDIF()
> +
>  # In case we used CPack, it could have copied some
>  # extra files that are not usable on different machines.
>  FILE(REMOVE ${PACKAGE_DIR}/CMakeCache.txt)
> 
> === modified file 'support-files/mysql.spec.sh'
> --- a/support-files/mysql.spec.sh	2010-11-24 15:55:23 +0000
> +++ b/support-files/mysql.spec.sh	2011-01-31 19:32:10 +0000
> @@ -1,4 +1,4 @@
> -# Copyright (c) 2000, 2010, Oracle and/or its affiliates. All rights reserved.
> +# Copyright (c) 2000, 2011, Oracle and/or its affiliates. All rights reserved.
>  #
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -908,6 +908,8 @@ echo "====="                            
>  %doc %{license_files_server}
>  %endif
>  %doc %{src_dir}/Docs/ChangeLog
> +%doc %{src_dir}/Docs/INFO_SRC*
> +%doc release/Docs/INFO_BIN*
>  %doc release/support-files/my-*.cnf
>  
>  %doc %attr(644, root, root) %{_infodir}/mysql.info*
> @@ -1085,6 +1087,10 @@ echo "====="                            
>  # merging BK trees)
>  ##############################################################################
>  %changelog
> +* Mon Jan 31 2011 Joerg Bruehe <joerg.bruehe@stripped>
> +
> +- Install the new "manifest" files: "INFO_SRC" and "INFO_BIN".
> +
>  * Tue Nov 23 2010 Jonathan Perkin <jonathan.perkin@stripped>
>  
>  - EXCEPTIONS-CLIENT has been deleted, remove it from here too
> 

second patch:

> === modified file 'cmake/info_macros.cmake.in'
> --- a/cmake/info_macros.cmake.in	2011-01-31 21:14:38 +0000
> +++ b/cmake/info_macros.cmake.in	2011-02-01 13:53:07 +0000
> @@ -17,12 +17,22 @@
>  # Handle/create the "INFO_*" files describing a MySQL (server) binary.
>  # This is part of the fix for bug#42969.
>  
> +
> +# Several of cmake's variables need to be translated from '@' notation
> +# to '${}', this is done by the "configure" call in top level "CMakeLists.txt".

I don't understand.

> +# If further variables are used in this file, add them to this list.
> +
>  SET(VERSION "@VERSION@")
>  SET(CMAKE_SOURCE_DIR "@CMAKE_SOURCE_DIR@")
>  SET(CMAKE_BINARY_DIR "@CMAKE_BINARY_DIR@")
>  SET(CMAKE_GENERATOR "@CMAKE_GENERATOR@")
>  SET(CMAKE_SIZEOF_VOID_P "@CMAKE_SIZEOF_VOID_P@")
>  SET(BZR_EXECUTABLE "@BZR_EXECUTABLE@")
> +SET(CMAKE_CROSSCOMPILING "@CMAKE_CROSSCOMPILING@")
> +SET(CMAKE_HOST_SYSTEM "@CMAKE_HOST_SYSTEM@")
> +SET(CMAKE_HOST_SYSTEM_PROCESSOR "@CMAKE_HOST_SYSTEM_PROCESSOR@")
> +SET(CMAKE_SYSTEM "@CMAKE_SYSTEM@")
> +SET(CMAKE_SYSTEM_PROCESSOR "@CMAKE_SYSTEM_PROCESSOR@")
>   
>   
>  # Create an "INFO_SRC" file with information about the source (only).
> @@ -76,7 +86,7 @@ MACRO(CREATE_INFO_BIN)
>    ELSEIF(UNIX)
>      EXECUTE_PROCESS(COMMAND date "+%Y-%m-%d %H:%M:%S" OUTPUT_VARIABLE TMP_DATE
> OUTPUT_STRIP_TRAILING_WHITESPACE)
>    ELSE()
> -    SET(TMP_DATE "No date command known")
> +    SET(TMP_DATE "(no date command known for this platform)")
>    ENDIF()
>    SITE_NAME(HOSTNAME)
>    FILE(APPEND ${INFO_BIN} "Build was run at ${TMP_DATE} on host '${HOSTNAME}'\n\n")
> @@ -84,10 +94,10 @@ MACRO(CREATE_INFO_BIN)
>    # According to the cmake docs, these variables should always be set.
>    # However, they are empty in my tests, using cmake 2.6.4 on Linux, various Unix,
> and Windows.
>    # Still, include this code, so we will profit if a build environment does provide
> that info.
> -  IF(${CMAKE_HOST_SYSTEM})
> +  IF(CMAKE_HOST_SYSTEM)

Even with this fix, the variables are still mysteriously empty?

>      FILE(APPEND ${INFO_BIN} "Build was done on  ${CMAKE_HOST_SYSTEM} using
> ${CMAKE_HOST_SYSTEM_PROCESSOR}\n")
>    ENDIF()
> -  IF(${CMAKE_SYSTEM})
> +  IF(CMAKE_CROSSCOMPILING)
>      FILE(APPEND ${INFO_BIN} "Build was done for ${CMAKE_SYSTEM} using
> ${CMAKE_SYSTEM_PROCESSOR}\n")
>    ENDIF()
>  
> @@ -112,7 +122,7 @@ MACRO(CREATE_INFO_BIN)
>    IF(EXISTS ${CMAKE_BINARY_DIR}/CMakeCache.txt)
>      # Attention: "-N" prevents cmake from entering a recursion, and it must be a
> separate flag from "-L".
>      EXECUTE_PROCESS(COMMAND ${CMAKE_COMMAND} -N -L ${CMAKE_BINARY_DIR}
> OUTPUT_VARIABLE FEATURE_FLAGS)
> -    FILE(APPEND ${INFO_BIN} ${FEATURE_FLAGS} "\n\n")
> +    FILE(APPEND ${INFO_BIN} ${FEATURE_FLAGS} "\n")
>    ELSE()
>      FILE(APPEND ${INFO_BIN} "File 'CMakeCache.txt' is not yet found.\n\n")
>    ENDIF()
> 
> 

-- 
Mr. Guilhem Bichot <guilhem.bichot@stripped>
Oracle / MySQL / Optimizer team, Lead Software Engineer
Bordeaux, France
www.oracle.com / www.mysql.com
Thread
review of bug#42969Guilhem Bichot4 Feb
  • Re: review of bug#42969Joerg Bruehe4 Feb
    • RE: review of bug#42969Vladislav Vaintroub4 Feb
      • Re: review of bug#42969Guilhem Bichot4 Feb
        • RE: review of bug#42969Vladislav Vaintroub4 Feb
    • Re: review of bug#42969Guilhem Bichot4 Feb