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

Bugfix: Replace bashisms with standard sh to fix build on non-BASH systems #5038

Merged
merged 4 commits into from Oct 7, 2014

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Oct 3, 2014

Note: I have not actually tested this on a non-BASH system yet.

Downstream Gentoo bug: https://bugs.gentoo.org/show_bug.cgi?id=524332

@theuni
Copy link
Member

theuni commented Oct 3, 2014

ACK in principle. As you said on IRC, I'd like to have travis testing for this. Shouldn't be too hard to spawn in a new shell and export a few things as needed.

One request though.. please split into 3 commits: build/tests/gitian, that way we can backport more easily if needed.

@luke-jr
Copy link
Member Author

luke-jr commented Oct 3, 2014

Split

@theuni
Copy link
Member

theuni commented Oct 3, 2014

@luke-jr Please see #5040 and grab the commit from there if you agree with it (I'll close that PR).

If that is green, ACK.

@theuni
Copy link
Member

theuni commented Oct 6, 2014

In case it's non-obvious to anyone reading after-the-fact:

This PR adds fixes for shell-scripts that needlessly used bashisms. There was previously no test for these. #5040 introduced a test, and the build failed as expected. This PR fixes the scripts and adds the test from #5040. The fact that it's green means that they're confirmed fixed.

ACK.

@gmaxwell
Copy link
Contributor

gmaxwell commented Oct 7, 2014

utACK.

@laanwj
Copy link
Member

laanwj commented Oct 7, 2014

Not meaning to depreciate this pull, but is there really no modern non-bash alternative to uglyness like

if test x$use_pkgconfig = x; then

I remember, from maybe 20 years ago, that constructions like that were used for compatibility with some shell from the 80's. Hasn't really anything changed on that front?

@luke-jr
Copy link
Member Author

luke-jr commented Oct 7, 2014

@laanwj I'm pretty sure

if test "$use_pkgconfig" = ""; then

is fine in most shells, but when the entire purpose of a configure script is portability, it makes sense to use the most portable code.

@laanwj
Copy link
Member

laanwj commented Oct 7, 2014

It's just so stupid to have to add a dummy string to do a string comparison, as if it's some amazing new thing. In other places we also have limits on how far we go back with regard to compatibility.

Anyhow, I was just asking. For anything worthwhile (that is actually meant to be reviewed) we should be using Python, not shellscript.

utACK.

@laanwj laanwj merged commit 0b17964 into bitcoin:master Oct 7, 2014
laanwj added a commit that referenced this pull request Oct 7, 2014
0b17964 Bugfix: Replace bashisms with standard sh in tests/tools (Luke Dashjr)
ab72068 Bugfix: Replace bashisms with standard sh in gitian descriptors (Luke Dashjr)
b77b4ed Bugfix: Replace bashisms with standard sh to fix build on non-BASH systems (Luke Dashjr)
d6b0539 travis: add non-default shell testing to travis. (Cory Fields)
laanwj added a commit to laanwj/bitcoin that referenced this pull request Oct 7, 2014
@Michagogo
Copy link
Contributor

Um, why did we make the changes within the gitian build scripts? Those, by definition, run on Ubuntu 12.04, and with bash, unless I'm missing something. I just checked gitian's source, and the build script has a hard-coded #!/bin/bash, as well as the fact that the actual command run is on-target setarch #{@arches[arch]} bash -x < var/build-script > var/build.log 2>&1

@luke-jr
Copy link
Member Author

luke-jr commented Oct 8, 2014

Better to use standards?

@Michagogo
Copy link
Contributor

It just seems weird to me to make it ugly in the name of compatibility/reducing bashisms, in a script that will only ever be run by bash.

@laanwj
Copy link
Member

laanwj commented Oct 8, 2014

@Michagogo Agreed. Wouldn't have been needed for the gitian descriptors as it forces bash. It's just the basic build system that needs to be compatible between shells. @theuni We could revert this with #4727, which completely overhauls the gitian descriptors anyway.

@theuni
Copy link
Member

theuni commented Oct 8, 2014

Yea, all of the above is fine by me. I didn't argue it because they should be obsoleted soon anyway, as you said.

@luke-jr luke-jr deleted the fix_bashisms branch October 9, 2014 01:54
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants