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

curl-config --libs returns incorrect value on cygwin #5793

Closed
ramsay-jones opened this issue Aug 7, 2020 · 30 comments
Closed

curl-config --libs returns incorrect value on cygwin #5793

ramsay-jones opened this issue Aug 7, 2020 · 30 comments
Labels
build

Comments

@ramsay-jones
Copy link

I did this

Last night I updated my cygwin installation and (among others) my curl packages were updated.
This lead to my git build failing while linking git-imap-send.exe as the linker could not find the
libraries: -lnghttp2, -lidn2, -lssh, -lpsl, -lldap, -llber, -lbrotlidec, -lbrotlidec (yes it was noted twice).

$ cygcheck -cd | grep curl
curl 7.71.1-1
libcurl-devel 7.71.1-1
libcurl-doc 7.71.1-1
libcurl4 7.71.1-1
$

The git makefile uses 'curl-config --libs', which produces:

$ curl-config --libs
-lcurl -lnghttp2 -lidn2 -lssh -lpsl -lssl -lcrypto -lldap -llber -lbrotlidec -lbrotlidec -lz
$

Looking at the '/usr/lib/pkgconfig/libcurl.pc' file, I find:

$ cat /usr/lib/pkgconfig/libcurl.pc
...
Libs: -L${libdir} -lcurl
Libs.private: -lnghttp2 -lidn2 -lssh -lpsl -lssl -lcrypto -lldap -llber -lbrotlidec -lbrotlidec -lz
...
$

I had a quick look at the curl git repo, and this seems to be due to a recent commit 98e5904
(libcurl.pc: Merge Libs.private into Libs for static-only builds, 2020-05-11).

The git Makefile allows me to override the value returned by curl-config, by setting 'CURL_LDFLAGS=-lcurl'
in my config.mak, so I have simple solution for now - just thought you should know. ;-)

I expected the following

git to build correctly!

curl/libcurl version

$ curl -V
curl 7.71.1 (x86_64-pc-cygwin) libcurl/7.71.1 OpenSSL/1.1.1f zlib/1.2.11 brotli/1.0.7 libidn2/2.2.0 libpsl/0.21.0 (+libidn2/2.0.4) libssh/0.8.7/openssl/zlib nghttp2/1.37.0
Release-Date: 2020-07-01
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: AsynchDNS brotli Debug HTTP2 HTTPS-proxy IDN IPv6 Largefile libz NTLM NTLM_WB PSL SSL TLS-SRP TrackMemory UnixSockets
$

operating system

$ uname -a
CYGWIN_NT-10.0 satellite 3.1.6(0.340/5/3) 2020-07-09 08:20 x86_64 Cygwin
$
@bagder bagder added the build label Aug 7, 2020
@bagder
Copy link
Member

bagder commented Aug 7, 2020

Did you build libcurl static only?

@ramsay-jones
Copy link
Author

ramsay-jones commented Aug 7, 2020 via email

@bagder
Copy link
Member

bagder commented Aug 8, 2020 •
edited

I asked about static vs shared because the configure generated curl-config --libs should only return the full list like that if not built shared, and AFAIK shared builds are by default. For a shared build/lib (which is what you seem to be using) a single -lcurl is probably the correct thing as the rest will then be dynamically linked.

So yeah, it could be a cygwin-specific packaging issue or perhaps even a cygwin-specific configure issue in curl. I don't know.

@me-and
Copy link

me-and commented Aug 10, 2020

@ramsay-jones pointed me at this issue; I'm the Cygwin maintainer for Git. This is indeed a Cygwin-specific packaging issue; it looks like the Curl maintainer mistakenly dropped a patch to the compile-time configuration that affects the linking behaviour.

I'm expecting this to be fixed in Cygwin shortly; a new patch was proposed at https://cygwin.com/pipermail/cygwin/2020-August/245820.html earlier today.

@bagder
Copy link
Member

bagder commented Aug 10, 2020

Thanks that input @me-and. I presume we can then consider this case closed @ramsay-jones ?

@ramsay-jones
Copy link
Author

ramsay-jones commented Aug 11, 2020 via email

@bagder
Copy link
Member

bagder commented Aug 11, 2020

