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

Comments

Projects
None yet
4 participants
@dimpase

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

infinnovation-dev commented Oct 17, 2018

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

This comment has been minimized.

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

This comment has been minimized.

infinnovation-dev commented Oct 17, 2018

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

This comment has been minimized.

dimpase commented Oct 17, 2018

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

@bagder

This comment has been minimized.

Member

bagder commented Oct 18, 2018

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

@bagder

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

danielgustafsson commented Oct 22, 2018

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)

bagder added a commit that referenced this issue Oct 25, 2018

@bagder bagder closed this in abfdf6a Oct 25, 2018

@dimpase

This comment has been minimized.

dimpase commented Oct 25, 2018

Thanks!

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