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

API breakage with version 7.5.0 #926

Closed
fritsch opened this Issue Jul 24, 2016 · 15 comments

Comments

Projects
None yet
4 participants
@fritsch

fritsch commented Jul 24, 2016

This commit: 9adf3c4 breaks public API / ABI.

You change the types from an opaque typedef to a non-opaque typedef. Every application out there that was forward declaring these types won't compile anymore without adjustments. Furthermore if applications want to support old (7.49.0) and new curl > 7.50.0, this needs to be workarounded with preprocessor statements.

As I am not against typesafe declarations please bump the relevant things so that user code needs rebuilding and the issues appearing can be fixed.

@bagder

This comment has been minimized.

Member

bagder commented Jul 24, 2016

(This does not break ABI by any means.)

As I said before, I'm not convinced that our symbols are free for others to forward declare like that (symbols that start with curl we claim are "ours") as you make assumptions about what it is, and like here, your assumption is wrong. So yes, it is a question about what our API promises.

In your case, you can avoid the need to forward declare the type and thus avoid version checking #ifdefs by simply including the public curl header, can't you?

@fritsch

This comment has been minimized.

fritsch commented Jul 24, 2016

Of course I can - but I also already told why forward decleration in a header is a good thing vs including a lot of further dependencies.

I will go with the #ifdef for now and drop it when major distributions bump their libcurl. For now - this change broke kodi on gentoo, arch, void and some more :-) - this line of code was there since 2009.

To say that again: I am not against this typesafety change, but be careful if you change typedefs in public headers because some users might forward declare them.

@david-geiger

This comment has been minimized.

david-geiger commented Jul 31, 2016

Hi,
I can confirm this breakage too!
On mageia we have now some packages who fails to build with curl 7.50.0:

-libreoffice:
http://pkgsubmit.mageia.org/autobuild/cauldron/x86_64/core/2016-07-27/libreoffice-5.1.4.2-5.mga6.src.rpm/build.0.20160727204007.log

-cclive:
http://pkgsubmit.mageia.org/autobuild/cauldron/x86_64/core/2016-07-27/cclive-0.9.3-6.mga6.src.rpm/build.0.20160727204007.log

-manaplus:
http://pkgsubmit.mageia.org/autobuild/cauldron/x86_64/core/2016-07-27/manaplus-1.6.5.7-1.mga6.src.rpm/build.0.20160729080711.log

-megaglest:
http://pkgsubmit.mageia.org/autobuild/cauldron/x86_64/core/2016-07-27/megaglest-3.12.0-3.mga6.src.rpm/build.0.20160729080711.log

-quvi:
http://pkgsubmit.mageia.org/autobuild/cauldron/x86_64/core/2016-07-27/quvi-0.9.5-5.mga6.src.rpm/build.0.20160729080711.log

-spring:
http://pkgsubmit.mageia.org/autobuild/cauldron/x86_64/core/2016-07-27/spring-101.0-1.mga6.src.rpm/build.0.20160727204007.log

-springlobby:
http://pkgsubmit.mageia.org/autobuild/cauldron/x86_64/core/2016-07-27/springlobby-0.225-8.mga6.src.rpm/build.0.20160728214945.log

-sword:
http://pkgsubmit.mageia.org/autobuild/cauldron/x86_64/core/2016-07-27/sword-1.7.3-7.mga6.src.rpm/build.0.20160729080711.log

-sysdig:
http://pkgsubmit.mageia.org/autobuild/cauldron/x86_64/core/2016-07-27/sysdig-0.8.0-2.mga6.src.rpm/build.0.20160729080711.log

-warmux:
http://pkgsubmit.mageia.org/autobuild/cauldron/x86_64/core/2016-07-27/warmux-11.04.1-12.mga6.src.rpm/build.0.20160727204007.log

-and kodi of course

Regards,
David

@fritsch

This comment has been minimized.

fritsch commented Jul 31, 2016

Basically I gave up - after the other side was immune to arguments - thanks for providing additional examples.

@bagder

This comment has been minimized.

Member

bagder commented Jul 31, 2016

after the other side was immune to arguments

You just repeated the single argument, that doesn't make me immune...

@bagder

This comment has been minimized.

Member

bagder commented Jul 31, 2016

I can confirm this breakage too!
On mageia we have now some packages who fails to build

I agree that looks bad. But this change also makes subtle abuses generate errors now (which I consider good) so we can't blindly just say that this is libcurl's fault without checking what they're actually doing in their source code. Are they all doing forward declarations (ie declaring the CURL * themselves only to find it doesn't match our header) as kodi?

@fritsch

This comment has been minimized.