I think I would be more interested about the need for a cygwin specific patch to the curl build system to make it work! ;-)

I would agree, but I don't see any strong desire to fix that. It seems the curl maintainers in cygwin has been doing their own patches on curl for a very long time without having tried to fix them directly upstream - in curl.

@BrianInglis
Copy link
Contributor

I'm the new Cygwin curl maintainer, and I would prefer to drop local patches if we can get appropriate changes made upstream wherever possible.
The local specific patch against an earlier curl release did not apply, and I did not notice any issues, so I released the package upgrade to the latest curl.
If we need to keep a local patch against curl/-config/.in I don't have a problem, but would prefer to discuss an approach and offer a patch/PR if desirable.
Often product maintainers have their own preferred approaches based on their deep knowledge and experience of the packages they maintain, and will come up with a more general approach than my narrow look at one issue would suggest.

It looks to me as if the generated curl-config --libs statements:

        if test "Xyes" = "Xno" -o "Xyes" = "Xyes"; then
          echo ${CURLLIBDIR}-lcurl -lnghttp2 -lidn2 -lssh -lpsl -lssl -lcrypto
-lldap -llber -lbrotlidec -lbrotlidec -lz

based on curl-config.in:

        if test "X@ENABLE_SHARED@" = "Xno" -o "X@REQUIRE_LIB_DEPS@" = "Xyes"; then
          echo ${CURLLIBDIR}-lcurl @LIBCURL_LIBS@

REQUIRE_LIB_DEPS should be no, derived from configure.ac:

if test "X$enable_shared" = "Xyes" -a "X$link_all_deplibs" = "Xno"
then
    REQUIRE_LIB_DEPS=no
else
    REQUIRE_LIB_DEPS=yes
fi
AC_SUBST(REQUIRE_LIB_DEPS)
AM_CONDITIONAL(USE_EXPLICIT_LIB_DEPS, test x$REQUIRE_LIB_DEPS = xyes)

but for Cygwin and a number of other platforms link_all_deplibs remains defaulted to unknown, so either that variable should be set in Cygwin configure to no, or that condition should perhaps be changed to:

if test "X$enable_shared" = "Xyes" -a "X$link_all_deplibs" != "Xyes"

bagder added a commit that referenced this issue Aug 12, 2020
@bagder
configure: adjust the REQUIRE_LIB_DEPS logic
086b75c 
Fixes curl-config genreration on cygwin by making sure REQUIRE_LIB_DEPS
is set to 'no' there.

Assisted-by: Brian Inglis
Fixes #5793
@bagder bagder mentioned this issue Aug 12, 2020
Closed
@bagder
Copy link
Member

bagder commented Aug 12, 2020

Thanks! It feels a little risky as this might change a little too much for other platforms, but hey let's try this. See #5804 for what I take is your suggested fix?

@kbrow1i
Copy link

kbrow1i commented Aug 12, 2020

This strikes me as a very fragile fix, likely to confuse anyone reading the code in the future, for two reasons.

  1. The libtool manual says the following about link_all_deplibs: "Whether libtool must link a program against all its dependency libraries. Set to ‘yes’ or ‘no’. Default is ‘unknown’, which is a synonym for ‘yes’." But this patch treats 'unknown' as different from 'yes'.

  2. Setting REQUIRE_LIB_DEPS to 'no' on Cygwin gives the impression that we don't need to specify all dependent libraries when building curl on Cygwin. [In fact we do need to do this, and libtool is smart enough to do it regardless of how REQUIRE_LIB_DEPS is defined.]

How about the following much simpler fix, which is essentially what's been done in the Cygwin build of curl for years, until the most recent build:

--- origsrc/curl-7.71.1/curl-config.in 2020-06-27 18:03:53.000000000 -0400
+++ src/curl-7.71.1/curl-config.in 2020-08-12 12:34:53.341934900 -0400
@@ -160,7 +160,7 @@ while test $# -gt 0; do
else
CURLLIBDIR=""
fi

  •    if test "X@ENABLE_SHARED@" = "Xno" -o "X@REQUIRE_LIB_DEPS@" = "Xyes"; then

  •    if test "X@ENABLE_SHARED@" = "Xno"; then
     echo ${CURLLIBDIR}-lcurl @LIBCURL_LIBS@
   else
     echo ${CURLLIBDIR}-lcurl

