[Home]

Summary:ASTERISK-03890: [patch] Allow app_curl to be compiled with outdated libcurl
Reporter:Paul Cadach (pcadach)Labels:
Date Opened:2005-04-07 16:00:08Date Closed:2008-01-15 15:31:35.000-0600
Priority:TrivialRegression?No
Status:Closed/CompleteComponents:Applications/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20050408__curl_fix2.diff.txt
( 1) asterisk-curl.diff
( 2) asterisk-curl2.diff
Description:Looks like libcurl have slightly changed API and defined another option - CURLOPT_WRITEDATA instead of CURLOPT_WRITEINFO for old libcurl.

Working of this patch isn't tested and just allows to build Asterisk cleanly on RH-7.3.

****** ADDITIONAL INFORMATION ******

Disclaimer is on file.
Comments:By: Matthew Fredrickson (mattf) 2005-04-07 16:20:47

Looks good.
By: Matthew Fredrickson (mattf) 2005-04-07 16:26:02

Committed.  Thanks
By: Tilghman Lesher (tilghman) 2005-04-07 16:32:08

I'd feel much more comfortable if we didn't just commit bugs, especially ones that say, very clearly, UNTESTED.
By: Tilghman Lesher (tilghman) 2005-04-07 16:39:06

And in fact, an cursory examination of the libcurl API reveals, in fact, that this patch is the WRONG THING TO DO.

Please revert.
By: Matthew Fredrickson (mattf) 2005-04-07 17:29:29

