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

misc: Fix bash path in shebangs #16494

Merged
merged 1 commit into from Aug 27, 2017

Conversation

Projects
None yet
4 participants
@asomers
Copy link
Contributor

commented Jul 21, 2017

/bin/bash is a Linuxism. Other operating systems install bash to
different paths. Use /usr/bin/env in shebangs to find bash.

@asomers asomers force-pushed the asomers:bin_bash branch from 2627023 to 5e44e39 Jul 21, 2017

@asomers

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2017

CC @wjw , who's done a lot of work on Ceph portability

# -*- mode:shell-script; tab-width:8; sh-basic-offset:2; indent-tabs-mode:t -*-
# vim: ts=8 sw=8 ft=bash smarttab

source $(dirname $0)/../ceph-helpers.sh

set -e
set -ex

This comment has been minimized.

Copy link
@wjwithagen

wjwithagen Jul 21, 2017

Contributor

@asomers
This change silences the -x on the ceph-helpers.sh, not sure you want to change behaviour in such an invasive patch.

@@ -1,4 +1,4 @@
#!/bin/bash -e
#!/usr/bin/env bash

This comment has been minimized.

Copy link
@wjwithagen

wjwithagen Jul 21, 2017

Contributor

@asomers
Missing -e

This comment has been minimized.

Copy link
@asomers

asomers Jul 22, 2017

Author Contributor

Actually, not. There's already a redundant "set -e" on line 8

@@ -1,4 +1,4 @@
#!/bin/bash -e
#!/usr/bin/env bash

This comment has been minimized.

Copy link
@wjwithagen

wjwithagen Jul 21, 2017

Contributor

@asomers
Missing -e

This comment has been minimized.

Copy link
@asomers

asomers Jul 22, 2017

Author Contributor

Likewise, there's already a redundant set -e on line 9

@wjwithagen

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

@wjwithagen wjwithagen requested review from tchaikov and wjwithagen Jul 21, 2017

@asomers asomers force-pushed the asomers:bin_bash branch from 5e44e39 to a269a58 Jul 24, 2017

@asomers

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2017

It turns out that some submodules also use /bin/bash shebangs. So I'm reverting the changes I made to README.FreeBSD. But the other changes are still good.

@asomers asomers force-pushed the asomers:bin_bash branch from a269a58 to 7048df5 Jul 24, 2017

@wjwithagen

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

jenkins retest please

@wjwithagen

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

@asomers
Hi Alan,
You need to remove ceph-disk from the commit, since the code that had a bash ref in it has gone.

@asomers asomers force-pushed the asomers:bin_bash branch 2 times, most recently from 96c6323 to d02bdf3 Jul 26, 2017

@@ -14,7 +14,8 @@
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Library Public License for more details.
#
source $CEPH_ROOT/qa/standalone/ceph-helpers.sh
set -x
source $CEPH_ROOT/qa/workunits/ceph-helpers.sh

This comment has been minimized.

Copy link
@wjwithagen

wjwithagen Jul 27, 2017

Contributor

@asomers
You are moving code back to workunits, were a few commits back things got moved to standalone.

This comment has been minimized.

Copy link
@asomers

asomers Jul 27, 2017

Author Contributor

grahh, missed one. I'll fix it.

@@ -1,4 +1,4 @@
#!/bin/bash -x
#!/usr/bin/env bash

set -e
set -x

This comment has been minimized.

Copy link
@wjwithagen

wjwithagen Jul 27, 2017

Contributor

@asomers
Why not join them on one line?

This comment has been minimized.

Copy link
@asomers

asomers Jul 27, 2017

Author Contributor

Sure, I could do that too.

@@ -1,4 +1,4 @@
#!/bin/bash
#!/usr/bin/env bash

set -e
set -x

This comment has been minimized.

Copy link
@wjwithagen

wjwithagen Jul 27, 2017

Contributor

@asomers Dito, unless you only want to change what really needs to change.

@wjwithagen wjwithagen added the needs-qa label Jul 27, 2017

@wjwithagen

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

@tchaikov
Could you take a look at this as well, and run it thru QA?

scripts: fix bash path in shebangs
/bin/bash is a Linuxism.  Other operating systems install bash to
different paths.  Use /usr/bin/env in shebangs to find bash.

Signed-off-by: Alan Somers <asomers@gmail.com>

@asomers asomers force-pushed the asomers:bin_bash branch from d02bdf3 to 3aae5ca Jul 27, 2017

@liewegas liewegas changed the title Fix bash path in shebangs misc: Fix bash path in shebangs Aug 8, 2017

@wjwithagen

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2017

retest this please

@@ -1,4 +1,4 @@
#! /bin/bash
#! /usr/bin/env bash

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 18, 2017

Contributor

i think systemd is used by Linux only. so this change is not necessary?

This comment has been minimized.

Copy link
@wjwithagen

wjwithagen Aug 18, 2017

Contributor

@tchaikov
I happy to say it is Linux only...
The amount of unexpected irritation I hear from my colleague makes me a content init user.

@liewegas

This comment has been minimized.

Copy link
Member

commented Aug 26, 2017

@tchaikov ok to merge?

@tchaikov tchaikov merged commit 30b5b46 into ceph:master Aug 27, 2017

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@asomers asomers deleted the asomers:bin_bash branch Sep 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.