Skip to content

add auto-update for the d-keyring #457

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 2, 2020
Merged

add auto-update for the d-keyring #457

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
921fed9
add auto-update for the d-keyring
Jun 13, 2020
e6cc835
correct some Travis CI / shellcheck errors
Jun 20, 2020
afe6c9f
correct some Travis CI / shellcheck errors.. check is now OK due to h…
Jun 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions script/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
# Authors: Martin Nowak
# Documentation: https://dlang.org/install.html

# set -x
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't have been committed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

forgot this after "debugging", but then again, it is commented out and shouldn't harm, that's why I finally decided to not delete it again. anyhow, feel free to remove it...


_() {

# Returns false if the script is invoked from a Windows command prompt.
Expand Down Expand Up @@ -80,7 +82,7 @@ retry() {
for i in {0..4}; do
if "$@"; then
break
elif [ $i -lt 4 ]; then
elif [ "$i" -lt 4 ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

i is a number, not a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct.
if I'm nor wrong, I had it like you suggest it now initially but "shellcheck" complained so I made it as it is... also here, feel free to change it to whatever fit's your standards / requirements...

sleep $((1 << i))
else
fatal "Failed to download '$url'"
Expand Down Expand Up @@ -407,6 +409,14 @@ parse_args() {

# run_command command [compiler]
run_command() {
# check if tools where installed already and update d-keyring.gpg if necessary
local tmp
tmp=$(mkdtemp)
if [ -f "$ROOT/d-keyring.gpg" ] && [ "$(command curl https://dlang.org/d-keyring.gpg -z "$ROOT/d-keyring.gpg" -o "$tmp/d-keyring.gpg" -s -L -w "%{http_code}")" == "200" ] ; then
Copy link
Contributor

@wilzbach wilzbach Sep 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is the real problem:

  1. This probably needs retry logic
  2. Probably needs the fallback mirrors
  3. Should verify the authenticity
  4. Shouldn't re-download the file(s) if they haven't changed
  5. Needs to update install.sh as well (the script changes a lot more often than the keyring)
  6. Needs to provide the user an option to opt-out
  7. Should probably use "Last-Modified", "ETag" or similar to avoid unnecessary downloads
  8. Auto-updates need to be documented in help page, documentation and changelog (there are serious security concerns with it)
  9. Should have a test

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 all together is far beyond my bash foo and also the time I can / want spend on this topic, sorry...
nevertheless, some comments from here...

  1. => make's sense but I couldn't find a (smart & easy) way how to check if the file was changed without downloading it. actually I did read plenty of "curl way" post / blogs, i.e. https://blog.yjl.im/2012/03/downloading-only-when-modified-using.html and https://www.thegeekstuff.com/2012/04/curl-examples/ but at the end it seems like curl ALLWAYS downloads the file to be able to check for a change... and as it's downloaded anyhow, I decided to implement it as it is

  2. => well, if the script itself should also be updated, neither this change nor the "update" option of the script makes any sense... because in other words you ask to make the already existing"update" option an always applied defauld (from my point of view, I wouldn't like this kind of change... I want to know and control when and why a tool gets updated)

  3. => see my comments on topic nr4

mv "$tmp/d-keyring.gpg" "$ROOT/d-keyring.gpg"
log "auto-updated $ROOT/d-keyring.gpg"
fi

case $1 in
install)
check_tools curl
Expand Down Expand Up @@ -494,13 +504,13 @@ Run \`$(display_path "$ROOT/$2/activate.bat")\` to add $2 to your PATH."
}

install_dlang_installer() {
local tmp mirrors
local tmp mirrors keyring_mirrors
tmp=$(mkdtemp)
local mirrors=(
mirrors=(
"https://dlang.org/install.sh"
"https://s3-us-west-2.amazonaws.com/downloads.dlang.org/other/install.sh"
)
local keyring_mirrors=(
keyring_mirrors=(
"https://dlang.org/d-keyring.gpg"
"https://s3-us-west-2.amazonaws.com/downloads.dlang.org/other/d-keyring.gpg"
)
Expand Down