Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

Avoid using GNUisms and BASHisms #2537

Closed
wants to merge 2 commits into from
Closed

Conversation

grimreaper
Copy link
Contributor

This should be a portable project and should run without relying on bash.

/bin/bash is always wrong
/usr/bin/env bash is sometimes correct
/bin/sh is always correct

@callahad
Copy link
Contributor

I haven't reviewed, but I'm definitely +1 for this sort of change.

@matthewp
Copy link

If the goal is portability we should also rewrite the Makefile in something non-posix. Perhaps ShellJs (created by a Mozillian I believe): https://github.com/arturadib/shelljs

@h4ck3rm1k3
Copy link

Bash and gnu are very portable, where dont they run?

@grimreaper
Copy link
Contributor Author

bash is not installed by default on FreeBSD. In addition it should never be installed in /bin on FreeBSD. I believe the same is true for { net, open, dragonfly } bsd. Many operating systems prefer to not use GNU software and specifically avoid GNU extensions.

Maybe people prefer using zsh instead of bash, and may even deinstall bash.

bashisms are not portable. gnuisms are not portable. POSIX is generally portable (except if you care about windows). please don't give into the embrace, extend, and extinguish model. ;)

@h4ck3rm1k3
Copy link

umm, prefer is one thing. I am talking about portability not politics. Show me where gnu bash does not run or cannot be made to run.

@seanmonstar
Copy link
Contributor

@matthewp that is awesome! Being the only Windows dev on browserid, that's pretty important to me. I'll get on it so we use shelljs.

@grimreaper
Copy link
Contributor Author

h4ck3rm1k3: this has nothing to do with "can be made to run" - this about using standards compliant shell scripting technique. Many operating systems do not install bash by default and users should not be required to do in order to run a simple shell script.

Show me one platform Internet Explorer can not be made to run with wine. Should we use IE specific code because "IE can be made to run" ?

Remember we are talking about portability, not politics: use standards compliant code, not bash extensions. (p.s. the use of ";)" indicates a joke)

@h4ck3rm1k3
Copy link

Ok @grimreaper sounds fair to me.
If you want to make an build system/configure that runs out of the box and does not need gnu, have you looked at cmake?
we are talking about build scripts here? I am just learning about the js world, and come from the c world.
Normally projects use automake/autoconf or cmake or scons to create the build system and it does this in a portable way.
I have not reviewed all bash scripts but sure, writing bash scripts might not be a good idea if you have better tools for it, porting them to a javascript shell sounds funky but i dont know if it the right direction. I will need some time to get up to speed here. Just trying to help.
mike

@h4ck3rm1k3
Copy link

Oh, now i see the commits, yeah that looks fine to me! I did not see that this was a pull but just jumped into the discussion about bash.
mike

@callahad
Copy link
Contributor

If it ain't broke... ShellJS looks cool, and probably makes sense for some of our internal, administrative scripts, but I'm not sure if we'll be able to use it everywhere. I'd be in favor of merging these changes (pending review) and resolving this bug independent of a move to ShellJS.

@seanmonstar
Copy link
Contributor

@callahad Maybe not in every single script (though, eventually, why not?), but some of the scripts are indeed broken on Windows (test_backend, for example).

@callahad
Copy link
Contributor

@lloyd will review and merge this once we double-check that this totally works on our end.

Thanks for the pull request, @grimreaper !

@lloyd
Copy link
Contributor

lloyd commented Oct 11, 2012

code looks great, if anyone wants to verify the scripts execute fine on linux and macos we can hit the green button, otherwise I'll try to make time to manually test shortly.

@ghost ghost assigned lloyd Nov 1, 2012
@lloyd
Copy link
Contributor

lloyd commented Jan 23, 2013

tested, manually merging. r+

@lloyd lloyd closed this in 9aeec47 Jan 23, 2013
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

6 participants