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

curl-config assumes that bc is present #3143

Closed
dimpase opened this issue Oct 16, 2018 · 13 comments
Closed

curl-config assumes that bc is present #3143

dimpase opened this issue Oct 16, 2018 · 13 comments

Comments

@dimpase
Copy link

dimpase commented Oct 16, 2018

bc might not be installed on the box (typical for VMs, e.g. if you use a default Debian install on a Google Compute node). Then curl-config --checkfor reports nonsense.

$ curl-config --checkfor 7.22
/usr/bin/curl-config: 1: /usr/bin/curl-config: bc: not found
/usr/bin/curl-config: 1: /usr/bin/curl-config: bc: not found
/usr/bin/curl-config: 112: test: Illegal number:
requested version 7.22 is newer than existing 7.52.1

not sure what the right fix is.

A platform-specific fix for Debian would add bc to pre-reqs for libcurl, but well, perhaps it can be improved generically.

@bagder
Copy link
Member

bagder commented Oct 16, 2018

Brought in commit 6ca627a, May 2006 and this is the first time someone has an issue with it... We could replace the bc use with a line using printf instead, but it would of course still rely on an external tool being present. I don't actually think this is a curl bug, other than perhaps that it could be documented somewhere.

@dimpase
Copy link
Author

dimpase commented Oct 16, 2018

One does not need any external tools here. Indeed, the 1st line using bc can be written as

checknum="$(($cmajor*256*256 + $cminor*256 + ${cpatch:-0}))"

Similarly if you don't use @VERSIONNUM@, but parse @CURLVERSION@ in the same way as you parse the input and get checknum, you can get nownum from the shell.

If you'd care to take a pull request fixing this, I can do it...

@bagder
Copy link
Member

bagder commented Oct 16, 2018

If you'd care to take a pull request fixing this, I can do it.

Yes please. Just make sure you're also not doing any bash'isms as I believe $() is? And I don't think a plain shell evaluates math expressions like that...

@infinnovation-dev
Copy link

Using only Bourne sh, expr and sed:

        checknum=`expr $cmajor \* 256 \* 256 + $cminor \* 256 + ${cpatch:-0}`
        numdigits=`echo @VERSIONNUM@ | sed -e 'y/abcdef/ABCDEF/; s/0x//; s/./ &/g; s/[ABCDEF]/1&/g; y/ABCDEF/012345/'`
        nownum=0
        for d in $numdigits; do nownum=`expr $nownum \* 16 + $d`; done

@dimpase
Copy link
Author

dimpase commented Oct 17, 2018

expr is another external program.

Anyway, I do not understand why one needs to do the arithmetic of this sort, as you can start by comparing major versions, if they are equal move to the minor ones, and finally, if needed, to the patch level... Yes it would be few lines longer, but much less cryptic.

@infinnovation-dev
Copy link

expr is typically part of a coreutils package and available on any sensible distro. But I think you're right about unnecessary arithmetic. Just unpick @CURLVERSION@ in the same way as $checkfor, rather than unhexifying @VERSIONNUM@.

@dimpase
Copy link
Author

dimpase commented Oct 17, 2018

Would testing with FreeBSDs /bin/sh suffice, or you recommend some other "bad" shell to test with?

@bagder
Copy link
Member

bagder commented Oct 18, 2018

Yes, I think that would be a good test. Or 'dash' on a Linux system.

@bagder
Copy link
Member

bagder commented Oct 18, 2018

The challenge is to make a conversion from or a comparison with a hexadecimal number without using a separate tool. I don't think that's possible. Then the question is really which separate tool that is most likely to be around and thus least troublesome to depend on... Contenders include bc and printf. I don't think expr alone can do it?

@dimpase
Copy link
Author

dimpase commented Oct 22, 2018

I don't understand why you insist on having to deal with that hexadecimal number --- as instead you can using the normal split version format right away.

@bagder
Copy link
Member

bagder commented Oct 22, 2018

Oh yes, you can indeed. I was just too stuck in the same mindset that code is currently written! The solution could be to simply use @CURLVERSION@ instead of @VERSIONNUM@.

Alternatively, we remove all dependencies (even on sh) and go with #3154

@danielgustafsson
Copy link
Member

We can indeed use @CURLVERSION@, but we still don't get away from requiring a shell which has POSIX cut and test utils, and I don't think that's entirely representative of the platforms where curl builds. (granted, a tool like curl-config has perhaps the most use in a unix environment toolchain, but that's not to say that someone with a build from MSVC can't find it useful)

@dimpase
Copy link
Author

dimpase commented Oct 25, 2018

Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

4 participants