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

alt bash path support #1013

Merged
merged 1 commit into from
Aug 3, 2021
Merged

Conversation

hackacad
Copy link
Contributor

Signed-off-by: hackacad [email protected]

Description

Be more liberal about the location of bash

Issues Resolved

required for upstream FreeBSD support

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: hackacad <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed fbc72c2

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success fbc72c2

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success fbc72c2

@dblock
Copy link
Member

dblock commented Jul 27, 2021

start gradle check

@dblock
Copy link
Member

dblock commented Jul 27, 2021

So what could go wrong when changing #!/bin/bash to #!/usr/bin/env bash? Is there anything written about what the shebang is supposed to be across *nix'es?

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success fbc72c2
Log 352

Reports 352

@hackacad
Copy link
Contributor Author

Using env is the recommended way in POSIX.
https://www.google.com/search?tbm=bks&q=linux+unix+shell+script+usr+bin+env

Using hard coded path to bash is a common issue (of course, there's a built-in shebang_fix in FreeBSD Makefile/built system, but using a env should make it more portable for every nix OS)

(env should always be available on every UNIX/Linux at /usr/bin/env)

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

cc: @nknize for a quick second look, but this makes sense to me

@hackacad hackacad mentioned this pull request Jul 27, 2021
@hackacad
Copy link
Contributor Author

cc: @nknize for a quick second look, but this makes sense to me

just ftr:
this is already used in some OpenSearch scripts like
/dev-tools/atomic_push.sh
/gradlew

@dblock
Copy link
Member

dblock commented Jul 27, 2021

cc: @nknize for a quick second look, but this makes sense to me

just ftr:
this is already used in some OpenSearch scripts like
/dev-tools/atomic_push.sh
/gradlew

Let's then also fix anywhere we're not doing this in this PR?

@hackacad
Copy link
Contributor Author

Let's then also fix anywhere we're not doing this in this PR?

These are the tools that are necessary during/for runtime.
I’m not sure if the other ones have any dependencies or checksums which might be effected then.
I’ll have a look at it.

@hackacad
Copy link
Contributor Author

hackacad commented Aug 2, 2021

I rethought the idea of using env for start-up scripts. In most cases, this should work, but I'm not aware of every use-case, especially in custom build Linux/Unix systems.
Is there a way of fixing that during build and create a separate FreeBSD version? I created a ticket on FreeBSD side with all necessary fixes.
I'd agree if you refuse that PR, which compensates for the shebang (that could be resolved during package building).

@dblock
Copy link
Member

dblock commented Aug 3, 2021

I am going to go off https://unix.stackexchange.com/questions/206350/what-is-the-difference-if-i-start-bash-with-bin-bash-or-usr-bin-env-bash and adopt the more portable version that you're suggesting and merge this. Note that I read the concerns of malware preempting bash in PATH, if someone thinks those apply we can always reconsider and revert.

If you have time, mind taking a look at all the other places we use bin/bash, such as dev tools and bulk replace them? Thanks.

@dblock dblock merged commit 0fd14e8 into opensearch-project:main Aug 3, 2021
@dblock
Copy link
Member

dblock commented Aug 3, 2021

Is there a way of fixing that during build and create a separate FreeBSD version? I created a ticket on FreeBSD side with all necessary fixes.

We'll leave that ticket open. Making code portable is the first step, we don't want to be patching bash scripts during packaging unless we must. Next, if we're going to be releasing a FreeBSD version, we will need to be signing binaries and running the massive integration test suite on them, which is definitely possible but I can't promise timelines or releases.

@hackacad hackacad deleted the freebsd_support branch August 4, 2021 18:30
@hackacad hackacad mentioned this pull request Aug 4, 2021
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.

3 participants