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

Remove dependency on gnu sed #1716

Closed
wants to merge 1 commit into from
Closed

Remove dependency on gnu sed #1716

wants to merge 1 commit into from

Conversation

pcj
Copy link
Member

@pcj pcj commented Sep 2, 2016

BSD sed (the default on osx) does not support the -r --regexp-extended flag. Fixes #1651

BSD sed (the default on osx) does not support the -r --regexp-extended flag.  Fixes bazelbuild#1651
@bazel-io
Copy link
Member

bazel-io commented Sep 2, 2016

Can one of the admins verify this patch?

@pcj
Copy link
Member Author

pcj commented Sep 2, 2016

@damienmg Pls review or assign. Thx.

@damienmg damienmg assigned aehlig and unassigned aehlig Sep 6, 2016
@damienmg
Copy link
Contributor

damienmg commented Sep 6, 2016

jenkins: test this please

DOCKER_MAJOR_VERSION=$(echo "$FULL_DOCKER_VERSION" | sed -r 's#^([0-9]+)\..*#\1#')
DOCKER_MINOR_VERSION=$(echo "$FULL_DOCKER_VERSION" | sed -r 's#^[0-9]+\.([0-9]+).*#\1#')
DOCKER_MAJOR_VERSION=$(echo "$FULL_DOCKER_VERSION" | awk -F\. '{ print $1 }')
DOCKER_MINOR_VERSION=$(echo "$FULL_DOCKER_VERSION" | awk -F\. '{ print $2 }')
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to be more portable? Is awk more stable than sed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both awk and sed are in POSIX, and would be expected to be present everywhere bazel would run, including busybox. Both awk and sed are present in the bazel codebase, with sed outnumbering awk by a factor of 4.

Is awk more portable/stable then sed? Hard to say. What I can say is that the -r sed option is not portable, so this is an improvement and IMHO clearer to read.

@damienmg damienmg self-assigned this Sep 6, 2016
@damienmg
Copy link
Contributor

damienmg commented Sep 6, 2016

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants