Hadoop Common
  1. Hadoop Common
  2. HADOOP-8921

ant build.xml in branch-1 ignores -Dcompile.native

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Unresolved
    • Affects Version/s: 1.2.0
    • Fix Version/s: None
    • Component/s: build
    • Labels:
    • Environment:

      Mac OS X 10.7.4

    • Release Note:
      Prevent autotools dependency when native libraries are disabled/unavailable

      Description

      ant -Dcompile.native=false still runs autoconf and libtoolize

      According to ant 1.8 manual, any <target if> conditions are checked only after the dependencies are run through. The current if condition in code fails to prevent the autoconf/libtool components from running.

      Fixing it by moving the if condition up into the "compile-native" target and changing it to a param substitution instead of being evaluated as a condition.

      1. HADOOP-8921.4.patch
        1 kB
        Gopal V
      2. HADOOP-8921.5.patch
        2 kB
        Chris Nauroth
      3. hadoop-8921.6.patch
        1 kB
        Giridharan Kesavan
      4. HADOOP-8921.7.patch
        0.6 kB
        Arpit Agarwal

        Issue Links

          Activity

          Hide
          Tsz Wo Nicholas Sze added a comment -

          Hi Giri, does the latest patch looks good to you? Could you commit it?

          Show
          Tsz Wo Nicholas Sze added a comment - Hi Giri, does the latest patch looks good to you? Could you commit it?
          Hide
          Arpit Agarwal added a comment -

          I edited Gopal's patch to keep the bare minimal changes to fix the OS X build.

          Verified build on OS X and native build on Linux (branch-1).

          Show
          Arpit Agarwal added a comment - I edited Gopal's patch to keep the bare minimal changes to fix the OS X build. Verified build on OS X and native build on Linux (branch-1).
          Hide
          Arpit Agarwal added a comment -

          +1 for the minimal change that gets "ant compile" working on OS X. This change can be left out.

             <!-- taskcontroller targets -->
          -  <target name="task-controller" depends="init">
          +  <target name="task-controller" depends="init" if="compile.native" 
          +      description="compile's task-controller if compile.native is set">
          

          Also minor typo in the comment (no apostrophe).

          +  <target name="compile-native" description="compile's native code by setting the compile.native">
          
          Show
          Arpit Agarwal added a comment - +1 for the minimal change that gets "ant compile" working on OS X. This change can be left out. <!-- taskcontroller targets --> - <target name= "task-controller" depends= "init" > + <target name= "task-controller" depends= "init" if= "compile.native" + description= "compile's task-controller if compile.native is set" > Also minor typo in the comment (no apostrophe). + <target name= "compile-native" description= "compile's native code by setting the compile.native" >
          Hide
          Chris Nauroth added a comment -

          FYI, I filed MAPREDUCE-4995 for task-controller compilation failure on Mac.

          Show
          Chris Nauroth added a comment - FYI, I filed MAPREDUCE-4995 for task-controller compilation failure on Mac.
          Hide
          Giridharan Kesavan added a comment -

          If anyone has comments pls do so before 02/05. I'm planning to commit version 6 (removing if compile.native) by tomorrow.

          Show
          Giridharan Kesavan added a comment - If anyone has comments pls do so before 02/05. I'm planning to commit version 6 (removing if compile.native) by tomorrow.
          Hide
          Chris Nauroth added a comment -

          Also linked to HADOOP-7927, which is a similar bug report.

          Show
          Chris Nauroth added a comment - Also linked to HADOOP-7927 , which is a similar bug report.
          Hide
          Chris Nauroth added a comment -

          Actually, I take back my last comment about task-controller. It would still be incompatible in the sense that people running "ant package" may currently expect it to bundle task-controller, and the change I described would require them to start passing the compile.native parameter.

          I am +1 for version 6 of the patch after removal of if="compile.native" from the task-controller target. This is enough to get "ant compile" working on Mac. We'd still need a Linux VM to run "ant package".

          Does anyone think that perhaps the right thing to do is to get task-controller compiling on Mac? This would be handled in a separate jira. I've pasted the compilation failures below. PATH_MAX undeclared is trivially fixed by adding #include <limits.h>. The missing functions are trickier. In my quick grep of the Mac header files, I didn't find any declarations for those functions.

               [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c:84: error: ‘PATH_MAX’ undeclared (first use in this function)
               [exec] cc1: warnings being treated as errors
               [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c: In function ‘mkdirs’:
               [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c:343: warning: implicit declaration of function ‘mkdirat’
               [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c:352: warning: implicit declaration of function ‘openat’
               [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c: In function ‘run_task_as_user’:
               [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c:904: warning: implicit declaration of function ‘fcloseall’
               [exec] make: *** [impl/task-controller.o] Error 1
          
          Show
          Chris Nauroth added a comment - Actually, I take back my last comment about task-controller. It would still be incompatible in the sense that people running "ant package" may currently expect it to bundle task-controller, and the change I described would require them to start passing the compile.native parameter. I am +1 for version 6 of the patch after removal of if="compile.native" from the task-controller target. This is enough to get "ant compile" working on Mac. We'd still need a Linux VM to run "ant package". Does anyone think that perhaps the right thing to do is to get task-controller compiling on Mac? This would be handled in a separate jira. I've pasted the compilation failures below. PATH_MAX undeclared is trivially fixed by adding #include <limits.h>. The missing functions are trickier. In my quick grep of the Mac header files, I didn't find any declarations for those functions. [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c:84: error: ‘PATH_MAX’ undeclared (first use in this function) [exec] cc1: warnings being treated as errors [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c: In function ‘mkdirs’: [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c:343: warning: implicit declaration of function ‘mkdirat’ [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c:352: warning: implicit declaration of function ‘openat’ [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c: In function ‘run_task_as_user’: [exec] /Users/chris/git/hadoop-common/src/c++/task-controller/impl/task-controller.c:904: warning: implicit declaration of function ‘fcloseall’ [exec] make: *** [impl/task-controller.o] Error 1
          Hide
          Chris Nauroth added a comment -

          Thanks, Giri. This makes sense. For my part, I was really just looking for a way to run "ant clean package" on Mac without triggering autoreconf and the other native build steps. Moving these from depends to antcalls inside compile-core-native accomplishes the same goal, and like you said, it preserves the behavior of not needing to set the compile.native property when you're already calling the targets related to native build.

          Regarding task-controller, the difficulty is that task-controller is called as a subant of package, which makes it difficult to test the package target on Mac, where task-controller fails to compile. Could we set this up in a way similar to compile-native, which calls compile-core-native, and explicitly turns on the compile.native parameters? Then, inclusion of task-controller would be controlled by the compile.native flag when running "ant package", but users who run "ant task-controller" would still get the existing behavior without needing to pass the extra parameter. (Then, there would be no need to mark this "incompatible".)

          Show
          Chris Nauroth added a comment - Thanks, Giri. This makes sense. For my part, I was really just looking for a way to run "ant clean package" on Mac without triggering autoreconf and the other native build steps. Moving these from depends to antcalls inside compile-core-native accomplishes the same goal, and like you said, it preserves the behavior of not needing to set the compile.native property when you're already calling the targets related to native build. Regarding task-controller, the difficulty is that task-controller is called as a subant of package, which makes it difficult to test the package target on Mac, where task-controller fails to compile. Could we set this up in a way similar to compile-native, which calls compile-core-native, and explicitly turns on the compile.native parameters? Then, inclusion of task-controller would be controlled by the compile.native flag when running "ant package", but users who run "ant task-controller" would still get the existing behavior without needing to pass the extra parameter. (Then, there would be no need to mark this "incompatible".)
          Hide
          Giridharan Kesavan added a comment -

          I agree with Brandon. compile-native target was made available for the purpose of building libhadoop without having to set the compile.native flag.

          Yes, I agree, when calling compile-core-native without setting compile.native flag autoreconf should get triggered.

          Here is what I propose, How about moving the depends into antcalls inside the compile-core-native? by doing so the autoreconf won't get triggered if compile.native is not set.

          Finally if we think task-controller target should be controlled with compile.native flag then we should mark this jira "incompatible" At least for me I do ant task-controller and expect it to build.

          here is my v6 patch with my thoughts above.

          Show
          Giridharan Kesavan added a comment - I agree with Brandon. compile-native target was made available for the purpose of building libhadoop without having to set the compile.native flag. Yes, I agree, when calling compile-core-native without setting compile.native flag autoreconf should get triggered. Here is what I propose, How about moving the depends into antcalls inside the compile-core-native? by doing so the autoreconf won't get triggered if compile.native is not set. Finally if we think task-controller target should be controlled with compile.native flag then we should mark this jira "incompatible" At least for me I do ant task-controller and expect it to build. here is my v6 patch with my thoughts above.
          Hide
          Chris Nauroth added a comment -

          Thanks for providing this patch, Gopal V. I've found that in the current branch-1 codebase, I also get a failure on Mac due to the native compile of task controller. I'm uploading HADOOP-8921.5.patch. The only difference from the version 4 patch is that I have added the if native check to the task-controller target:

          -  <target name="task-controller" depends="init">
          +  <target name="task-controller" depends="init" if="${compile.native}">
          
          Show
          Chris Nauroth added a comment - Thanks for providing this patch, Gopal V . I've found that in the current branch-1 codebase, I also get a failure on Mac due to the native compile of task controller. I'm uploading HADOOP-8921 .5.patch. The only difference from the version 4 patch is that I have added the if native check to the task-controller target: - <target name= "task-controller" depends= "init" > + <target name= "task-controller" depends= "init" if = "${compile. native }" >
          Hide
          Gopal V added a comment -

          I've personally wasted over an hour to figure out that option has to be explicitly disabled before it skips running autoconf/automake (in create-configure part).

          The older approach was bad for someone who did a git checkout and ran a build without paying attention to the docs about native library compatibility.

          I'd say the optimizations (i.e non-core features) can be skipped over for a clean build on platforms where the code won't compile / isn't supported. Every single attempt to build on a Mac would result in a failed compilation, until someone discovers the -Dcompile.native option.

          Clean builds on "ant compile" should be encouraged (on any platform) without RTFMing for a -D option.

          Of course on platforms where the native code is indeed supported, it would error out if say, JNI headers can't be found.

          Show
          Gopal V added a comment - I've personally wasted over an hour to figure out that option has to be explicitly disabled before it skips running autoconf/automake (in create-configure part). The older approach was bad for someone who did a git checkout and ran a build without paying attention to the docs about native library compatibility. I'd say the optimizations (i.e non-core features) can be skipped over for a clean build on platforms where the code won't compile / isn't supported. Every single attempt to build on a Mac would result in a failed compilation, until someone discovers the -Dcompile.native option. Clean builds on "ant compile" should be encouraged (on any platform) without RTFMing for a -D option. Of course on platforms where the native code is indeed supported, it would error out if say, JNI headers can't be found.
          Hide
          Brandon Li added a comment -

          The patch looks good. Even though "-Dcompile.native=true" is redundant in the command "ant compile-native -Dcompile.native=true"("ant compile-native" can do the job itself), it's still good to be consistent.

          One minor thing is the os-arch check. Original code doesn't check the x86_64/x86 availability, native code compilation can fail on certain platforms(such as MacOS). At lease the developer will notice the failure. With the os-arch check in the patch, the "ant compile-native" command will report success but actually didn't do the work. The original behavior seems more intuitive to me.

          Show
          Brandon Li added a comment - The patch looks good. Even though "-Dcompile.native=true" is redundant in the command "ant compile-native -Dcompile.native=true"("ant compile-native" can do the job itself), it's still good to be consistent. One minor thing is the os-arch check. Original code doesn't check the x86_64/x86 availability, native code compilation can fail on certain platforms(such as MacOS). At lease the developer will notice the failure. With the os-arch check in the patch, the "ant compile-native" command will report success but actually didn't do the work. The original behavior seems more intuitive to me.
          Hide
          Hadoop QA added a comment -

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1621//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/12548878/HADOOP-8921.4.patch against trunk revision . -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1621//console This message is automatically generated.
          Hide
          Gopal V added a comment -

          Enable native libraries only after checking for x86_64/x86 availability.

          Show
          Gopal V added a comment - Enable native libraries only after checking for x86_64/x86 availability.
          Hide
          Gopal V added a comment -

          Re-update patch to run native builds by default, but disable when -Dcompile.native=false is provided.

          Show
          Gopal V added a comment - Re-update patch to run native builds by default, but disable when -Dcompile.native=false is provided.
          Hide
          Gopal V added a comment -

          Expand patch to cover the compile-core target as well

          Show
          Gopal V added a comment - Expand patch to cover the compile-core target as well
          Hide
          Gopal V added a comment -

          After patch

          $ ant compile-native  -Dcompile.native=false
          Buildfile: /home/gopal/hw/hadoop-1/build.xml
          
          compile-native:
          
          BUILD SUCCESSFUL
          Total time: 0 seconds
          
          
          
          $ ant compile-native  -Dcompile.native=true
          Buildfile: /home/gopal/hw/hadoop-1/build.xml
          
          compile-native:
          
          create-native-configure:
               [exec] configure.ac:42: warning: AC_COMPILE_IFELSE was called before AC_USE_SYSTEM_EXTENSIONS
          
          
          Show
          Gopal V added a comment - After patch $ ant compile- native -Dcompile. native = false Buildfile: /home/gopal/hw/hadoop-1/build.xml compile- native : BUILD SUCCESSFUL Total time: 0 seconds $ ant compile- native -Dcompile. native = true Buildfile: /home/gopal/hw/hadoop-1/build.xml compile- native : create- native -configure: [exec] configure.ac:42: warning: AC_COMPILE_IFELSE was called before AC_USE_SYSTEM_EXTENSIONS
          Hide
          Gopal V added a comment -

          Patch to build.xml

          Show
          Gopal V added a comment - Patch to build.xml

            People

            • Assignee:
              Gopal V
              Reporter:
              Gopal V
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development