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

Configure test broken for --with-winidn on mingw32 #1669

Closed
jeroen opened this Issue Jul 8, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@jeroen
Contributor

jeroen commented Jul 8, 2017

I am building a version of mingw-w64-curl with --with-winidn (PKGBUILD). It works great on mingw64 however when building in mingw32 I see:

checking whether versioned symbols are wanted... no
checking whether to enable Windows native IDN (Windows native builds only)... yes
checking if IdnToUnicode can be linked... no
configure: WARNING: Cannot find libraries for IDN support: IDN disabled

And the resulting binary then does:

* Rebuilt URL to: http://www.malmö.se/
* IDN support not present, can't parse Unicode domains
* timeout on name lookup is not supported
* getaddrinfo(3) failed for www.malmö.se:80
* Couldn't resolve host 'www.malmö.se'

The problem is that on 32bit IdnToUnicode is only available on Windows Vista and higher. Therefore it is only exposed if we compile with:

-D_WIN32_WINNT=0x0600 -DWINVER=0x0600

However the configure test seems to conflict with this macro. The config.log file shows:

configure:26798: checking whether to enable Windows native IDN (Windows native builds only)
configure:26816: result: yes
configure:26843: checking if IdnToUnicode can be linked
configure:26872: i686-w64-mingw32-gcc -o conftest.exe -D_WIN32_WINNT=0x0600 -DWINVER=0x0600 -Werror-implicit-function-declaration -O2 -Wno-system-headers -D_FORTIFY_SOURCE=2 -D__USE_MINGW_ANSI_STDIO=1 -IC:/msys32/mingw32/include  -pipe -LC:/msys32/mingw32/lib  conftest.c -lnormaliz -lcrypt32 -lwldap32 -lz -lws2_32  >&5
C:\msys32\tmp\cccptlBZ.o:conftest.c:(.text.startup+0xc): undefined reference to `IdnToUnicode'

I tried running exactly the same command on a simple test.c file which has a call to IdnToUnicode() and there it works fine. So the conftest.c as generated by autotools does not work with -DWINVER=0x0600:

if test "$want_winidn" = "yes"; then
    clean_CPPFLAGS="$CPPFLAGS"
  clean_LDFLAGS="$LDFLAGS"
  clean_LIBS="$LIBS"
  WINIDN_LIBS="-lnormaliz"
  #
  if test "$want_winidn_path" != "default"; then
            WINIDN_LDFLAGS="-L$want_winidn_path/lib$libsuff"
    WINIDN_CPPFLAGS="-I$want_winidn_path/include"
    WINIDN_DIR="$want_winidn_path/lib$libsuff"
  fi
  #
  CPPFLAGS="$CPPFLAGS $WINIDN_CPPFLAGS"
  LDFLAGS="$LDFLAGS $WINIDN_LDFLAGS"
  LIBS="$WINIDN_LIBS $LIBS"
  #
  { $as_echo "$as_me:${as_lineno-$LINENO}: checking if IdnToUnicode can be linked" >&5
$as_echo_n "checking if IdnToUnicode can be linked... " >&6; }
  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h.  */


#define IdnToUnicode innocuous_IdnToUnicode
#ifdef __STDC__
# include <limits.h>
#else
# include <assert.h>
#endif
#undef IdnToUnicode
#ifdef __cplusplus
extern "C"
#endif
char IdnToUnicode ();
#if defined __stub_IdnToUnicode || defined __stub___IdnToUnicode
choke me
#endif

int main (void)
{
return IdnToUnicode ();
 ;
 return 0;
}

Is a way to improve this test so that it works in conjunction with -D_WIN32_WINNT=0x0600 ?

@jtanx

This comment has been minimized.

Contributor

jtanx commented Jul 8, 2017

So as mentioned in Alexpux/MINGW-packages#2677, it's actually because AC_LINK_IFELSE doesn't account for IdnToUnicode having the stdcall calling convention (which affects only 32-bit systems).

The fix would posibly be:

  • To do something like https://github.com/libarchive/libarchive/blob/master/build/autoconf/check_stdcall_func.m4
  • Write a custom check for AC_CHECK_PROG
  • Something else in autoconf (??? I'm sure there are other options that I'm not aware of with autoconf)
  • Assume that IdnToUnicode exists on Windows. This is actually quite sane, because the minimum version that comes with this function is Windows Vista. You can even get it on XP if you install a redistributable, but hey, XP isn't supported... right? And if there's an explicit --with-idn option, then I'd say this option makes the most sense.
@bagder

This comment has been minimized.

Member

bagder commented Jul 10, 2017

XP isn't supported... right?

Not supported by whom? We still support building XP afaik, we have a not insignificant amount of users on XP and earlier Windows versions.

@bagder bagder added the build label Jul 10, 2017

@jtanx

This comment has been minimized.

Contributor

jtanx commented Jul 10, 2017

Not supported by whom?

curl officially supports XP and even older than that? :(

IMO, I'd say that while it's nice if it did support said platforms, you shouldn't have to go out of your way to support what is essentially a very old, and unmaintained OS.

And just to reiterate, I think it's fair to assume that if --with-idn is passed, and if it's being built on Windows, then IdnToUnicode is present. The option then for XP support is to not compile with --with-idn enabled.

@bagder

This comment has been minimized.

Member

bagder commented Jul 26, 2017

you shouldn't have to go out of your way to support

Right, and we don't go out of our way to support it. But we also don't purposely break what we already have working there.

(As for the exact fix for this issue, I don't build/run on windows so I won't write the fix since I can't really test it.)

@bagder bagder added the help wanted label Jul 29, 2017

@jeroen

This comment has been minimized.

Contributor

jeroen commented Jul 30, 2017

Best approach would be to skip the configure test if the user configures explicitly --with-winidn.

@bagder

This comment has been minimized.

Member

bagder commented Jul 30, 2017

Right, which then will make the build fail on windows versions before Vista, but we can of course just document that fact.

I'm curious though, as I was under the impression that some builds have actually been done in the past that had support for winidn while you're saying that it can't detect IdnToUnicode at all.

Possibly more important: is there other functions that configure tries to detect that it fails to detect due to this weirdness?

(I marked this PR-welcome as I personally will not patch this problem as I don't have a build system myself to test/verify such a change on.)

@jtanx

This comment has been minimized.

Contributor

jtanx commented Jul 30, 2017

I'm curious though, as I was under the impression that some builds have actually been done in the past that had support for winidn while you're saying that it can't detect IdnToUnicode at all.

Probably 64-bit builds only, for reasons as mentioned here: Alexpux/MINGW-packages#2677 (comment)

@jeroen

This comment has been minimized.

Contributor

jeroen commented Jul 30, 2017

The thing is that this problem only appears on 32 bit windows. There have been previous builds with IdnToUnicode() but probably only for 64 bit windows. For example the 32bit builds on the curl website use winidn on win64 but libidn on win32: https://curl.haxx.se/gknw.net/7.40.0/

BTW I was able to build a 32bit libcurl with winidn using the hack below so I can confirm that it does work, if you can only bypass the configure test.

export CPPFLAGS="-DUSE_WIN32_IDN -DWINVER=0x0600"	
@jeroen

This comment has been minimized.

Contributor

jeroen commented Jul 30, 2017

I hope @jtanx can maybe help us construct a PR because I don't understand autotool well enough :)

jtanx added a commit to jtanx/curl that referenced this issue Jul 31, 2017

@bagder bagder closed this in f262b35 Jul 31, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.