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

nix/install: Rewrite to not rely on bashisms. #16

Closed
wants to merge 4 commits into from

Conversation

aszlig
Copy link
Member

@aszlig aszlig commented Aug 31, 2014

Gets rid of [[ ... ]] and provides a fallback to which if the type builtin is not available.

This now longer relies on "set -e", because we're checking the errors carefully by ourselves and properly clean up whenever there was an error.

Also, we no longer download into a temporary file but download, decompress and extract on the fly. The main reason for this is because older tar versions don't either detect the compression type or even do not have -j at all.

Gets rid of [[ ... ]] and provides a fallback to "which" if the "type"
builtin is not available.

Also, this now longer relies on "set -e", because we're checking the
errors carefully by ourselves and properly clean up whenever there was
an error.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Older versions of tar didn't detect the compression type and thus tried
to extract an uncompressed archive and even older versions of tar didn't
even have the -j argument at all, so let's download, decompress and
unpack in one go.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Of course, the install script from Nix itself will use bash, but that's
an entirely different case. The problem with the install instructions
itself is that it even relied on bash for its "<()" syntax.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
@edolstra
Copy link
Member

Is it actually a problem to rely on bashisms? The supported platforms (Linux, Darwin) generally have bash.

BTW, the `wrap()' thingy is necessary to prevent execution of a partially downloaded script.

@edolstra
Copy link
Member

Also, set -e is supported by /bin/sh, right?

@aszlig
Copy link
Member Author

aszlig commented Sep 23, 2014

Indeed, set -e is supported, but it's unnecessary here and only clutters up the script with several || :s.
And I'll re-add the wrap() thingy with a comment.

And yes, on these platforms it's usually not a problem to rely on bashisms, but it shortens the installation command line by two characters and doesn't hurt if we one day should support platforms without bash :-)

As @edolstra pointed out, this is to prevent execution of a partially
downloaded script. So let's reintroduce the wrapper with a comment to
make sure it won't be removed again.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
@edolstra
Copy link
Member

Applied, thanks!

@edolstra edolstra closed this Sep 24, 2014
wmertens added a commit to wmertens/nix that referenced this pull request Sep 24, 2014
edolstra pushed a commit to NixOS/nix that referenced this pull request Sep 24, 2014
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.

None yet

2 participants