-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[vcpkg][gdal] Switch build method to vcpkg_*_nmake(win)/vcpkg_*_make(unix) #9100
Conversation
…nt `all` [gdal]Switch build method to vcpkg_*_nmake(win)/vcpkg_*_make(unix)
@Neumann-A This PR should fix your issue. Can you review it? Thanks. |
@Neumann-A You are right, I found that almost all features were broken. Mostly because the dependent libraries were not added to the test case. |
@JackBoosY You should probably ask @lakinwecker to check this PR. |
@Neumann-A In the end, I found that the order of ports in gdal configure/configure.ac is incorrect. We can only solve these configuration problems by adding |
Current status:
|
The CI system did not really test gdal, we need to fix it before merging this PR. |
shouldn't those be external? (If I understand external correctly)
|
@Neumann-A Because these 6 ports are not added as functions. This PR only fixes existing features. |
|
Hey @JackBoosY. We like |
Explanation of common autoconf-style arguments: https://autotools.io/autoconf/arguments.html It looks like "enable" is typically paired with "disable" and "with" is paired with "without". |
@ras0219-msft According to the documentation, with should be paired with without, but in general, the judgment in configure.ac is:
or
|
@JackBoosY gdal[core] (commit
and help in
Here additional features omitted, libgeotiff - internal renamed, jpeg - external, qhull - disabled (all just for quick test). |
@JackBoosY small typo fix
|
/azp run |
|
||
-if test "$with_liblzma" = "yes" ; then | ||
+if test "$with_liblzma" != "no" ; then | ||
AC_CHECK_LIB(lzma,lzma_code,LIBLZMA_SETTING=yes,LIBLZMA_SETTING=no,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use AC_CHECK_LIB due to possible debug suffixes. Instead use AC_SEARCH_LIBS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are too many AC_CHECK_LIB
in the source, so I think we should keep it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean you can also set the autotool variable by hand and pass it to configure if you feel like it but it general all those check libs calls need to be replaced with search lib calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Neumann-A As my knowledge of configure is limited, can you help me do this?
|
||
- if test "$LIBLZMA_SETTING" = "yes" -a "$ac_cv_header_lzma_h" = "yes" ; then | ||
+ if test "$LIBLZMA_SETTING" = "yes" ; then | ||
LIBS="-llzma $LIBS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't link by name. Use $ac_cv_search_(function) (in this case $ac_cv_search_lzma_code). Requires a check if $ac_cv_search_(function)
is not equal to none required
. Compare with the libwandio changes in 2e711ae
AC_CHECK_LIB(hdf5,H5Fopen,HAVE_HDF5=yes,HAVE_HDF5=no,) | ||
|
||
if test "$HAVE_HDF5" = "yes" ; then | ||
- LIBS="-lhdf5 $LIBS" | ||
+ LIBS="-lhdf5_hl -lhdf5 -lszip $LIBS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
I won't comment on all the AC_CHECK_LIB
and named links below but those should be changed as described above.
if test "$TIFF_SETTING" = "external" ; then | ||
- LIBS="-ltiff $LIBS" | ||
+ LIBS="-ltiff -llzma $LIBS" | ||
AC_MSG_RESULT([using pre-installed libtiff.]) | ||
else | ||
AC_MSG_RESULT([using internal TIFF code.]) | ||
@@ -1667,10 +1666,10 @@ else | ||
|
||
TIFF_SETTING=external | ||
if test -r "$with_libtiff/tiff.h" ; then | ||
- LIBS="-L$with_libtiff -ltiff $LIBS" | ||
+ LIBS="-L$with_libtiff -ltiff -llzma $LIBS" | ||
EXTRA_INCLUDES="-I$with_libtiff $EXTRA_INCLUDES" | ||
else | ||
- LIBS="-L$with_libtiff/lib -ltiff $LIBS" | ||
+ LIBS="-L$with_libtiff/lib -ltiff -llzma $LIBS" | ||
EXTRA_INCLUDES="-I$with_libtiff/include $EXTRA_INCLUDES" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a search for tiff, Why is lzma linked in again? It should already be a part of LIBS
elif test "$with_netcdf" = "yes" -o "$with_netcdf" = "" ; then | ||
|
||
- AC_CHECK_LIB([netcdf], [nc_open], [NETCDF_SETTING=yes], [NETCDF_SETTING=no],) | ||
+ AC_CHECK_LIB([netcdf], [nc_open], [NETCDF_SETTING=yes], [NETCDF_SETTING=no], -lcurl -lssl -lcrypto) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curl has a debug suffix so this won't work in debug builds. Better do a search for CURL.
This is too hard for me, close it temporary. |
This PR will:
BUILD_TARGET
tovcpkg_*_nmake
to select target.MAKEFILE
tovcpkg_build_make
to select the makefile.vcpkg_use_system_ports
to use configure's option to handle vcpkg installed ports.vcpkg_*_nmake
(Windows) /vcpkg_*_make
(UNIX).Related: #9068.