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

Make bin/dist_get fall back to other downloaders if one fails #3692

Merged
merged 3 commits into from
Mar 2, 2017

Conversation

mateon1
Copy link
Contributor

@mateon1 mateon1 commented Feb 15, 2017

Also refactored the code a bit, and added httpie.
Note: I used redirection for httpie, as http -d url -o output complained its output was redirected.

License: MIT
Signed-off-by: Mateusz Naściszewski <[email protected]>
License: MIT
Signed-off-by: Mateusz Naściszewski <[email protected]>
bin/dist_get Outdated
url="$1"
output="$2"
command="$3"
util_name=$(set -- $3; echo $1)
Copy link
Member

Choose a reason for hiding this comment

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

What is this doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sets arguments within this shell substitution from $3, allowing us to get the first word of the command (wget, curl, http) with $1.
Arguments outside this shell substitution are not clobbered.

Copy link
Member

Choose a reason for hiding this comment

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

This might be reason it fails on circle, or other bashizm. CircleCI uses fsh as sh bin. You probably should change the hashbang to bash or remove the bashizm (preferably the latter)

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Thanks @mateon1! I think this looks good, my only concern is what happens when one tool fails for some network or unrelated error (i.e. the tool works, but something else failed randomly).

@whyrusleeping
Copy link
Member

Very weird circleCI failure... Worth investigating

@Kubuxu
Copy link
Member

Kubuxu commented Feb 16, 2017

List of issues found by shellcheck, some of the might not be because of this PR.


In dist_get line 9:
	type "$1" > /dev/null 2> /dev/null
        ^-- SC2039: In POSIX sh, 'type' is undefined.


In dist_get line 20:
	util_name=$(set -- $3; echo $1)
                           ^-- SC2086: Double quote to prevent globbing and word splitting.
                                    ^-- SC2086: Double quote to prevent globbing and word splitting.


In dist_get line 22:
	if ! have_binary $util_name; then
                         ^-- SC2086: Double quote to prevent globbing and word splitting.


In dist_get line 40:
	test "$#" -eq "2" || die "download requires exactly two arguments, was given $@"
                                                                                     ^-- SC2145: Argument mixes string and array. Use * or separate argument.


In dist_get line 68:
				cat "$ua_infile" | tar -O -z -x -f - "$ua_distname/$ua_distname" > "$ua_outfile"
                                    ^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.


In dist_get line 158:
if [ $? -ne 0 ]; then
     ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.


In dist_get line 163:
if [ $? -ne 0 ]; then
     ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

@Kubuxu
Copy link
Member

Kubuxu commented Feb 16, 2017

You can try running Bash in a mode closer to POSIX with --posix flag.

The bashizm is only my guess why things failed, I am not sure.

@mateon1
Copy link
Contributor Author

mateon1 commented Feb 16, 2017

@Kubuxu That is odd, as the set -- command seems to work for me in both sh and bash --posix.
I am going to push some changes that fix shellcheck errors, at least those caused by me.

License: MIT
Signed-off-by: Mateusz Naściszewski <[email protected]>
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Tests look a bit greener this time. You happy with this one @Kubuxu ?

@whyrusleeping whyrusleeping added this to the Ipfs 0.4.7 milestone Feb 21, 2017
@whyrusleeping whyrusleeping merged commit ea8e0f5 into ipfs:master Mar 2, 2017
@mateon1 mateon1 deleted the fix/dist-get branch March 2, 2017 07:17
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