librdkafka icon indicating copy to clipboard operation
librdkafka copied to clipboard

Remove small bashism from Makefile

Open paravoid opened this issue 2 years ago • 5 comments
trafficstars

The LICENSES.txt target makes a shell for loop, in which it tries to evalute the wildcard "LICENSE.*[^~]".

[^] is a bashism, and fails when /bin/sh is not bash (i.e. every Debian-based system by default):

$ /bin/bash -c "ls LICENSE.*[^~]" LICENSE.cjson LICENSE.fnv1a LICENSE.lz4 LICENSE.pycrc LICENSE.regexp LICENSE.tinycthread LICENSE.crc32c LICENSE.hdrhistogram LICENSE.murmur2 LICENSE.queue LICENSE.snappy LICENSE.wingetopt

$ /bin/sh -c "ls LICENSE.[^~]" ls: cannot access 'LICENSE.[^~]': No such file or directory

The equivalent POSIX way to do this is to use [!].

Tested with bash, dash and posh.

paravoid avatar Jan 12 '23 02:01 paravoid

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

cla-assistant[bot] avatar Aug 21 '23 15:08 cla-assistant[bot]

It looks like section 2 of your CLA requires for me to assign (way too) many of my rights to Confluent, including the right to relicense the work to a proprietary license (i.e. the ability to pull a Hashicorp). I don't consider this reasonable for what is an unpaid/volunteer contribution, and as such I don't intend to sign such an agreement.

Feel free to consider this as a bug report rather than a PR and do a clean room implementation of it, or close without fixing it. (Note that in this case we'll have to carry this bug in Debian indefinitely, as it's necessary for librdkafka to build in Debian).

paravoid avatar Aug 21 '23 15:08 paravoid

@paravoid thanks, we'll do the fix. We don't have this problem when we're building Confluent distributed packages, as we're using bash, but I acknowledge the need to distribute the package in Debian using the default repository.

Is that the only thing that prevents you from building the package?

emasab avatar Jun 04 '24 15:06 emasab

@paravoid thanks, we'll do the fix. We don't have this problem when we're building Confluent distributed packages, as we're using bash, but I acknowledge the need to distribute the package in Debian using the default repository.

There are no guarantees that /bin/sh is "bash" in any system, really. If the Makefile depends on a bash-specific behavior, it can alternatively declare SHELL := /bin/bash. But given how limited this bashism is, the fix I've proposed is easier :)

Is that the only thing that prevents you from building the package?

Yes this is the only patch we carry. To be clear, it's not "preventing" us; we do have librdkafka in Debian, ever since I uploaded it in 2013 or so :wink: It's just that we've just been carrying this patch locally in the Debian package since the bashism was introduced last year, and I thought it'd be good to submit it here for others to benefit from it, too.

paravoid avatar Jun 04 '24 16:06 paravoid

There are no guarantees that /bin/sh is "bash" in any system, really.

Sure, I never said so

emasab avatar Jun 06 '24 07:06 emasab

@paravoid thanks, we'll do the fix.

@emasab Ping, since I have your attention :wink:

paravoid avatar Sep 01 '25 14:09 paravoid

Done here: https://github.com/confluentinc/librdkafka/pull/5184

I'll need an internal review for this one and #5183 before merging.

emasab avatar Sep 01 '25 16:09 emasab