Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix cdf5 configure option #1033

Merged
merged 4 commits into from
Jun 28, 2018
Merged

fix cdf5 configure option #1033

merged 4 commits into from
Jun 28, 2018

Conversation

wkliao
Copy link
Contributor

@wkliao wkliao commented Jun 28, 2018

This PR fixes configure/cmake that fails to disable CDF5 on 64-bit machines when user explicitly disables CDF5. It also errors out when user explicitly enable CDF5 on 32-bit machines (size_t being of size 4 bytes).

configure.ac Outdated
@@ -888,18 +888,20 @@ AC_CHECK_SIZEOF(unsigned long long)
# Check whether we want to enable CDF5 support.
AC_MSG_CHECKING([whether CDF5 support should be enabled])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to "whether CDF5 support should be disabled"

Copy link
Collaborator

@DennisHeimbigner DennisHeimbigner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you get rid of the line

-OPTION(ENABLE_CDF5 "Enable CDF5 Support." ON)
isn't there a cmake command to list the available options (like configure --help)? This would mean
that ENABLE_CDF5 will not show up as an explicit option?

@wkliao
Copy link
Contributor Author

wkliao commented Jun 28, 2018

If we kept that line, then ENABLE_CDF5 will be set to either ON or OFF. Unlike autoconf which allows us to set to a customized value, AUTO in this case, meaning user did not explicitly set this option.

I am not aware cmake has a similar command-line option to --help that shows all available options. If you know a way to do so, I am interested to learn.

@DennisHeimbigner
Copy link
Collaborator

cmake -L
althought it is a bit noisy.

@wkliao
Copy link
Contributor Author

wkliao commented Jun 28, 2018

Do people actually run that command to check all available options?
I don't know what to add to achieve the same effect as OPTION, if we want to know whether user explicitly enable or disable an option.

@DennisHeimbigner
Copy link
Collaborator

I have used it but rarely. I suspect however, that people do peruse
a CMakeLists.txt looking for option() commands.

Copy link
Collaborator

@DennisHeimbigner DennisHeimbigner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think understand. I would only suggest
that you leave the option() command at line
at line 1290, but commented out with a short
explanation of the fact that the option command does not allow a way to test if the user actually
set the flag or not.

@wkliao
Copy link
Contributor Author

wkliao commented Jun 28, 2018

This commit should achieve the same effect of using OPTION.
The trick learned from
https://cmake.org/pipermail/cmake/2016-October/064342.html

@DennisHeimbigner
Copy link
Collaborator

That seems like a good solution. Wonder why the cmake developers
did not support tristate from the get-go. Oh well.

@@ -12,19 +12,19 @@ services:
env:
matrix:
# Ubuntu
- DOCKIMG=unidata/nctests:serial USECMAKE=TRUE USEAC=TRUE DISTCHECK=TRUE USE_CC=gcc AC_COPTS='CFLAGS=-fsigned-char --disable-netcdf-4 --disable-dap-remote-tests --enable-cdf5' COPTS='-DENABLE_NETCDF_4=OFF -DCMAKE_C_FLAGS=-fsigned-char -DENABLE_DAP_REMOTE_TESTS=OFF -DENABLE_CDF5=TRUE' USECP=FALSE CURHOST=docker-gcc-x64-signed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter so we will accept this, but as a point of reference cmake will support 'ON/OFF' and 'TRUE/FALSE' interchangeably.

@WardF WardF merged commit 0af56bf into Unidata:master Jun 28, 2018
@wkliao
Copy link
Contributor Author

wkliao commented Jun 28, 2018

I thought it was interchangeable, but when I used -DENABLE_CDF5=FALSE, it does not disable CDF5. Adding a MESSAGE command to print out the value of ENABLE_CDF5 and I got its value ON. Please give it a try. I hope it is not just me. (Note this applies only to the master branch before my PR was merged.)

@wkliao
Copy link
Contributor Author

wkliao commented Jun 29, 2018

In addition, my PR changed the type of ENABLE_CDF5 from BOOL to STRING. So using TRUE/FALSE will fail.

@WardF
Copy link
Member

WardF commented Jun 29, 2018

Using -DENABLE_[WHATEVER OPTION]=FALSE disables the corresponding feature on my system. I just tested -DENABLE_NETCDF_4=FALSE, and saw netcdf4 API support turn off. I saw similar behavior when testing -DENABLE_CDF5=FALSE, before accepting the pull request; I tested before making my comment.

@WardF
Copy link
Member

WardF commented Jun 29, 2018

We can see from the cmake documentation found at https://cmake.org/cmake/help/v3.12/command/if.html that OFF and FALSE are treated as the same. We will need to update the new logic in CMakeLists.txt so that it respects valid CMake boolean specifiers as users would expect, before the next release.

@WardF
Copy link
Member

WardF commented Jun 29, 2018

I am seeing test failures on ARM that exist with this merge. I am going to revert this pull request for now as I'm about to go out of town and won't be available to debug this. We can reevaluate this pull request after I return, and figure out what is going on.

Failures can be seen here:

wkliao added a commit to wkliao/netcdf-c that referenced this pull request Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants