Skip to content

Comments

Use fallback mirrors in the install script#291

Merged
MartinNowak merged 5 commits intodlang:masterfrom
wilzbach:dmd-mirror
Feb 17, 2018
Merged

Use fallback mirrors in the install script#291
MartinNowak merged 5 commits intodlang:masterfrom
wilzbach:dmd-mirror

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Jan 7, 2018

Failures are getting more frequent, see e.g. travis-ci/travis-build#1282
This is a first step at adding more mirrors.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

retry curl2 -sS "$url" -o "$path"
try_all_mirrors() {
for mirror in "${mirrors[@]}" ; do
if curl2 -sS "$mirror" -o "$path" ; then
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 is copy due to curl2 being used here and curl in download. Is there any reason not to use curl2 in download?

mirrors=(
"https://dlang.org/install.sh"
"https://downloads.dlang.org/other/install.sh"
"https://nightlies.dlang.org/install.sh"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would add https://github.com/dlang/installer/raw/stable/script/install.sh to the list too, but I wasn't sure whether you would be okay with it.

Copy link
Member

Choose a reason for hiding this comment

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

Let's not automatically deploy this, it's not worth the security implications.
Even without sudo (which often is cached), rm -rf ~ can do a lot of damage.

verify() {
local keyring_mirrors=(
"https://dlang.org/d-keyring.gpg"
"https://github.com/dlang/dlang.org/raw/846269e913114fd77af0cc1aff67269046b70db8/d-keyring.gpg"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find another mirror for this.

Copy link
Member

Choose a reason for hiding this comment

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

There you go, https://nightlies.dlang.org/d-keyring.gpg.
In fact you can upload files to sebastian@nightlies.dlang.org:/var/www/builds yourself.
But you cannot move or delete my entries in that folder (sticky bit). Allowing any nginx group member to mess with the install script and the signature key sounds like disaster.

@wilzbach
Copy link
Contributor Author

FWIW for gdc downloads (e.g. at dlang/dmd) I still see failures related to the dub-registry being down often, e.g. https://semaphoreci.com/cybershadow/dmd/branches/pull-request-7659/builds/2, as gdc doesn't ship with dub.

So it would be cool to add a mirror for the DUB binaries too (see #273)

wilzbach added a commit to wilzbach/dmd that referenced this pull request Jan 12, 2018
"http://ftp.digitalmars.com/LATEST_BETA"
)
logV "Determing latest dmd-beta version (${mirrors[0]})."
COMPILER="dmd-$(fetch "${mirrors[*]}")"
Copy link
Member

Choose a reason for hiding this comment

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

Think this should be [@] expanded, so fetch really gets 2 arguments.

cnt() { echo $#; }
args=('entry with' 'spaces')
cnt "${args[*]}" # 1
cnt "${args[@]}" # 2
cnt ${args[*]} # 3
cnt ${args[@]} # 3

As with "$@" you always want to pass arguments in an array using `"${ary[@]}".

local url path
url=$1
local mirrors path
mirrors=($1)
Copy link
Member

Choose a reason for hiding this comment

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

This does IFS/whitespace splitting. Rather pass all the arguments separately on the call-side and use ("$@") here.

cnt() { echo $#; }
bug() { ary=($@); cnt "${ary[@]}"; }
bug 1 2 # -> 2
bug '1 1' 2 # -> 3 :/
ok() { ary=("$@"); cnt "${ary[@]}"; }
ok 1 2 # -> 2
ok '1 1' 2 # -> 2 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware (I did it on purpose after all) - urls aren't supposed to contain spaces 😉
I did have this, s.t. I can pack the urls for mirrors and their signatures as individual arguments for the download function.

}

# url, path, [gpg signature url to verify]
# url and the gpg signature can be an array of urls
Copy link
Member

Choose a reason for hiding this comment

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

Let's change it to path, verify, url... then, and always require 0 or 1 for verify.

Copy link
Member

@MartinNowak MartinNowak Jan 16, 2018

Choose a reason for hiding this comment

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

The signature can be assumed to be url+.sig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

log "Downloading $url"
download "$url" "$tmp/install.sh" "$url.sig"
log "Downloading ${mirrors[0]}"
download "${mirrors[*]}" "$tmp/install.sh" "${mirrors[*]/%/.sig}"
Copy link
Member

@MartinNowak MartinNowak Jan 16, 2018

Choose a reason for hiding this comment

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

Again .sig is implicit when verify=1, so the call would be.

download "$tmp/install.sh" 1 "${mirrors[@]}"

Or for better readability, use download_and_verify and plain download.

@MartinNowak
Copy link
Member

LMostlyGTM

@wilzbach wilzbach force-pushed the dmd-mirror branch 2 times, most recently from 72c54ec to 24dffbf Compare January 16, 2018 19:11
@wilzbach
Copy link
Contributor Author

Ping @MartinNowak - are we good here?
People are still seeing random failures on their CI jobs (e.g. https://travis-ci.org/d-widget-toolkit/dwt/builds/334451034). Note in that case the DUB binary failed, but I will revive #273 as soon as the mirroring functionality has been merged. with this PR #273 will be a simple two-line diff.

CC @jacob-carlborg

# path, urls...
download_without_verify() {
download "$1" 0 "${@:2}"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these to functions to avoid the need of passing around a magic 1 or 0


# path, urls...
download_and_unpack_without_verify() {
download_and_unpack "$1" 0 "${@:2}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two functions were added to avoid passing around magic 0 or 1

@wilzbach
Copy link
Contributor Author

wilzbach commented Feb 6, 2018

Ping @MartinNowak - just saw another random failure on Travis :/

- no https on s3
- gh url for keyring needs to be updated in install.sh script (we already have a distribution/update problem for d-keyring.gpg, so let's not add another one) (also shattered.io to a tiny degree)
@MartinNowak MartinNowak merged commit 3e8e46a into dlang:master Feb 17, 2018
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