Skip to content

Add configure options: --with-hdf5 --with-hdf4 and --with-pnetcdf#1396

Closed
wkliao wants to merge 1 commit intoUnidata:mainfrom
wkliao:configure_with
Closed

Add configure options: --with-hdf5 --with-hdf4 and --with-pnetcdf#1396
wkliao wants to merge 1 commit intoUnidata:mainfrom
wkliao:configure_with

Conversation

@wkliao
Copy link
Copy Markdown
Contributor

@wkliao wkliao commented May 15, 2019

This is to support #1391

@DennisHeimbigner
Copy link
Copy Markdown
Collaborator

I am not sure I want to revisit this. This a bit like the fortran separation
and people asking us to undo it.
We went to some effort to do this split and I do not see enough complaints
about this to warrant the change. Still, we can continue to talk about this
and leave the PR open for now.

@wkliao
Copy link
Copy Markdown
Contributor Author

wkliao commented May 16, 2019

Just for the reference. The commit that removed --with-hdf5 etc. is:
29e8619#diff-67e997bcfdac55191033d57a16d1408a
It was committed by @edhartnett in 2011 with log message "simplified build system".
This PR is just for connivence. I cannot think of any harm it may cause.

@DennisHeimbigner
Copy link
Copy Markdown
Collaborator

Do we also need to add --with-curl, --with-szip, etc?

@wkliao
Copy link
Copy Markdown
Contributor Author

wkliao commented May 17, 2019

I believe HDF5 is doing this for every external library.

@edhartnett
Copy link
Copy Markdown
Contributor

Personally I am not a huge fan of this approach, although I originally introduced these for netcdf (also based on the HDF5 example), then later removed them.

If you read the AC_ARG_WITH documentation (https://www.gnu.org/software/autoconf/manual/autoconf-2.60/html_node/External-Software.html) you will see that this usage is not mentioned. As the autoconf manual describes this macro, it is to be used for external libraries that may be optional in your build. The example they give is the readline library. A --without-readline may cause a package that optionally uses the readline library to not use it, even if it is present on the system.

In this way, we could use --without-hdf5/--with-hdf5 to turn off and on support for HDF5.

But the autotools docs mention there may be cases where AC_ARG_ENABLE is used to handle this case instead (as we have done). So far, so good.

Nowhere does the documentation mention using the --with-* option to specify the location of the library. This is already well-handled by the standard CPPFLAGS, LDFLAGS, etc.

The HDF5 team use it this way, but is that necessary or correct?

@DennisHeimbigner
Copy link
Copy Markdown
Collaborator

I wish you had documented the reasoning when you made the change
all those years ago. Oh well.

@edhartnett
Copy link
Copy Markdown
Contributor

Well if I had documented every decision I would have left you with 1000 volumes of detail you could never read. ;-)

@DennisHeimbigner
Copy link
Copy Markdown
Collaborator

Sure we would! But only when we needed it :-)

@wkliao
Copy link
Copy Markdown
Contributor Author

wkliao commented Jul 1, 2019

My suggested option does not affect the results of --without-package.
If it is used, autoconf will set variable with_package to no.
if --with-package is used without argument, then variable with_package is set to yes.

This PR is for convenience, and it does not breaking existing practical usage of autoconf.
In addition to HDF5, bot MPICH and OpenMPI use it.

@DennisHeimbigner
Copy link
Copy Markdown
Collaborator

Next question: should we advertise this option, or deprecate it to try to
force users to use the existing LDFLAGS, etc.

@wkliao
Copy link
Copy Markdown
Contributor Author

wkliao commented Jul 1, 2019

Maybe we can mention this option to the netcdf community to see if it is welcomed.
If someone pointed out it can cause harm, then we can abandon it.

@WardF WardF added this to the 4.7.2 milestone Aug 6, 2019
@WardF WardF modified the milestones: 4.7.2, 4.7.3 Oct 28, 2019
@edhartnett
Copy link
Copy Markdown
Contributor

I recommend that this PR be closed and not merged.

This usage of --with-hdf5 here is not in accordance with the automake documentation.

Furthermore, as noted by @DennisHeimbigner, this would imply that we should do the same for all the other libraries we use, and I don't think we want to do that. We would be adding many lines of code to our configure.ac file, for a non-standard way of setting library locations, which we can already do in a standard way. (Also we would be increasing our test burden, since we would have to test these options.)

Although there are some packages that use the --with options this way (ex. HDF5) I believe they are incorrect. And there are many many more packages that to NOT use the --with options in this way.

The documentation is clear, this is not how the --with option is intended to be used. LDFLAGS and CPPFLAGS already handle this functionality in a very well-understood and standard way.

I apologize to @wkliao and all the netCDF team for not better documenting the decision to remove these from netcdf-c configure.ac. But I believe it was the correct decision, and these should not be put back in.

I would also like to add that I very much value your contributions and efforts @wkliao , both with pnetcdf and netcdf-c. Please understand that, although I don't think this PR should be merged, I believe that you bring great value to the scientific data world with your many contributions. ;-)

@WardF WardF modified the milestones: 4.7.3, 4.8.0 Mar 27, 2020
@WardF WardF modified the milestones: 4.8.0, 4.8.2 Aug 30, 2021
@WardF WardF closed this Mar 10, 2022
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.

4 participants