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

Jump to bottom

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

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

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

jeroen opened this issue Jul 8, 2017 · 9 comments
Labels
build help wanted

Comments

@jeroen
Copy link
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 ?
@jeroen jeroen mentioned this issue Jul 8, 2017
Closed
@jtanx
Copy link
Contributor

jtanx commented Jul 8, 2017 •
edited

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
Copy link
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
Copy link
Contributor

jtanx commented Jul 10, 2017 •
edited

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
Copy link
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.)

@jeroen
Copy link
Contributor Author

jeroen commented Jul 30, 2017

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

@bagder
Copy link
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
Copy link
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: https://github.com/Alexpux/MINGW-packages/issues/2677#issuecomment-313884699

@jeroen
Copy link
Contributor Author

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
Copy link
Contributor Author

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
@jtanx
Fix the check for IdnToUnicode
1b4b0f2 
Fixes curl#1669
@jtanx jtanx mentioned this issue Jul 31, 2017
Closed
@bagder bagder closed this as completed 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.
Assignees
No one assigned
Labels
build help wanted
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bagder @jeroen @jtanx