@kbrow1i
Copy link

kbrow1i commented Aug 12, 2020

Whoops! I see the patch got mangled. Let me try attaching it instead.
curl-7.71.1-1.src.txt

@BrianInglis
Copy link
Contributor

This strikes me as a very fragile fix, likely to confuse anyone reading the code in the future, for two reasons.

  1. The libtool manual says the following about link_all_deplibs: "Whether libtool must link a program against all its dependency libraries. Set to ‘yes’ or ‘no’. Default is ‘unknown’, which is a synonym for ‘yes’." But this patch treats 'unknown' as different from 'yes'.

I noticed that in the autoconf/libtool docs and thought that perhaps a better option might be to make changes or get changes made in m4/libtool.m4 to set link_all_deplibs=no like Linux in the LT_SHLIBS section.

  1. Setting REQUIRE_LIB_DEPS to 'no' on Cygwin gives the impression that we don't need to specify all dependent libraries when building curl on Cygwin. [In fact we do need to do this, and libtool is smart enough to do it regardless of how REQUIRE_LIB_DEPS is defined.]

I understood the need for the patch was that Cygwin needs dynamically linked against libcurl only link_all_deplibs=no, but statically also needs linked against all the libcurl dependencies link_all_deplibs=yes, effectively the difference between libcurl.pc Libs list and Libs.private list, and that is what I understood to be the meaning of link_all_deplibs.

I did not need a patch because I was able to build both curl and git Cygwin packages, including the git programs where others had issues, with no changes to curl and git cygport build control scripts in the Cygwin package sources, so was unable to reproduce the issues reported by others.

With a large number of Cygwin package-devel packages and dependencies installed, I understand my development configuration may be atypical, and as curl-config did not generate the expected output, thought consideration should be given to getting that issue fixed.

How about the following much simpler fix, which is essentially what's been done in the Cygwin build of curl for years, until the most recent build:

To include a patch or any code block, use Markdown verbatim code block quotes around content: three graves ``` on separate lines before and after the block. If you also name the language or source type immediately following the graves on the first line - in this case ```patch additional source highlighting may occur. e.g.

--- origsrc/curl-7.71.1/curl-config.in	2020-06-27 18:03:53.000000000 -0400
+++ src/curl-7.71.1/curl-config.in	2020-08-12 12:34:53.341934900 -0400
@@ -160,7 +160,7 @@ while test $# -gt 0; do
         else
            CURLLIBDIR=""
         fi
-        if test "X@ENABLE_SHARED@" = "Xno" -o "X@REQUIRE_LIB_DEPS@" = "Xyes"; then
+        if test "X@ENABLE_SHARED@" = "Xno"; then
           echo ${CURLLIBDIR}-lcurl @LIBCURL_LIBS@
         else
           echo ${CURLLIBDIR}-lcurl

@bagder
Copy link
Member

bagder commented Aug 13, 2020

@chewi, you brought this change of this line in #5373, do you have concerns or other thoughts on this proposed patch?

bagder added a commit that referenced this issue Aug 13, 2020
@bagder
curl-config: ignore REQUIRE_LIB_DEPS in --libs output
a29e989 
Fixes a curl-config issue on cygwin by making sure REQUIRE_LIB_DEPS is
not considered for the --libs output.

Reported-by: ramsay-jones on github
Assisted-by: Brian Inglis and Ken Brown
Fixes #5793
@bagder bagder mentioned this issue Aug 13, 2020
Closed
@bagder
Copy link
Member

bagder commented Aug 13, 2020

In the mean time, I created a separate PR with that simpler take.

@chewi
Copy link
Contributor

chewi commented Aug 13, 2020

Thanks for notifying me, I'll take a look in a few hours tonight. Ironically, I made my change in the context of cross-compiling statically for Windows but not Cygwin.

@kbrow1i
Copy link

kbrow1i commented Aug 13, 2020

I understood the need for the patch was that Cygwin needs dynamically linked against libcurl only link_all_deplibs=no, but statically also needs linked against all the libcurl dependencies link_all_deplibs=yes, effectively the difference between libcurl.pc Libs list and Libs.private list, and that is what I understood to be the meaning of link_all_deplibs.

