Hadoop Common
  1. Hadoop Common
  2. HADOOP-8747

Syntax error on cmake version 2.6 patch 2 in JNIFlags.cmake

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.2-alpha
    • Fix Version/s: 2.0.2-alpha
    • Component/s: native
    • Labels:
      None

      Description

      On SUSE Linux Enterprise Server 11 SP1, "cmake version 2.6 patch 2" is installed.

      It seems to have trouble parsing this "if" statement in JNIFlags.cmake:

      IF((NOT JAVA_JVM_LIBRARY) OR (NOT JAVA_INCLUDE_PATH) OR (NOT JAVA_INCLUDE_PATH2))
      

      We should rephrase this if statement so that it will work on all versions of cmake above or equal to 2.6.

      1. HADOOP-8747.001.patch
        0.9 kB
        Colin Patrick McCabe
      2. HADOOP-8747.002.patch
        1 kB
        Colin Patrick McCabe

        Activity

        Colin Patrick McCabe created issue -
        Hide
        Colin Patrick McCabe added a comment -

        I think the handling of parentheses is at fault here. Here's a version of the if statement which doesn't use extra parentheses.

        Show
        Colin Patrick McCabe added a comment - I think the handling of parentheses is at fault here. Here's a version of the if statement which doesn't use extra parentheses.
        Colin Patrick McCabe made changes -
        Field Original Value New Value
        Attachment HADOOP-8747.001.patch [ 12542955 ]
        Colin Patrick McCabe made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Andy Isaacson added a comment -

        The failure we saw was:

             [exec] CMake Error: Error in cmake code at
             [exec] /var/lib/jenkins/workspace/CDH4-Hadoop-MR2-2.0.0/hadoop-common-project/hadoop-common/src/JNIFlags.cmake:106:
             [exec] Parse error.  Function missing ending ")".  Instead found left paren with text "(".
        
        Show
        Andy Isaacson added a comment - The failure we saw was: [exec] CMake Error: Error in cmake code at [exec] / var /lib/jenkins/workspace/CDH4-Hadoop-MR2-2.0.0/hadoop-common-project/hadoop-common/src/JNIFlags.cmake:106: [exec] Parse error. Function missing ending ")" . Instead found left paren with text "(" .
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12542955/HADOOP-8747.001.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

        org.apache.hadoop.ha.TestZKFailoverController

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1381//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1381//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12542955/HADOOP-8747.001.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverController +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1381//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1381//console This message is automatically generated.
        Hide
        Colin Patrick McCabe added a comment -

        I tested this on on SUSE Linux Enterprise Server 11 SP1, using "cmake version 2.6 patch 2."

        Also tested on OpenSuSE 12.1 using cmake version 2.8.6.

        Show
        Colin Patrick McCabe added a comment - I tested this on on SUSE Linux Enterprise Server 11 SP1, using "cmake version 2.6 patch 2." Also tested on OpenSuSE 12.1 using cmake version 2.8.6.
        Hide
        Alejandro Abdelnur added a comment -

        +1. Colin, Before I commit, have you tested in other Linux distros as well?

        Show
        Alejandro Abdelnur added a comment - +1. Colin, Before I commit, have you tested in other Linux distros as well?
        Hide
        Todd Lipcon added a comment -

        Looks good. One suggestion: the strange empty IF clause makes this a bit hard to read. Can we do something like:

        +    IF(JAVA_JVM_LIBRARY AND JAVA_INCLUDE_PATH AND JAVA_INCLUDE_PATH2)
               MESSAGE("Using JAVA_JVM_LIBRARY=${JAVA_JVM_LIBRARY} JAVA_INCLUDE_PATH=${JAVA_INCLUDE_PATH} JAVA_INCLUDE_PATH2=${JAVA_INCLUDEPATH2}")
        +    ELSE()
        

        so that we don't have an empty block?

        Show
        Todd Lipcon added a comment - Looks good. One suggestion: the strange empty IF clause makes this a bit hard to read. Can we do something like: + IF(JAVA_JVM_LIBRARY AND JAVA_INCLUDE_PATH AND JAVA_INCLUDE_PATH2) MESSAGE( "Using JAVA_JVM_LIBRARY=${JAVA_JVM_LIBRARY} JAVA_INCLUDE_PATH=${JAVA_INCLUDE_PATH} JAVA_INCLUDE_PATH2=${JAVA_INCLUDEPATH2}" ) + ELSE() so that we don't have an empty block?
        Hide
        Colin Patrick McCabe added a comment -
        • new version without empty 'if' block
        Show
        Colin Patrick McCabe added a comment - new version without empty 'if' block
        Colin Patrick McCabe made changes -
        Attachment HADOOP-8747.002.patch [ 12543001 ]
        Hide
        Colin Patrick McCabe added a comment -

        @Alejandro: Jenkins builds on Debian, so that's another distro this patch has been tested on.

        Show
        Colin Patrick McCabe added a comment - @Alejandro: Jenkins builds on Debian, so that's another distro this patch has been tested on.
        Hide
        Colin Patrick McCabe added a comment -

        Also tested on centos 5.8 and ubuntu 12.04. Anyway, the really important issue here is the cmake version.

        Show
        Colin Patrick McCabe added a comment - Also tested on centos 5.8 and ubuntu 12.04. Anyway, the really important issue here is the cmake version.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12543001/HADOOP-8747.002.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

        org.apache.hadoop.ha.TestZKFailoverController

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1384//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1384//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12543001/HADOOP-8747.002.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverController +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1384//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1384//console This message is automatically generated.
        Hide
        Alejandro Abdelnur added a comment -

        +1

        Show
        Alejandro Abdelnur added a comment - +1
        Hide
        Alejandro Abdelnur added a comment -

        Thanks Colin. Committed to trunk and branch-2.

        Show
        Alejandro Abdelnur added a comment - Thanks Colin. Committed to trunk and branch-2.
        Alejandro Abdelnur made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags Reviewed [ 10343 ]
        Fix Version/s 2.2.0-alpha [ 12322473 ]
        Resolution Fixed [ 1 ]
        Arun C Murthy made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        2m 18s 1 Colin Patrick McCabe 29/Aug/12 18:07
        Patch Available Patch Available Resolved Resolved
        5h 34m 1 Alejandro Abdelnur 29/Aug/12 23:42
        Resolved Resolved Closed Closed
        42d 18h 2m 1 Arun C Murthy 11/Oct/12 17:45

          People

          • Assignee:
            Colin Patrick McCabe
            Reporter:
            Colin Patrick McCabe
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development