fritsch commented Jul 31, 2016

Yeah it seems they forward declare your public (!) header, which the new version of curl changed without bumping :-)

@bagder

This comment has been minimized.

Member

bagder commented Jul 31, 2016

Again, you all forward declare our header assuming its contents. I still believe we are allowed to do this, but since so many are doing it like this it makes the problem worse.

@fritsch

This comment has been minimized.

fritsch commented Jul 31, 2016

We all forward declare your public (!) header, which changed the types from a void (without size) to something not void with a size.

So just bump, release a new curl, break years old working software, by telling them: you do it wrong since ten years and all is fine.

@bagder

This comment has been minimized.

Member

bagder commented Jul 31, 2016

This is a possible mostly reversion that still allows willing applications to get better typechecks:

From 045230d34e0c9e504f0af75f097ed94ec57f6093 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Sun, 31 Jul 2016 11:48:44 +0200
Subject: [PATCH] include: revert 9adf3c4 and make public types void * again

Too many applications assume the actual contents of the public types and
use that to for example to forward declarations (saving them from
including our public header) which then breaks when we switch from void
* to a struct *.

I'm not convinced we were wrong, but since this practise seems
widespread enough I'm willing to (partly) step down.

Now libcurl uses the struct itself when it is built and it allows
applications to use it the type if CURL_STRICTER is defined at the time
of the #include.
---
 include/curl/curl.h  | 7 ++++++-
 include/curl/multi.h | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/curl/curl.h b/include/curl/curl.h
index 516ede6..7fd6d1f 100644
--- a/include/curl/curl.h
+++ b/include/curl/curl.h
@@ -89,11 +89,17 @@

 #ifdef  __cplusplus
 extern "C" {
 #endif

+#if defined(BUILDING_LIBCURL) || defined(CURL_STRICTER)
 typedef struct Curl_easy CURL;
+typedef struct Curl_share CURLSH;
+#else
+typedef void CURL;
+typedef void CURLSH;
+#endif

 /*
  * libcurl external API function linkage decorations.
  */

@@ -2256,11 +2262,10 @@ typedef void (*curl_lock_function)(CURL *handle,
                                    void *userptr);
 typedef void (*curl_unlock_function)(CURL *handle,
                                      curl_lock_data data,
                                      void *userptr);

-typedef struct Curl_share CURLSH;

 typedef enum {
   CURLSHE_OK,  /* all is fine */
   CURLSHE_BAD_OPTION, /* 1 */
   CURLSHE_IN_USE,     /* 2 */
diff --git a/include/curl/multi.h b/include/curl/multi.h
index 7a1040f..d1e00cc 100644
--- a/include/curl/multi.h
+++ b/include/curl/multi.h
@@ -50,11 +50,15 @@

 #ifdef  __cplusplus
 extern "C" {
 #endif

+#if defined(BUILDING_LIBCURL) || defined(CURL_STRICTER)
 typedef struct Curl_multi CURLM;
+#else
+typedef void CURLM;
+#endif

 typedef enum {
   CURLM_CALL_MULTI_PERFORM = -1, /* please call curl_multi_perform() or
                                     curl_multi_socket*() soon */
   CURLM_OK,
-- 
2.8.1
@fritsch

This comment has been minimized.

fritsch commented Jul 31, 2016

Thanks for the patch, though - as I am also a fan of typesafe API (no kidding), keep it without it and bump all the internal version scheme you have, so that software needs adjusting.

Or we will have the next issue when in some years the types change again.

@Paxxi

This comment has been minimized.

Paxxi commented Jul 31, 2016

This is a good way to handle it making the new behavior of in.

@bagder

This comment has been minimized.

Member

bagder commented Jul 31, 2016

I get people coming after me with pitchforks and fires if I just mention bumbing the SONAME, so I'll save that for a future day when we do a whole set of changes at once. A very long time into the future =)

@fritsch

This comment has been minimized.

fritsch commented Jul 31, 2016

Hehe :-) - oki, fine with us, you can close this issue after the new curl version with that patched is released. Thanks very much :-)

@david-geiger

This comment has been minimized.

david-geiger commented Jul 31, 2016

Yeah good news for us! this proposed patch seems fixed this API compatibility :)

Thanks!

@bagder bagder closed this in d660452 Jul 31, 2016

@womfoo womfoo referenced this issue Aug 3, 2016

Merged

curl: 7.50.0 -> 7.50.1 #17486

4 of 7 tasks complete

@garbas garbas referenced this issue Aug 5, 2016

Merged

curl: 7.47.1 -> 7.50.0 #17152

4 of 7 tasks complete

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

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