I think you're confusing two unrelated things. 'link_all_deplibs' is a libtool variable that affects the linking while building curl itself. If you try to build curl with link_all_deplibs=no, I think you'll find that the executable curl.exe will fail to link because many of the libs it needs will be omitted from the link command line. The default value 'unknown', which is synonymous with 'yes', is correct.

On the other hand, the output of 'curl-config --libs', which is the subject of this bug report, affects what other applications need to provide on their link command line when they want to link against libcurl. (This is necessarily dynamic linking, since the Cygwin build of curl disables static libraries.) As the author of this bug report explained very clearly, these applications need to provide only '-lcurl'. The value of link_all_deplibs when curl was built is not relevant.

@BrianInglis
Copy link
Contributor

While building curl itself, curl-config appears to be generated embedding static tests of the static values of REQUIRE_LIB_DEPS and link_all_deplibs which results in the incorrect output of which you complained: perhaps curl-config should not then be used in building other products.

Also while building git itself, curl-config may be invoked, causing the problems others have experienced, whereas I was unable to reproduce those issues, and git built and tested as expected.

Perhaps we need a better understanding of curl-config, its intent, outputs, and uses of those.

@ramsay-jones
Copy link
Author

ramsay-jones commented Aug 13, 2020 via email

@BrianInglis
Copy link
Contributor

Thank you!
That is likely the link I was missing.
I expected it may have been because I have built a number of (non-Cygwin) packages for my own interest and use and have currently 124 ...-devel packages installed.
I may consider using apt-cyg remove/-s install ...-devel to clean up my environment before regression tests like this.

My question would be why packages appear to be using curl-config for anything other than info, which seems to be its function, perhaps to document the version of libcurl linked, features supported, and perhaps other configuration options or arguments, like the gcc info in /proc/version?
There is a perfectly good /lib/pkgconfig/libcurl.pc file available for use with pkgconf/-config --libs as is usual in builds, which gives the correct expected results for both dynamic and static linking!

So now I am unsure exactly where the fix effort should really be directed - Cygwin, curl, git, libtool, autoconf?

The previous Cygwin patch used to kludge around the problem for Cygwin and we can continue to do so, but there should be an architected approach so that vanilla git, curl, and Cygwin can work together without a new kludge every release or few.
That patch originated from applying a Windows patch to the Cygwin environment, which is as POSIX (and related standards), and Linux compatible, and un-Windows-like, as can be under Windows, and perhaps I should take a look at what that changed, and see how to back out the effects for Cygwin builds, affecting only Windows builds.
That way I can leave aside the issue of whether git, libtool, and autoconf should be using curl-config rather than pkgconf/-config to use libcurl in builds, and any discussions or changes about that.

@chewi
Copy link
Contributor

chewi commented Aug 13, 2020

/me waves to @me-and. 👋

I've now had a chance to digest this. I agree with what @kbrow1i is proposing, and hence #5808. link_all_deplibs in this context is only relevant when building curl itself and curl-config is only used when building against curl. I don't know why they were ever linked in this way. I recall being confused by this very line when I made the earlier change.

My own distribution is Gentoo and link_all_deplibs is surprisingly unknown here too. curl-config --libs also returns the full set here. Perhaps this affects more distributions than you think. I wasn't hit by the issue though because Gentoo effectively always has the -devel files installed anyway.

More broadly speaking, I think *-config scripts are a horrible kludge that should be phased out. pkg-config does a far better job of handling these things, especially in exotic setups, such as when cross-compiling. Gentoo developers make a concerted effort to remove usage of these scripts in upstream projects where it is feasible.

@BrianInglis
Copy link
Contributor

I'm even happier to agree with #5808 with you @chewi and @bagder agreeing with @kbrow1i as it will resolve @me-and and @ramsay-jones build issues.
Cygwin maintainers @me-and and @kbrow1i - do you think I should build a curl-7.71.1-2 upgrade package ASAP with a local Cygwin curl #5808 patch, or wait for the next official curl release including the patch?

@bagder
Copy link
Member

bagder commented Aug 14, 2020

