Hadoop Common
  1. Hadoop Common
  2. HADOOP-8776

Provide an option in test-patch that can enable / disable compiling native code

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.0
    • Fix Version/s: 3.0.0
    • Component/s: build
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      test-patch.sh adds a new option "--build-native". When set to false native
      components are not built. When set to true native components are built. The
      default value is true.
      Show
      test-patch.sh adds a new option "--build-native". When set to false native components are not built. When set to true native components are built. The default value is true.

      Description

      The test-patch script in Hadoop source runs a native compile with the patch. On platforms like MAC, there are issues with the native compile that make it difficult to use test-patch. This JIRA is to try and provide an option to make the native compilation optional.

      1. HADOOP-8776.patch
        2 kB
        Hemanth Yamijala
      2. HADOOP-8776.patch
        3 kB
        Hemanth Yamijala
      3. HADOOP-8776.patch
        3 kB
        Hemanth Yamijala
      4. HADOOP-8776.patch
        3 kB
        Chris Nauroth

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1230 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1230/)
          HADOOP-8776. Provide an option in test-patch that can enable/disable compiling native code. Contributed by Chris Nauroth. (Revision 1399857)

          Result = SUCCESS
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1399857
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1230 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1230/ ) HADOOP-8776 . Provide an option in test-patch that can enable/disable compiling native code. Contributed by Chris Nauroth. (Revision 1399857) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1399857 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1200 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1200/)
          HADOOP-8776. Provide an option in test-patch that can enable/disable compiling native code. Contributed by Chris Nauroth. (Revision 1399857)

          Result = SUCCESS
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1399857
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1200 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1200/ ) HADOOP-8776 . Provide an option in test-patch that can enable/disable compiling native code. Contributed by Chris Nauroth. (Revision 1399857) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1399857 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Yarn-trunk #8 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/8/)
          HADOOP-8776. Provide an option in test-patch that can enable/disable compiling native code. Contributed by Chris Nauroth. (Revision 1399857)

          Result = FAILURE
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1399857
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Yarn-trunk #8 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/8/ ) HADOOP-8776 . Provide an option in test-patch that can enable/disable compiling native code. Contributed by Chris Nauroth. (Revision 1399857) Result = FAILURE suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1399857 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk-Commit #2886 (See https://builds.apache.org/job/Hadoop-trunk-Commit/2886/)
          HADOOP-8776. Provide an option in test-patch that can enable/disable compiling native code. Contributed by Chris Nauroth. (Revision 1399857)

          Result = SUCCESS
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1399857
          Files :

          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-trunk-Commit #2886 (See https://builds.apache.org/job/Hadoop-trunk-Commit/2886/ ) HADOOP-8776 . Provide an option in test-patch that can enable/disable compiling native code. Contributed by Chris Nauroth. (Revision 1399857) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1399857 Files : /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          Suresh Srinivas added a comment -

          BTW I forgot to explicitly add my +1 to the patch to the previous comment.

          Show
          Suresh Srinivas added a comment - BTW I forgot to explicitly add my +1 to the patch to the previous comment.
          Hide
          Suresh Srinivas added a comment -

          I committed the patch to trunk. Thank you Chris.

          Show
          Suresh Srinivas added a comment - I committed the patch to trunk. Thank you Chris.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 new or modified test files.

          +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 passed unit tests in .

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1647//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1647//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/12549738/HADOOP-8776.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +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 passed unit tests in . +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1647//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1647//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          Looks good to me.

          Show
          Colin Patrick McCabe added a comment - Looks good to me.
          Hide
          Chris Nauroth added a comment -

          I'm attaching an updated patch that incorporates the last round of feedback:

          1. The option is now --build-native=<bool> for increased flexibility.
          2. We now suppress -Drequire.test.lib when not building native.

          I have tested the following:

          1. OS X: default builds native
          2. OS X: --build-native=true builds native
          3. OS X: --build-native=false --run-tests skips building native and runs tests
          4. Ubuntu: default builds native
          5. Ubuntu: --run-tests builds native and runs tests

          How does this look?

          Show
          Chris Nauroth added a comment - I'm attaching an updated patch that incorporates the last round of feedback: 1. The option is now --build-native=<bool> for increased flexibility. 2. We now suppress -Drequire.test.lib when not building native. I have tested the following: 1. OS X: default builds native 2. OS X: --build-native=true builds native 3. OS X: --build-native=false --run-tests skips building native and runs tests 4. Ubuntu: default builds native 5. Ubuntu: --run-tests builds native and runs tests How does this look?
          Hide
          Jianbin Wei added a comment -

          I agree this needs to be done and merged into trunk.

          Another developer's time was wasted.

          Show
          Jianbin Wei added a comment - I agree this needs to be done and merged into trunk. Another developer's time was wasted.
          Hide
          Chris Nauroth added a comment -

          I'm interested in getting a solution to this, as I just accidentally repeated this work on HADOOP-8933 (now closed as duplicate). Let me know if it's helpful for me to work on this patch, code review, test, or anything else.

          Thanks,
          --Chris

          Show
          Chris Nauroth added a comment - I'm interested in getting a solution to this, as I just accidentally repeated this work on HADOOP-8933 (now closed as duplicate). Let me know if it's helpful for me to work on this patch, code review, test, or anything else. Thanks, --Chris
          Hide
          Colin Patrick McCabe added a comment -
          • I think we should call the flag --build-native, and have it take true or false as an argument. That way it will be useful in more cases. For example, we may want to verify that the unit tests pass on Linux when the native libraries are not present.
          • remember to rebase on trunk now that HDFS-3753 is in. You'll need to avoid passing -Drequire.test.libhadoop when the native build is disabled.
          Show
          Colin Patrick McCabe added a comment - I think we should call the flag --build-native , and have it take true or false as an argument. That way it will be useful in more cases. For example, we may want to verify that the unit tests pass on Linux when the native libraries are not present. remember to rebase on trunk now that HDFS-3753 is in. You'll need to avoid passing -Drequire.test.libhadoop when the native build is disabled.
          Hide
          Jianbin Wei added a comment -

          Now test-patch will be platform specific. Ideally, it must be tested on all platforms before committing.

          Yes I agree completely. As you said, the core is to fix it on "broken" platforms. This is just a workaround.

          It has more logic (either --enable-native is passed OR it is a supported platform, etc.). Not earth-shattering, but still needs to be understood in context by someone fresh looking at it.

          I would say only the developer to fix the script will need to understand the extra complexity. So it should be transparent to most other developers.

          The dependency it will put on us to have to track removing this once native compile is fixed. Given that we cannot rely on contributors being always there and watchful, I am worried that someone will forget to document in a related JIRA to fix test-patch and we will end up in a situation that native compile is fixed, and test-patch isn't testing it.

          The developer who disables the test needs to

          • file a ticket to get "broken" platform fixed (if it is not there yet) AND
          • document the needs to enable the test in the ticket. In this case if it would be you

          Can you also print some messages to notify others that the native compilation is disabled due to issues?

          My goal is to provide best experiences for both end users and developers.

          Other thoughts?

          Show
          Jianbin Wei added a comment - Now test-patch will be platform specific. Ideally, it must be tested on all platforms before committing. Yes I agree completely. As you said, the core is to fix it on "broken" platforms. This is just a workaround. It has more logic (either --enable-native is passed OR it is a supported platform, etc.). Not earth-shattering, but still needs to be understood in context by someone fresh looking at it. I would say only the developer to fix the script will need to understand the extra complexity. So it should be transparent to most other developers. The dependency it will put on us to have to track removing this once native compile is fixed. Given that we cannot rely on contributors being always there and watchful, I am worried that someone will forget to document in a related JIRA to fix test-patch and we will end up in a situation that native compile is fixed, and test-patch isn't testing it. The developer who disables the test needs to file a ticket to get "broken" platform fixed (if it is not there yet) AND document the needs to enable the test in the ticket. In this case if it would be you Can you also print some messages to notify others that the native compilation is disabled due to issues? My goal is to provide best experiences for both end users and developers. Other thoughts?
          Hide
          Hemanth Yamijala added a comment -

          Hi, I have a patch that implements what is discussed in the comments above. Roughly, it inverts the option from the earlier patch, i.e. the user can now pass --enable-native to enable native compilation. If that flag isn't passed, we check based on uname if the platform is one on which native compilation is supported and enable the native profile based on that.

          However:

          I still am not convinced about the value this is adding. Please note that I am not talking about the complexity to implement the patch. I am concerned about the complexity the patch will introduce after it is committed.

          Specifically:

          • Now test-patch will be platform specific. Ideally, it must be tested on all platforms before committing.
          • It has more logic (either --enable-native is passed OR it is a supported platform, etc.). Not earth-shattering, but still needs to be understood in context by someone fresh looking at it.
          • The dependency it will put on us to have to track removing this once native compile is fixed. Given that we cannot rely on contributors being always there and watchful, I am worried that someone will forget to document in a related JIRA to fix test-patch and we will end up in a situation that native compile is fixed, and test-patch isn't testing it.

          IMHO, with the simpler --disable-native option, a developer will struggle initially like I did to figure out how to make test-patch work in spite of broken native compiles, figure out how to get around it, and then remember it for future. Agreed that there is overhead on all developers on the unsupported platforms initially, but I feel that is outweighed by the simplicity and clarity of that option.

          Jianbin, please let me know what you feel. It would be good for others watching the JIRA to weigh in as well. If it is felt otherwise, I will put up my new patch for review.

          Show
          Hemanth Yamijala added a comment - Hi, I have a patch that implements what is discussed in the comments above. Roughly, it inverts the option from the earlier patch, i.e. the user can now pass --enable-native to enable native compilation. If that flag isn't passed, we check based on uname if the platform is one on which native compilation is supported and enable the native profile based on that. However: I still am not convinced about the value this is adding. Please note that I am not talking about the complexity to implement the patch. I am concerned about the complexity the patch will introduce after it is committed. Specifically: Now test-patch will be platform specific. Ideally, it must be tested on all platforms before committing. It has more logic (either --enable-native is passed OR it is a supported platform, etc.). Not earth-shattering, but still needs to be understood in context by someone fresh looking at it. The dependency it will put on us to have to track removing this once native compile is fixed. Given that we cannot rely on contributors being always there and watchful, I am worried that someone will forget to document in a related JIRA to fix test-patch and we will end up in a situation that native compile is fixed, and test-patch isn't testing it. IMHO, with the simpler --disable-native option, a developer will struggle initially like I did to figure out how to make test-patch work in spite of broken native compiles, figure out how to get around it, and then remember it for future. Agreed that there is overhead on all developers on the unsupported platforms initially, but I feel that is outweighed by the simplicity and clarity of that option. Jianbin, please let me know what you feel. It would be good for others watching the JIRA to weigh in as well. If it is felt otherwise, I will put up my new patch for review.
          Hide
          Jianbin Wei added a comment -

          In hadoop-common-project/hadoop-common/src/main/conf/hadoop-env.sh the platform check is done through

          MAC_OSX=false
          case "`uname`" in
          Darwin*) MAC_OSX=true;;
          esac
          if $MAC_OSX; then
          export HADOOP_OPTS="$HADOOP_OPTS -Djava.security.krb5.realm= -Djava.security.krb5.kdc="
          fi

          Show
          Jianbin Wei added a comment - In hadoop-common-project/hadoop-common/src/main/conf/hadoop-env.sh the platform check is done through MAC_OSX=false case "`uname`" in Darwin*) MAC_OSX=true;; esac if $MAC_OSX; then export HADOOP_OPTS="$HADOOP_OPTS -Djava.security.krb5.realm= -Djava.security.krb5.kdc=" fi
          Hide
          Colin Patrick McCabe added a comment -

          I don't have a strong opinion about this issue. However, I will note that you can determine if the platform is Linux quite easily:

          if uname -o | grep -q Linux; then
            ... set linux-specific defaults...
          fi
          

          So your patch shouldn't take an hour to prepare if you do choose to go this route
          Of course you'll probably want to add an --enable-native if you do choose to do this.

          Show
          Colin Patrick McCabe added a comment - I don't have a strong opinion about this issue. However, I will note that you can determine if the platform is Linux quite easily: if uname -o | grep -q Linux; then ... set linux-specific defaults... fi So your patch shouldn't take an hour to prepare if you do choose to go this route Of course you'll probably want to add an --enable-native if you do choose to do this.
          Hide
          Jianbin Wei added a comment -

          Hemanth,

          I agree that the core issue is the broken native compilation and that should be fixed.

          My suggestion is to disable the native compilation for "all" broken platforms by default, file tickets to have them fixed and enable native compilation as part of the fix.

          My purpose is to make it more convenient/efficient for other developers.

          Every time I generate a patch, I do the patch test and it fails due to javac warning. I had to check if it is because of native compilation or not. The whole process for compilation and checking takes about 3 minutes.

          If we can save this 3 minutes for each patch/developer, we may easily save thousands of minutes before the native compilation is fixed. This should justify a slightly more complicated workaround that may take you 60 minutes.

          Just my 2 cents.

          Show
          Jianbin Wei added a comment - Hemanth, I agree that the core issue is the broken native compilation and that should be fixed. My suggestion is to disable the native compilation for "all" broken platforms by default, file tickets to have them fixed and enable native compilation as part of the fix. My purpose is to make it more convenient/efficient for other developers. Every time I generate a patch, I do the patch test and it fails due to javac warning. I had to check if it is because of native compilation or not. The whole process for compilation and checking takes about 3 minutes. If we can save this 3 minutes for each patch/developer, we may easily save thousands of minutes before the native compilation is fixed. This should justify a slightly more complicated workaround that may take you 60 minutes. Just my 2 cents.
          Hide
          Hemanth Yamijala added a comment -

          Jianbin, Thanks for your interest.

          I was told that native compilation / functionality is not working on Solaris as well. So, it looks like the issue is not just MacOS specific.

          The way I see this, the core issue is with native compilation being broken. This JIRA is just providing a workaround that will serve a (hopefully) temporary purpose. I'd rather keep the workaround simple (given that there are other platforms as well that might need to be considered). Please let me know if you are fine with this.

          Show
          Hemanth Yamijala added a comment - Jianbin, Thanks for your interest. I was told that native compilation / functionality is not working on Solaris as well. So, it looks like the issue is not just MacOS specific. The way I see this, the core issue is with native compilation being broken. This JIRA is just providing a workaround that will serve a (hopefully) temporary purpose. I'd rather keep the workaround simple (given that there are other platforms as well that might need to be considered). Please let me know if you are fine with this.
          Hide
          Jianbin Wei added a comment -

          I agree that it is needed. But I would agree more with

          default it to true on Linux, and false on MacOS

          We can put in HADOOP-8744 that this needs to be enabled again by default.

          It is much more convenient and error-proof.

          Show
          Jianbin Wei added a comment - I agree that it is needed. But I would agree more with default it to true on Linux, and false on MacOS We can put in HADOOP-8744 that this needs to be enabled again by default. It is much more convenient and error-proof.
          Hide
          Tom White added a comment -

          +1 I tried the modified test-patch on a Mac with the --disable-native flag and it worked fine.

          Show
          Tom White added a comment - +1 I tried the modified test-patch on a Mac with the --disable-native flag and it worked fine.
          Hide
          Hemanth Yamijala added a comment -

          Eli / Thomas, would it be possible for one of you to take a look and see if its fine ? This patch is helping a few of us develop patches from Mac.

          Show
          Hemanth Yamijala added a comment - Eli / Thomas, would it be possible for one of you to take a look and see if its fine ? This patch is helping a few of us develop patches from Mac.
          Hide
          Hemanth Yamijala added a comment -

          Thanks for the review Colin.

          Show
          Hemanth Yamijala added a comment - Thanks for the review Colin.
          Hide
          Colin Patrick McCabe added a comment -

          Looks good to me.

          Show
          Colin Patrick McCabe added a comment - Looks good 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/12546801/HADOOP-8776.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 1 new or modified test files.

          +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 passed unit tests in .

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1536//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1536//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/12546801/HADOOP-8776.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +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 passed unit tests in . +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1536//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1536//console This message is automatically generated.
          Hide
          Hemanth Yamijala added a comment -

          As per discussion, uploading a patch that leaves the native compile enabled by default, and removes the hint from the earlier patch.

          Show
          Hemanth Yamijala added a comment - As per discussion, uploading a patch that leaves the native compile enabled by default, and removes the hint from the earlier patch.
          Hide
          Colin Patrick McCabe added a comment -

          Given that you are fixing the native build, I assume we would like to enable native compile by default on MacOS too after that. Given that, I feel like leaving it enabled by default like in the last patch.

          I would certainly like to review patches or suggest approaches to fix to the native Mac build. However, HADOOP-8744 is currently not assigned to me. It's not likely to be me personally who fixes it because I don't have a Mac

          Since MacOS is a POSIX platform, we generally try to support it by making our native code POSIX-compliant (or at least able to work under POSIX when advanced features are not detected.)

          Feels like this would be an option to keep things going for now, and not need any further changes once things get better with the native build. Thoughts ?

          Sounds reasonable to me. My main objection to the previous patch was the hint text, as I said.

          Show
          Colin Patrick McCabe added a comment - Given that you are fixing the native build, I assume we would like to enable native compile by default on MacOS too after that. Given that, I feel like leaving it enabled by default like in the last patch. I would certainly like to review patches or suggest approaches to fix to the native Mac build. However, HADOOP-8744 is currently not assigned to me. It's not likely to be me personally who fixes it because I don't have a Mac Since MacOS is a POSIX platform, we generally try to support it by making our native code POSIX-compliant (or at least able to work under POSIX when advanced features are not detected.) Feels like this would be an option to keep things going for now, and not need any further changes once things get better with the native build. Thoughts ? Sounds reasonable to me. My main objection to the previous patch was the hint text, as I said.
          Hide
          Hemanth Yamijala added a comment -

          And default it to true on Linux, and false on MacOS? You can use uname to find the operating system name in the script. What do you think?

          Given that you are fixing the native build, I assume we would like to enable native compile by default on MacOS too after that. Given that, I feel like leaving it enabled by default like in the last patch.

          We can remove the hint, I agree that it seems out of place. If the developer gets a failure due to native build, he could probably look at the test-patch script to see if there's an option to turn it off and find it. We could probably also document it on Wiki etc.

          Feels like this would be an option to keep things going for now, and not need any further changes once things get better with the native build. Thoughts ?

          Show
          Hemanth Yamijala added a comment - And default it to true on Linux, and false on MacOS? You can use uname to find the operating system name in the script. What do you think? Given that you are fixing the native build, I assume we would like to enable native compile by default on MacOS too after that. Given that, I feel like leaving it enabled by default like in the last patch. We can remove the hint, I agree that it seems out of place. If the developer gets a failure due to native build, he could probably look at the test-patch script to see if there's an option to turn it off and find it. We could probably also document it on Wiki etc. Feels like this would be an option to keep things going for now, and not need any further changes once things get better with the native build. Thoughts ?
          Hide
          Colin Patrick McCabe added a comment -

          The hint seems a little out of place. There are a lot of reasons why compilation might fail... the fact that the native build broke is only one.

          Hmm. Maybe we should have an option like

          --enable-native={true|false}
          

          And default it to true on Linux, and false on MacOS? You can use uname to find the operating system name in the script. What do you think?

          While working on the patch, I saw that we don't do a native compile when we baseline trunk (without the patch). I don't know what the reason for this is, but one option could be to actually start native compile with baseline and if that fails, we are sure it is not because of the patch. Thoughts ?

          I have some fixes in the works which will make it more obvious when the native build fails. For now, let's not do anything that would slow down test-patch.sh.

          Show
          Colin Patrick McCabe added a comment - The hint seems a little out of place. There are a lot of reasons why compilation might fail... the fact that the native build broke is only one. Hmm. Maybe we should have an option like --enable- native ={ true | false } And default it to true on Linux, and false on MacOS? You can use uname to find the operating system name in the script. What do you think? While working on the patch, I saw that we don't do a native compile when we baseline trunk (without the patch). I don't know what the reason for this is, but one option could be to actually start native compile with baseline and if that fails, we are sure it is not because of the patch. Thoughts ? I have some fixes in the works which will make it more obvious when the native build fails. For now, let's not do anything that would slow down test-patch.sh.
          Hide
          Hemanth Yamijala added a comment -

          Attaching a patch flipping the choice of native compilation to be on by default. And providing a hint if compilation with the patch fails.

          While working on the patch, I saw that we don't do a native compile when we baseline trunk (without the patch). I don't know what the reason for this is, but one option could be to actually start native compile with baseline and if that fails, we are sure it is not because of the patch. Thoughts ?

          Show
          Hemanth Yamijala added a comment - Attaching a patch flipping the choice of native compilation to be on by default. And providing a hint if compilation with the patch fails. While working on the patch, I saw that we don't do a native compile when we baseline trunk (without the patch). I don't know what the reason for this is, but one option could be to actually start native compile with baseline and if that fails, we are sure it is not because of the patch. Thoughts ?
          Hide
          Hemanth Yamijala added a comment -

          Hi Colin, Thanks for your comments. I guess what you're suggesting is to not disable the compile of native code by default. But instead, provide a switch to not compile native code if required. Ultimately when HADOOP-8744 is fixed, contributors can just stop using the flag.

          My rationale for this JIRA is that without an option like this, we are actually preventing (some) contributors from testing anything locally because test-patch simply fails. For the same reason, I made the default option to switch off native compile.

          One thing we could do is to enable native compile by default, but print a message hinting to use the disable flag in case compile failed in test-patch. I will work on that and provide a new patch.

          Show
          Hemanth Yamijala added a comment - Hi Colin, Thanks for your comments. I guess what you're suggesting is to not disable the compile of native code by default. But instead, provide a switch to not compile native code if required. Ultimately when HADOOP-8744 is fixed, contributors can just stop using the flag. My rationale for this JIRA is that without an option like this, we are actually preventing (some) contributors from testing anything locally because test-patch simply fails. For the same reason, I made the default option to switch off native compile. One thing we could do is to enable native compile by default, but print a message hinting to use the disable flag in case compile failed in test-patch. I will work on that and provide a new patch.
          Hide
          Colin Patrick McCabe added a comment -

          In the end, we don't want to submit code unless it has been tested with the full test-patch.sh, including the native compilation. The purpose of test-patch.sh is to identify regressions, and that purpose is defeated if it doesn't test as much as possible.

          However, it seems reasonable to provide various switches to skip various parts of the testing, including the native testing, as long as the default is to test everything.

          Show
          Colin Patrick McCabe added a comment - In the end, we don't want to submit code unless it has been tested with the full test-patch.sh, including the native compilation. The purpose of test-patch.sh is to identify regressions, and that purpose is defeated if it doesn't test as much as possible. However, it seems reasonable to provide various switches to skip various parts of the testing, including the native testing, as long as the default is to test everything.
          Hide
          Hemanth Yamijala added a comment -

          The attached patch adds an option --compile-native to the test-patch script. If passed, it enables the native profile wherever it is being currently used.

          This approach (same as what Eli suggested on the mailing list) changes how test-patch currently works, though. In that, by default native code will not be compiled. Given that the most common use case will be around Java patches, I suppose it is OK.

          But would be glad to discuss other options as required.

          Show
          Hemanth Yamijala added a comment - The attached patch adds an option --compile-native to the test-patch script. If passed, it enables the native profile wherever it is being currently used. This approach (same as what Eli suggested on the mailing list) changes how test-patch currently works, though. In that, by default native code will not be compiled. Given that the most common use case will be around Java patches, I suppose it is OK. But would be glad to discuss other options as required.

            People

            • Assignee:
              Chris Nauroth
              Reporter:
              Hemanth Yamijala
            • Votes:
              1 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development