Skip to content

Commit

Permalink
Fix typo, exploring solution to #1324
Browse files Browse the repository at this point in the history
  • Loading branch information
@WardF
WardF committed Feb 27, 2019
1 parent e906114 commit f940969
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion nc-config.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ Available values for OPTION include:
--has-parallel4 whether has parallel IO support via HDF5
--has-parallel whether has parallel IO support via HDF5 or PnetCDF
--libs library linking information for netcdf
--prefix Install prefixx
--prefix Install prefix
--includedir Include directory
--libdir Library directory
--version Library version
Expand Down
2 changes: 1 addition & 1 deletion netcdf.pc.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Description: NetCDF Client Library for C
URL: http://www.unidata.ucar.edu/netcdf
Version: @PACKAGE_VERSION@
Libs: -L${libdir} @NC_LIBS@
Libs.private: @LIBS@
Cflags: -I${includedir}

12 comments on commit f940969

@Dave-Allured
Copy link
Contributor

Choose a reason for hiding this comment

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

I cloned and tested v4.6.3-dev-branch-gh1324.wif with Cmake on Mac. Looks good, these values as expected in my result file netcdf.pc:

Libs: -L${libdir} -lnetcdf
Libs.private: -lhdf5_hl -lhdf5 -lz -ldl -lm -lcurl

However, what is going on with the package version number in the same netcdf.pc file? I will assume this is normal and will be corrected to 4.6.3 for the release:

Version: 4.6.4-development

@WardF
Copy link
Member Author

@WardF WardF commented on f940969 Feb 27, 2019 via email

Choose a reason for hiding this comment

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

@wkliao
Copy link
Contributor

@wkliao wkliao commented on f940969 Mar 8, 2019

Choose a reason for hiding this comment

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

Shouldn't @NC_LIBS@ in line 11 of file netcdf.pc.in be simply -lnetcdf?

@WardF
Copy link
Member Author

@WardF WardF commented on f940969 Mar 8, 2019

Choose a reason for hiding this comment

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

@wkliao yes, are you seeing something different?

@wkliao
Copy link
Contributor

@wkliao wkliao commented on f940969 Mar 8, 2019

Choose a reason for hiding this comment

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

I am seeing below in master branch (I was referring to the .in file).

netcdf-c/netcdf.pc.in

Line 11 in c0d24c8

Libs: -L${libdir} @NC_LIBS@

@Dave-Allured
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that @NC_LIBS@ was changed somewhere else at about the same time, to make it consistent between autotools and Cmake builds. It seems that @NC_LIBS@ is now always emitted as -lnetcdf only, and the extra library flags are now in @LIBS@ only. These both come from somewhere in configure scripts. It is all working well in my tests on release 4..6.3, both build systems. @WardF?

@wkliao
Copy link
Contributor

@wkliao wkliao commented on f940969 Mar 8, 2019

Choose a reason for hiding this comment

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

My understanding is because NetCDF C library only produces one library (libnetcdf.a and libnetcdf.so), it is safe to just use Libs: -L${libdir} -lnetcdf in netcdf.pc.in.

FYI. NC_LIBS is set in configure.ac and CMakeLists.txt.
When --disable-shared is set, it includes user's LIBS environment variable.

@wkliao
Copy link
Contributor

@wkliao wkliao commented on f940969 Mar 9, 2019

Choose a reason for hiding this comment

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

Since HDF5 is a dependent package of NetCDF, should the following be added to netcdf.pc.in?
Requires: hdf5

Similar for hdf4. if hdf5.pc and hdf4.pc are available.

@Dave-Allured
Copy link
Contributor

Choose a reason for hiding this comment

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

Requires vs. Libs does not have a simple answer. This would involve a deeper study of pkg-config rules. Meanwhile, this commit appears to solve the original overlinking problem, though that has not been verified yet by the original issue #1324 author.

I suspect there is room for further improvement to netcdf.pc.in along these lines. I don't plan to work on it any more, unless a new problem is reported.

@WardF
Copy link
Member Author

@WardF WardF commented on f940969 Mar 11, 2019

Choose a reason for hiding this comment

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

I agree there is more work to be done, and I will revisit it when I get the chance. I am inclined to leave the system as it is for now because, as @Dave-Allured notes, it does appear to be working consistently between the build systems. Hardcoding -lnetcdf into the template file at this point doesn't solve any problem that I've seen, so if it does change it will likely be concurrent with other changes that ultimately need to be made.

@wkliao
Copy link
Contributor

@wkliao wkliao commented on f940969 Mar 12, 2019

Choose a reason for hiding this comment

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

FYI. When I configured NetCDF with --disable-shared, I got the followings in netcdf.pc.

Libs: -L${libdir}  -lnetcdf -lm -lcurl 
Libs.private: -lm -lcurl

I assume this is OK for static-library build only. My configure command line is:

./configure --disable-netcdf-4 --disable-shared

@Dave-Allured
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is okay for now, for static builds. In @NC_LIBS@, it looks like configure is performing some flag grouping that should be reserved for later handling in pkg-config. But the current result is functional for all cases that I can see without digging.

Please sign in to comment.