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

Adding support for fetching files using wget instead, if curl is unavailable #15351

Merged
merged 1 commit into from Jul 26, 2018

Conversation

Projects
None yet
3 participants
@fusion809
Copy link
Contributor

fusion809 commented Jul 15, 2018

Hi,

This pull is me just making the job of building OpenRA on Linux a little easier, by adding support for using wget instead of curl, should curl not be installed. wget comes pre-installed on a few distros that curl doesn't come pre-installed on (e.g. Ubuntu).

Check list:

  • Make sure that you have read and understand the OpenRA Coding Standard (see https://github.com/OpenRA/OpenRA/wiki/Coding-Standard).
    (N/A, as not editing C# files. Only part that might be applicable is using tabs instead of spaces. I noticed the scripts already used spaces for indentation, so I just copy-pasted the indentation already in use.)
  • Write quality commit messages (see https://chris.beams.io/posts/git-commit/).
    (I even edited my commit message to try to make it more concise and helpful).
  • Only commit changes that directly relate to your Pull Request. Use your Git interface to unstage any unrelated changes to project files, line endings, whitespace, or other files.
  • Review the code diff view below to double check that your changes satisfy the above three points.
  • Use the make test and make check commands to check for (and fix!) any issues that are reported by our automated tests.
    (Tested on openSUSE Tumbleweed 20180712)

I have tried my best here to make sure what I have done complies to the standards of this repository, but if there's something I've done wrong do say and I'll try my best to fix it.

Thanks for your time and hope this helps,
Brenton

@Phrohdoh
Copy link
Member

Phrohdoh left a comment

Looks good, thank you!

My only nit would be to prefer $(...) instead of ticks but I don't believe we have a standard on that yet.

@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 15, 2018

Sorry, what do you mean by you'd prefer $(...) instead of ticks? Do you mean in my checklist?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 15, 2018

https://www.shellcheck.net/ prefers $(...) over ``, but in this case I think we don't want to use either (e.g. if command -v curl >/dev/null 2>&1; then).

I don't think most of the scripts touched here have been shellcheck-cleaned yet - this would be a worthwhile followup PR if you are interested @fusion809.

@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 15, 2018

Oh you mean something like:

if `command -v curl >/dev/null 2>&1`; then

being replaced with: if $(command -v curl >/dev/null 2>&1); then? OK, well first of all I'll apply that change to my changes in this PR.

@fusion809 fusion809 force-pushed the fusion809:patch-1 branch from 46c6433 to eb91d75 Jul 15, 2018

@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 15, 2018

There we go, I squashed the two commits into one as the `` vs $(...) issue only arose due to my first commit.

@@ -55,8 +55,14 @@ popd > /dev/null

# Add native libraries
echo "Downloading dependencies"
curl -s -L -O https://github.com/OpenRA/AppImageSupport/releases/download/${DEPENDENCIES_TAG}/libs.tar.bz2 || exit 3
curl -s -L -O https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage || exit 3
if $(command -v curl >/dev/null 2>&1); then

This comment has been minimized.

@pchote

pchote Jul 15, 2018

Member

^-- SC2091: Remove surrounding $() to avoid executing output.

I guess you missed my comment above while dealing with @Phrohdoh's.

As you've already checked that the user has curl or wget this pattern can simplify further to

echo "Downloading dependencies"
if command -v curl >/dev/null 2>&1; then
    curl -s -L -O https://github.com/OpenRA/AppImageSupport/releases/download/${DEPENDENCIES_TAG}/libs.tar.bz2 || exit 3
    curl -s -L -O https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage || exit 3
else
    wget -cq https://github.com/OpenRA/AppImageSupport/releases/download/${DEPENDENCIES_TAG}/libs.tar.bz2 || exit 3
    wget -cq https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage || exit 3
fi

This comment has been minimized.

@fusion809

fusion809 Jul 15, 2018

Author Contributor

Ah, sorry, 'tis done now.

@fusion809 fusion809 force-pushed the fusion809:patch-1 branch from 882fee8 to 3ad22ae Jul 15, 2018

@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 15, 2018

ShellCheck doesn't seem to like $(...) as it gives the warning:

[shellcheck] Remove surrounding $() to avoid executing output. [SC2091]
@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 15, 2018

ShellCheck does give quite a few warnings, for quite a few scripts, that I could fix, so @pchote I think I may create a second PR after this one is merged.

@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 15, 2018

Maybe I should remove $(...), just leave the command -v curl > /dev/null 2>&1 as the condition? I've tested it out and it works. Well as you folks seem to have gone off to lunch or w/e, I'll just make this change.

@fusion809 fusion809 force-pushed the fusion809:patch-1 branch from 94301ab to 94e2f77 Jul 15, 2018

@pchote
Copy link
Member

pchote left a comment

The changes here look fine now, just need to fix inconsistencies with tabs and spaces (OpenRA style is to use tabs).

@@ -13,6 +13,10 @@ filename="GeoLite2-Country.mmdb.gz"
if [ ! -e $filename ] || [ -n "$(find . -name $filename -mtime +30 -print)" ]; then
rm -f $filename
echo "Updating GeoIP country database from MaxMind."
curl -s -L -O http://geolite.maxmind.com/download/geoip/database/$filename
if command -v curl >/dev/null 2>&1; then
curl -s -L -O http://geolite.maxmind.com/download/geoip/database/$filename

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

These lines mix tabs with (different numbers of!) spaces. Please stick to a single tab for each level of indentation.

curl -s -L -O https://github.com/OpenRA/AppImageSupport/releases/download/${DEPENDENCIES_TAG}/libs.tar.bz2 || exit 3
curl -s -L -O https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage || exit 3
if command -v curl >/dev/null 2>&1; then
curl -s -L -O https://github.com/OpenRA/AppImageSupport/releases/download/${DEPENDENCIES_TAG}/libs.tar.bz2 || exit 3

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

Please use tabs here too.

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

Missed these ones!

fi

if [ ! -f Eluant.dll ]; then
echo "Fetching Eluant from GitHub."
curl -s -L -O https://github.com/OpenRA/Eluant/releases/download/20160124/Eluant.dll
if command -v curl >/dev/null 2>&1; then
curl -s -L -O https://github.com/OpenRA/Eluant/releases/download/20160124/Eluant.dll

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

Tabs.

archive="$1"
version="$2"
curl -o "$archive.zip" -Ls https://nuget.org/api/v2/package/"$archive"/"$version"
if command -v curl >/dev/null 2>&1; then
curl -o "$archive.zip" -Ls https://nuget.org/api/v2/package/"$archive"/"$version"

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

Tabs.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 26, 2018

Would you mind making similar fixes to https://github.com/OpenRA/OpenRAModSDK as well?

@fusion809 fusion809 force-pushed the fusion809:patch-1 branch from cf2e809 to d3f6d54 Jul 26, 2018

@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 26, 2018

I do believe the tab indentation issue is fixed now. OK, I'll go fix it too.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 26, 2018

Looks good now. Can you please rebase and squash the fixup commits?

@fusion809 fusion809 force-pushed the fusion809:patch-1 branch from 0654fcc to 4220a7e Jul 26, 2018

@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 26, 2018

Well I squashed the tab fix commits, but since I pulled the latest commits to this repo (the ones made between the start of this PR and now) that were made between my first commits and my last ones in this PR I don't think I can squash them all together (as wouldn't I squash the commits made to this repo between my first and last commits in this PR? The ones brought in when I merged the bleed branch into my patch-1 branch). If I'm wrong, as I am not a git expert, please do correct me.

@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 26, 2018

Sorry if I'm not explaining it right. I mean since there's been many commits made to this repo since I opened this PR, wouldn't squashing my indentation fix commits into my original commits squash all the commits made to this repo since I started the PR?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 26, 2018

Rebasing will move your commits to be based on the latest merged commit so that you can safely squash them.

@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 26, 2018

Ah what command do I run exactly? Running git rebase -i HEAD~2 (to squash my last two commits together) brings up two commits I didn't make, namely:

pick e9c3927b0c Filter Explodes->DeathTypes when Threshold is above 0.
pick dcf93203ea Get rid of unit pathing delay completely

# Rebase 6073de52ca..dcf93203ea onto 6073de52ca (2 commands)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit
# f, fixup <commit> = like "squash", but discard this commit's log message
# x, exec <command> = run command (the rest of the line) using shell
# d, drop <commit> = remove commit
# l, label <label> = label current HEAD with a name
# t, reset <label> = reset HEAD to a label
# m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
# .       create a merge commit using the original merge commit's
# .       message (or the oneline, if no original merge commit was
# .       specified). Use -c <commit> to reword the commit message.
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
#       However, if you remove everything, the rebase will be aborted.
#
#       
# Note that empty commits are commented out

. Sorry for my ignorance, I haven't done many PRs I'm afraid, so I only know git's basics.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 26, 2018

git fetch upstream
git checkout patch-1
git rebase -i upstream/bleed

should get you to the commit list like you commented above. This should show only the two real commits in this PR. Change the second one to f then save and exit. Then

git push origin +patch-1
@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 26, 2018

I see:

pick 94e2f77b6 Adding wget support to fetch scripts
pick bdc31f45b Update AppImage dependencies to 20180723 tag.
pick ee0bbeab7 Changing space indentation with tab indentation

# Rebase dcf93203e..ee0bbeab7 onto dcf93203e (3 commands)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit
# f, fixup <commit> = like "squash", but discard this commit's log message
# x, exec <command> = run command (the rest of the line) using shell
# d, drop <commit> = remove commit
# l, label <label> = label current HEAD with a name
# t, reset <label> = reset HEAD to a label
# m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
# .       create a merge commit using the original merge commit's
# .       message (or the oneline, if no original merge commit was
# .       specified). Use -c <commit> to reword the commit message.
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
#	However, if you remove everything, the rebase will be aborted.
#
#	
# Note that empty commits are commented out
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 26, 2018

I'm not sure why that AppImage shows, probably something to do with merging instead of resetting/rebasing. Just delete that line continue.

@fusion809 fusion809 force-pushed the fusion809:patch-1 branch from 4220a7e to 1262126 Jul 26, 2018

@fusion809

This comment has been minimized.

Copy link
Contributor Author

fusion809 commented Jul 26, 2018

Thanks, we should be good to go now.

@pchote

pchote approved these changes Jul 26, 2018

@pchote pchote merged commit e1b1070 into OpenRA:bleed Jul 26, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.