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

Adds detection of BusyBox v1.30.0+ with updated timeout arguments #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iturgeon
Copy link
Contributor

@iturgeon iturgeon commented Nov 4, 2019

This fixes #71 - adds support for BusyBox 1.30.0+ and maintains support for versions below.

The problem

BusyBox below v1.30.0 had a non-standard implementation of timeout. BusyBox v1.30.0+ updated timeout to match coreutils. This library was previously checking if timeout belongs to busybox and assuming it uses the non-standard flags.

See mirror/busybox@c9720a7 as noted by @socketpair here: #6

To fix the problem without breaking support for older versions of BusyBox, this adds a version check of BusyBox to determine which version of the flags to use.

Note: the changes use bash's regular expression's BASH_REMATCH to get the job done. For Alpine users, this means you'll have to install bash and execute wait-for-it using bash.

The last time I contributed to wait-for-it, it was not compatible with ash, but things may have changed. If requiring bash is a problem, just say so.

I've only tested on Alpine 3.1 and 3.10 w/ bash, but welcome additional testing and feedback.

Fixes #71

Busybox updated timeout's flags to match coreutils.
This broke the script that was specifically dealing
with BusyBox being different from coreutils.

To fix the problem without breaking support for older
versions of busy box, this a version check of busybox
to determine which version of the flags to use.

Note, the changes use bash's regular expression's BASH_REMATCH
to get the job done.  For Alpine users, this means you'll have
to install bash and execute wait-for-it using bash.
else
WAITFORIT_ISBUSY=0
WAITFORIT_BUSYTIMEFLAG=""
# see if timeout.c args have been updated in busybox v1.30.0 or newer
Copy link

@robertp-indeed robertp-indeed Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just read timeout --version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's possible. It's just that timeout, at least on busybox, doesn't have a --version flag.

timeout --version
unrecognized option `--version'
BusyBox v1.22.1 (2015-03-20 11:09:37 GMT) multi-call binary.` 

However, it does output the busybox version info in the error, so it could still be used.

It was just a judgment call to use busybox itself to gather the version info since relying on timeout error output felt slightly worse.

Copy link

@robertp-indeed robertp-indeed Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, that's a great reason. Looks good to me, then!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not get what is expected by "timeout.c" (".c")
Is it a misprint?

@bukowa
Copy link

bukowa commented Jan 24, 2020

Great thank you!

# see if timeout.c args have been updated in busybox v1.30.0 or newer
# note: this requires the use of bash on Alpine
if [[ $WAITFORIT_ISBUSY && $(busybox | head -1) =~ ^.*v([[:digit:]]+)\.([[:digit:]]+)\..+$ ]]; then
if [[ ${BASH_REMATCH[1]} -le 1 && ${BASH_REMATCH[2]} -lt 30 ]]; then
Copy link
Contributor

@kolomenkin kolomenkin Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This simplified version comparison looks not good since it can detect BusyBox 0.60 as new version.

However BusyBox 1.0.0-pre1 was release in 2003. It is unlikely somebody will use it in 2020 with latest wait-for-it.sh
https://busybox.net/oldnews.html

And for future versions of BusyBox this code will work OK.

WAITFORIT_ISBUSY=0
WAITFORIT_BUSYTIMEFLAG=""
# see if timeout.c args have been updated in busybox v1.30.0 or newer
# note: this requires the use of bash on Alpine
Copy link
Contributor

@kolomenkin kolomenkin Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know wait-for-it.sh already needs bash in Alpine.
So there is no need to state this here additionally and frighten reviewers.

@alallier
Copy link

This works and is backwards compatible, but it breaks the current tests that got merged into upstream after this was made

mmorales08
mmorales08 approved these changes Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

not working on alpine 3.10 with bash installed
6 participants