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

Makefile.m32: stop trying to build libcares.a [ci skip] #9169

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jul 17, 2022

Before this patch, lib/Makefile.m32 had a rule to build libcares.a in
-cares-enabled builds, via c-ares's own Makefile.m32. Committed in
2007 [1]. The commit message doesn't specifically address this particular
change. No other curl dependency has such special treatment. It probably
helped building a curl + c-ares combo in a single go, back when there
were less optional curl dependencies? I can only guess.

This feature creates problems when building c-ares first, using CMake
and pointing LIBCARES_PATH to its install prefix, where Makefile.m32
is missing in such case. A sub-build for c-ares is undesired also when
c-ares had already been build via its own Makefile.m32.

So, to avoid the sub-build, this patch deletes its Makefile rule. After
this patch libcares.a needs to be manually built before using it in
Makefile.m32. Aligning it with the rest of dependencies.

[1] 46c92c0

Before this patch, `lib/Makefile.m32` had a rule to build `libcares.a` in
`-cares`-enabled builds, via c-ares's own `Makefile.m32`. Committed in
2007 [1]. The commit message doesn't specifically address this particular
change. No other curl dependency has such special treatment. It probably
helped building a curl + c-ares combo in a single go, back when there
were less optional curl dependencies? I can only guess.

This feature creates problems when building c-ares first, using CMake
and pointing `LIBCARES_PATH` to its install prefix, where `Makefile.m32`
is missing in such case. A sub-build for c-ares is undesired also when
c-ares had already been build via its own `Makefile.m32`.

So, to avoid the sub-build, this patch deletes its Makefile rule. After
this patch `libcares.a` needs to be manually built before using it in
`Makefile.m32`. Aligning it with the rest of dependencies.

[1] 46c92c0
@vszakats vszakats added build Windows Windows-specific name lookup DNS and related tech labels Jul 17, 2022
@bagder
Copy link
Member

bagder commented Jul 17, 2022

I believe this might be tracing back from the oooold days when c-ares actually was a sub-tree in the curl source tree...

@vszakats
Copy link
Member Author

Thanks for the historical insight, that explains it perfectly. I'm going to include it in the commit message.

@vszakats vszakats closed this in f9ff59e Jul 17, 2022
@vszakats vszakats deleted the m32-cares-fix branch July 17, 2022 21:46
vszakats added a commit to curl/curl-for-win that referenced this pull request Jul 19, 2022
Experimental, not enabled by default and no promises to keep it around.

```
curl 7.84.0 (x86_64-w64-mingw32) libcurl/7.84.0 mbedTLS/3.2.1 (BoringSSL/7528f03c) (Schannel) zlib/1.2.12 brotli/1.0.9 zstd/1.5.2 c-ares/1.17.2 libidn2/2.3.3 libpsl/0.21.1 (+libidn2/2.3.3) libssh/0.9.6/openssl/zlib nghttp2/1.48.0 ngtcp2/0.6.0 nghttp3/0.5.0 libgsasl/1.10.0
Release-Date: 2022-06-27
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: alt-svc AsynchDNS brotli gsasl HSTS HTTP2 HTTP3 IDN IPv6 Kerberos Largefile libz MultiSSL NTLM PSL SPNEGO SSL SSPI threadsafe UnixSockets zstd
```

PR opened for issue when enabling c-ares with `Makefile.m32`:
curl/curl#9169
vszakats added a commit to curl/curl-for-win that referenced this pull request Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build name lookup DNS and related tech Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

2 participants