-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
implement apt-transport-mirror handling #168
base: master
Are you sure you want to change the base?
Conversation
6350f41
to
feeadc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good, thank you! I haven't applied and tried the patch, I think it doesn't work with a more complex mirror list (apt >= 1.6, see the apt-transport-mirror manpage) but imho a basic support is better than none.
13e79b1
to
dbb7cf5
Compare
The new mirrorlist variable is $MIRRORLIST. The cleanup_dllist function is now refactored to accept some arguments. The refactoring was intended for MIRRORLIST, but it then occurred to me that it was not meant for persistence anyway. So now it gets insta-deleted without need for a cleanup function. TMP_ variables previously used a pretty maintaience-heavy way to be assigned. I changed it to use the "nameref" magic in bash. Basically these variables are read/write proxies to some other variables.
c65f8b3
to
8efd201
Compare
8efd201
to
83f5144
Compare
Alright. It does work on my end now. The grep regex is quite complicated now, but well that's what it takes... |
Got a seven-year-old bug fixed too. |
43233be
to
8fe8ffc
Compare
The value was not updating because it was already evaluated at assign- time. To make it always update we need to make it a function call.
8fe8ffc
to
11c8161
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked over everything, one challenge I see is that this solution requires the MIRRORS variable to be set alongside the apt-transport-mirror setup. Otherwise only the first mirror from that list is picked.
I would suggest to split the shadowing thing up from the rest so we can merge the other chunk where only minimal changes are needed. I also really love the fixes and improvements you did along the way. Thank you!
# I heard that people call this "variable shadowing." | ||
declare -n this="$1" tmpthis="TMP_$1" | ||
tmpthis="${this-${TMP_RANDOM}}" | ||
sudo_exports+=("TMP_$1=$tmpthis") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be without TMP_: sudo_exports+=("$1=$tmpthis")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. There is a for i in "${shadowvars[@]}"
part explicitly designed to unpack the TMP_ prefixed variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just test to overwrite any of the variables via command line (e.g. _APTMGR). (Remove TMP_ to support cmd var precedence)
|
||
local mirrors | ||
mapfile -t mirrors < "$mirrorfile" | ||
[ -f "MIRRORLIST" ] && rm -f "$MIRRORLIST" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing $MIRRORLIST
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dollar sign was missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!!!
[ -f "MIRRORLIST" ] && rm -f "$MIRRORLIST" | |
[ -f "$MIRRORLIST" ] && rm -f "$MIRRORLIST" |
I tested the apt-transport-mirror with following configuration: Sources list:
The local(host) mirrorlist(s):
|
I tested the shadowing with following test cases:
|
Hi, any update for this PR? or issue #88 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
README requires new entry to MIRRORLIST in config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor changes required. Mainly to fix the automatic sudo and cmd vars.
@@ -101,6 +104,14 @@ | |||
#DLLIST=/tmp/apt-fast.list | |||
|
|||
|
|||
# Mirror listfile | |||
# This determines where temporary resolutions for the mirror:// url is stored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url are stored
ftp_proxy="$TMP_ftp_proxy" \ | ||
http_proxy="$TMP_http_proxy" \ | ||
https_proxy="$TMP_https_proxy" \ | ||
exec sudo "${sudo_exports[@]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing '\' at end of line
# I heard that people call this "variable shadowing." | ||
declare -n this="$1" tmpthis="TMP_$1" | ||
tmpthis="${this-${TMP_RANDOM}}" | ||
sudo_exports+=("TMP_$1=$tmpthis") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just test to overwrite any of the variables via command line (e.g. _APTMGR). (Remove TMP_ to support cmd var precedence)
close #88