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

Allow the build system to work without bash. #209

Closed
wants to merge 2 commits into from
Closed

Allow the build system to work without bash. #209

wants to merge 2 commits into from

Conversation

rsmarples
Copy link
Contributor

Tested on NetBSD using pkgsrc without bash installed.

@rsmarples rsmarples requested a review from a team as a code owner July 9, 2019 16:49
Don't set -o pipefail as that's bash specific with no equivalent.
@isaacs
Copy link
Contributor

isaacs commented Jul 9, 2019

I'm not sure why this is necessary. The team that regularly runs these commands all use computers with bash installed. My concern would be that making these shell scripts fully sh-compliant imposes a constraint that we can't guarantee and won't notice violating in the future. (For example, /bin/sh on a Mac laptop is typically Bash in posixly-correct sh-emulating mode.)

The only script that would really need to be portable is install.sh, which is sort of deprecated as the default way to install npm anyway.

Is there a use-case I'm missing? Have you run into issues working with npm on systems that don't have Bash installed?

@rsmarples
Copy link
Contributor Author

My concern would be that making these shell scripts fully sh-compliant imposes a constraint that we can't guarantee and won't notice violating in the future. (For example, /bin/sh on a Mac laptop is typically Bash in posixly-correct sh-emulating mode.)

That's no different from using something provided by Linux but not present on NetBSD.
The interested people fix it - the important thing is that it still works for you.
Packagers are quite used to handling these issues downstream, but we would rather fix them upstream.

Can you guarantee bash version compliance? You could use the =~ operator in the future and that doesn't work with older versions of bash. Also, where is bash installed? On my hosts with bash it's in /usr/pkg/bin/bash.

Anyway, for the most part just changing from bash to sh still allows the scripts to mostly work - it's just the use of arrays that's the unportable part. String contatenation and wordsplitting solves this.

Is there a use-case I'm missing? Have you run into issues working with npm on systems that don't have Bash installed?

The systems I use npm on do infact have bash and npm works fine as is.
However, the new build host that makes the npm package from source does not and I thought I would have a go at addressing this upstream.

@isaacs
Copy link
Contributor

isaacs commented Jul 10, 2019

I'd recommend installing a modern (>= v3.x) version of Bash on your build machine. It's a relatively small binary, compared to the needs of npm (node, python, etc.) Maintaining this patch will be too likely to break in the future, so I'm going to close it.

Sorry for the inconvenience and wasted time. I hope this won't dissuade you from contributing in the future :)

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.

2 participants