You're right.  That's my mistake.  I just looked over the API docs for libcurl and WRITEINFO definitely does not take the same argument as WRITEDATA.  PCadach, are you sure that this is the right fix for your problem?  (I think he's asleep right now though)
By: Tilghman Lesher (tilghman) 2005-04-07 23:47:48

I think what's he's getting is that our check in the Makefile is insufficient to disqualify really old versions of libcurl.  So the solution would be to modify the Makefile to do a check on the version of libcurl, and not compile the app, if the version of libcurl is too old.

Of course, the more logical solution would be to tell people they ought not to be using really old versions of a distribution, and either update the specific library to a recent package, or simply upgrade the distribution.
By: Paul Cadach (pcadach) 2005-04-07 23:57:33

Yep, but better IMHO is to have #ifdef'ed code for support outdated libraries. I can't find libcurl's version where CURLOPT_WRITEINFO was defined to disable compilation of app_curl if library is outdated. If it couldn't be located than compatibility code is required.

I just published this one to note there is problems available with outdated libcurl, not a real patch....
By: Paul Cadach (pcadach) 2005-04-08 00:02:53

Anyway we shoudn't abort compilation if some libraries is outdated.
By: Tilghman Lesher (tilghman) 2005-04-08 00:04:11

Patch and a shell script uploaded.  Note that you'll need to make sure to set the executable bit on the shell script and place it in the apps subdirectory.  Due to the arithmetic being done on the version number, unfortunately, it cannot be placed in the Makefile itself.  Sorry, I tried.

Disclaimer on file.
By: Tilghman Lesher (tilghman) 2005-04-08 00:07:46

As noted in the libcurl documentation here:  http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTWRITEDATA 7.9.7 was the version which had this constant declared.  The patch uploaded checks against that specific version number and will not attempt to compile app_curl if that version number is not met.
By: Paul Cadach (pcadach) 2005-04-08 01:10:12

Done in makefile only, without additional shell script. See asterisk-curl2.diff.
By: Tilghman Lesher (tilghman) 2005-04-08 08:36:48

Hopefuly, unlike the previous patch, you've tested this one on all 3 types of systems?  1) systems without libcurl, 2) systems with an old version of libcurl, and 3) systems with the current version of libcurl?

I don't think your version is going to work, because 'libcurl --vernum' outputs a 24-bit hexadecimal number, not a decimal.
By: Tilghman Lesher (tilghman) 2005-04-08 08:51:42

Example:  the version of libcurl on Mandrake 10.1 is 7.11.0, which outputs as 70B00.  A decimal comparison will compare 070907 to 70B00, see the B character as invalid and truncate the vernum to 70, which is most certainly less than 070907.  Also, I don't think you'll be able to use 070907, because typically, any number preceded with a 0 is treated as octal, and the 9 is an invalid digit in octal.  That would leave the number you're comparing against as 070, which is 56 decimal.

So I don't think you're really doing the right comparison.
By: Paul Cadach (pcadach) 2005-04-08 09:22:48

Will update when get back to home.
By: Paul Cadach (pcadach) 2005-04-08 11:52:39

Fixed. But better is to check how to adapt existing app_curl to be compatible with outdated libcurl. Is CURLOPT_WRITEDATA really equal to CURLOPT_FILE as pointed at libcurl's documentation?

Citate:
This option is also known with the older name CURLOPT_FILE, the name CURLOPT_WRITEDATA was introduced in 7.9.7.
By: Tilghman Lesher (tilghman) 2005-04-08 12:35:59

Fair enough, it can be made shorter, with a different construct.

And yes, the CURLOPT_FILE is the same option, if deprecated:

# grep -ri curlopt_file /usr/include
/usr/include/curl/curl.h:#define CURLOPT_WRITEDATA CURLOPT_FILE
# curl-config --vernum
070a03
By: Paul Cadach (pcadach) 2005-04-08 12:50:02

Is your patch is safe for missing curl-config? My first patch you deleted is much simpler solution and allows curl functionality just with outdated libcurl (but I'd wrong about equivalence between WRITEINFO and WRITEDATA, fixed in attached asterisk-curl.diff).
By: Tilghman Lesher (tilghman) 2005-04-08 13:28:45

Yes, it works fine.  You get a brief message that curl-config cannot be found, but the compile continues.
By: Paul Cadach (pcadach) 2005-04-08 14:06:19

Could you explain how it (asterisk-curl2.diff) could continue compilation if libcurl isn't found (to be sure, curl-config)? As I understand you see warning message from 'else' part, which blocks addition of app_curl to list of modules for building, so app_curl isn't built if there is no libcurl installed or libcurl is outdated.

But IMHO compatibility fix (asterisk-curl.diff) is more simple and provides more compatibility than just excluding app_curl from building.

Or I misunderstood something?
By: Tilghman Lesher (tilghman) 2005-04-08 14:22:00

You're asking me about your own patches?
By: Paul Cadach (pcadach) 2005-04-08 14:33:16

Test #1 - no curl-config:
WARNING: Version of libcurl is outdated or library is not installed
make[1]: Entering directory `/usr/src/ast-cur/asterisk/apps'
make[1]: Nothing to be done for `all'.
make[1]: Leaving directory `/usr/src/ast-cur/asterisk/apps'

Test #2 - curl-config (and libcurl too, of course) is outdated (7.9.5):
WARNING: Version of libcurl is outdated or library is not installed
make[1]: Entering directory `/usr/src/ast-cur/asterisk/apps'
make[1]: Nothing to be done for `all'.
make[1]: Leaving directory `/usr/src/ast-cur/asterisk/apps'

Test #3 - curl-config is ok (7.11.0 or 7.10.3, i.e. 070B00 or 070a03):
make[1]: Entering directory `/usr/src/ast-cur/asterisk/apps'
gcc -pipe  -Wall ... -fPIC   -c -o app_curl.o app_curl.c

So, all works as needed (at least for me). Is I missing something?

Also, do you have any notes about asterisk-curl.diff?
By: Tilghman Lesher (tilghman) 2005-04-08 15:28:11

The only thing I'd hate to see is someone running something 5 years old asking us why app_curl doesn't work with libcurl v6 or earlier.  It's just much simpler to say "this is the version that is required" and leave it at that.  That's why I'm advocating the Makefile fix in 20050408__curl_fix2.diff.txt.
By: Tilghman Lesher (tilghman) 2005-04-08 15:29:02

Out of curiosity, are you actually using Curl() on RH7.3 or is this a moot issue for you?
By: Paul Cadach (pcadach) 2005-04-08 15:37:17

I uses RH-7.3 as my testbed, and without fixes I can't build Asterisk on this system. When I'm doing regular cvs update -d and generating patches this is hard to patch and rollback app_curl every time. So I suggest to have any sort of fix to have clean build of Asterisk tree independedly on libraries' version installed on build host, possible (but not best case) to miss some specific applications which is not affects functionality of core Asterisk.
By: Paul Cadach (pcadach) 2005-04-08 15:38:51

PS: Is your 20050408__curl_fix2.diff.txt works clean when curl-config is missed?
By: Tilghman Lesher (tilghman) 2005-04-08 15:42:06

Yes, my patch is clean when curl-config cannot be found.
By: Paul Cadach (pcadach) 2005-04-08 15:48:26

Will see when patch will be commited. ;-)
By: Mark Spencer (markster) 2005-04-10 22:38:54

So am I supposed to put in the Makefile patch or the curl app fix?
By: Paul Cadach (pcadach) 2005-04-10 22:48:40

I am recommends to have app_curl fix instead of Makefile because CURLOPT_FILE was just renamed to CURLOPT_WRITEDATA in newer versions of libcurl.
By: Tilghman Lesher (tilghman) 2005-04-10 23:11:23

How about both?
By: Paul Cadach (pcadach) 2005-04-10 23:15:55

If you have Makefile's "fix" then you will not to have app_curl compiled yet, so patch for app_curl shouldn't be required. Otherwise, if you patch app_curl then you don't need to patch Makefile because all would works pretty well with old libcurl libraries.

edited on: 04-10-05 23:16
By: Tilghman Lesher (tilghman) 2005-04-10 23:34:58

But if you have both in, then Asterisk won't try to build Curl, if your version is insufficient, unless you explicitly ask for it, with a 'make app_curl.so'.
By: Paul Cadach (pcadach) 2005-04-10 23:40:48

make app_curl.so will not sufficient because you will not have all required paths and libraries needs to compile app_curl unless you have "valid" version of libcurl. Also, why to disable compilation of app_curl if it will have compatibility fix? I don't see any reasons to do so.
By: Matthew Fredrickson (mattf) 2005-04-12 12:27:56

I'm sure that extending compatibility for libcurl would not be a bad thing in this case.  How about we just apply pcadach's fix and we'll keep it there until someone needs support for Redhat 0.0.1 or something like that :-)

* Actually, the more I think about it, we probably should be moving forward, not really backward.  libcurl is relatively easy to update.  I'm going to check in the Makefile fix and if there are any more problems, just open a new bug.

edited on: 04-13-05 10:17
By: Matthew Fredrickson (mattf) 2005-04-13 10:26:20

fix applied
By: Digium Subversion (svnbot) 2008-01-15 15:31:13.000-0600

Repository: asterisk
Revision: 5437

U   trunk/apps/app_curl.c

------------------------------------------------------------------------
r5437 | mattf | 2008-01-15 15:31:13 -0600 (Tue, 15 Jan 2008) | 2 lines

Bugfix for old versions of libcurl (ASTERISK-3890)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=5437
By: Digium Subversion (svnbot) 2008-01-15 15:31:35.000-0600

Repository: asterisk
Revision: 5463

U   trunk/apps/Makefile

------------------------------------------------------------------------
r5463 | mattf | 2008-01-15 15:31:34 -0600 (Tue, 15 Jan 2008) | 3 lines

Fix so that asterisk continues to build if libcurl is not present or the correct
version. bug ASTERISK-3890.

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=5463