pkg-config does a far better job of handling these things

Agreed. curl-config is older and was added to curl in April 2001 with inspiration from some other project that I now forget which it was. I don't know when pkg-config actually became a real "thing" but curl has supported it since December 2004.

@bagder bagder closed this as completed in 97aca09 Aug 14, 2020
@kbrow1i
Copy link

kbrow1i commented Aug 14, 2020

Cygwin maintainers @me-and and @kbrow1i - do you think I should build a curl-7.71.1-2 upgrade package ASAP with a local Cygwin curl #5808 patch

Yes.

@BrianInglis
Copy link
Contributor

Patch #5808 disables support for static libraries and I believe that now prevents the curl tests from being built or run - alternative suggestions which support a patch to allow both dynamic and static library builds?

Making all in libtest
make[2]: Entering directory '/mnt/c/Users/bwi/src/cygwin/curl/curl-7.71.1-2.x86_64/build/tests/libtest'
  CCLD     libstubgss.la
libtool:   error: can't build x86_64-pc-cygwin shared library unless -no-undefined is specified
make[2]: *** [Makefile:2547: libstubgss.la] Error 1
make[2]: Target 'all' not remade because of errors.
make[2]: Leaving directory '/mnt/c/Users/bwi/src/cygwin/curl/curl-7.71.1-2.x86_64/build/tests/libtest'
Making all in unit
make[2]: Entering directory '/mnt/c/Users/bwi/src/cygwin/curl/curl-7.71.1-2.x86_64/build/tests/unit'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/mnt/c/Users/bwi/src/cygwin/curl/curl-7.71.1-2.x86_64/build/tests/unit'
make[2]: Entering directory '/mnt/c/Users/bwi/src/cygwin/curl/curl-7.71.1-2.x86_64/build/tests'
make[2]: Nothing to be done for 'all-am'.
make[2]: Leaving directory '/mnt/c/Users/bwi/src/cygwin/curl/curl-7.71.1-2.x86_64/build/tests'
make[1]: *** [Makefile:513: all-recursive] Error 1
make[1]: Target 'all' not remade because of errors.
make[1]: Target 'quiet-test' not remade because of errors.
make[1]: Leaving directory '/mnt/c/Users/bwi/src/cygwin/curl/curl-7.71.1-2.x86_64/build/tests'
make: *** [Makefile:1437: test] Error 2

@kbrow1i
Copy link

kbrow1i commented Aug 14, 2020

Patch #5808 disables support for static libraries

No, it doesn't. cygport does that by default, and it's standard Cygwin practice.

Making all in libtest
make[2]: Entering directory '/mnt/c/Users/bwi/src/cygwin/curl/curl-7.71.1-2.x86_64/build/tests/libtest'
CCLD libstubgss.la
libtool: error: can't build x86_64-pc-cygwin shared library unless -no-undefined is specified

This no longer has anything to do with the bug report. Let's move the discussion to the cygwin-apps list and I'll answer there. I can't do it immediately, but I'll get to it in a couple hours. If you're impatient, search the cygport manual for '-no-undefined'.

@BrianInglis
Copy link
Contributor

Sorry @kbrow1i is correct #5808 is good and has been applied locally to an updated Cygwin curl{,-devel,-doc} fix release.

@BrianInglis
Copy link
Contributor

Thanks for Cygwin changes included in latest release - our downstream package maintainers and consumers are happy that curl and libcurl build right from download, and downstreams like Cygwin git, and consumers like the OP @ramsay-jones , can now use either your release sources or the Cygwin packaged sources.
It would be good if @ramsay-jones could confirm and close?

@bagder
Copy link
Member

bagder commented Aug 24, 2020

Excellent news @BrianInglis. Thanks for the great cooperation and do let us know if there are other cygwin nits existing already now or appearing in the future that we should better fix.

@BrianInglis
Copy link
Contributor

Cheers @bagder and thank you.
I am sure someone will let me know quickly if any issues develop.

@ramsay-jones
Copy link
Author

ramsay-jones commented Aug 25, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build
Projects
None yet
Milestone
No milestone
6 participants
@chewi @bagder @me-and @kbrow1i @BrianInglis @ramsay-jones