Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3791007:
Allow custom repository mirrors and configure chroot for distro updates. (Closed)
|
Created:
10 years, 2 months ago by Michael Moss Modified:
9 years, 7 months ago Reviewers:
Markus (顧孟勤) CC:
chromium-reviews, Michael Moss Visibility:
Public. |
DescriptionAllow custom repository mirrors and configure chroot for distro updates.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=62684
Patch Set 1 #
Total comments: 23
Patch Set 2 : whitespace fixes #Patch Set 3 : review cleanups #Patch Set 4 : ignore irrelevant sources.list lines #
Total comments: 2
Patch Set 5 : review fix #Messages
Total messages: 8 (0 generated)
http://codereview.chromium.org/3791007/diff/1/2 File build/install-chroot.sh (right): http://codereview.chromium.org/3791007/diff/1/2#newcode14 build/install-chroot.sh:14: echo "usage: $(basename $0) [-m mirror]" Instead of $(basename $0) you could write ${0##*/}. That should always work, is slightly more efficient as it doesn't require an external tool, and doesn't get hung up on filenames with spaces or leading hyphens. If you insist on using basename, you should instead write $(basename -- "$0"), but that might not be portable. http://codereview.chromium.org/3791007/diff/1/2#newcode19 build/install-chroot.sh:19: process_opts() { Add the following line: local OPTNAME OPTIND OPTERR OPTARG http://codereview.chromium.org/3791007/diff/1/2#newcode20 build/install-chroot.sh:20: while getopts ":m:h" OPTNAME The rest of the code is usually written so that it puts the "do" on the same line as the "while". I don't particularly care either way. Just pointing out the small difference in style. http://codereview.chromium.org/3791007/diff/1/2#newcode22 build/install-chroot.sh:22: case $OPTNAME in ${OPTNAME} can evaluate to a question mark. Better make sure you put double quotes around the variable. http://codereview.chromium.org/3791007/diff/1/2#newcode23 build/install-chroot.sh:23: m ) The rest of the code doesn't have a space between the pattern and the parenthesis. Again, not really a big deal either way. http://codereview.chromium.org/3791007/diff/1/2#newcode24 build/install-chroot.sh:24: mirror="$OPTARG" if [ -n "${mirror}" ]; then echo "You can only specify exactly one mirror location" usage exit 1 fi http://codereview.chromium.org/3791007/diff/1/2#newcode41 build/install-chroot.sh:41: done if [ $# -ge ${OPTIND} ]; then eval echo "Unexpected command line argument: \${${OPTIND}}" usage exit 1 fi http://codereview.chromium.org/3791007/diff/1/2#newcode200 build/install-chroot.sh:200: [ "${alt_repos}" = "y" -a -r /var/lib/chroot/${target}/etc/apt/sources.list ] && I would tend to write [ -n "${alt_repos}" ] instead of [ "${alt_repos}" = "y" ] But shell scripts don't really have standard way to encode boolean values, so your approach is OK, too. Ideally, we should also have quotes around the path, but I realize that I have been a little sloppy about that, too. I guess I assumed that "$target" would never have special characters. That's probably true, but still not good coding style. http://codereview.chromium.org/3791007/diff/1/2#newcode201 build/install-chroot.sh:201: sudo sed -i '/\( [^ -]\+\) main$/p While it is not an error to use \(...\) in an address, it also doesn't actually do anything useful. Also, this pattern won't necessarily match. For example, on Maverick, my sources.list looks like this: deb http://archive.ubuntu.com/ubuntu maverick main restricted universe multiverse deb-src http://archive.ubuntu.com/ubuntu maverick main restricted universe multiverse You'll notice that "main" isn't the last token in each line. http://codereview.chromium.org/3791007/diff/1/2#newcode202 build/install-chroot.sh:202: t1 You cannot use "t" unless you have previously done a "s". This branch will always fail. But even if that wasn't the case, you are branching to the very next instruction. So, I am not sure what you want to do. Unless you are trying to clear the "has any substitution taken place" status. But since you are at the beginning of your script, that status is always going to be false anyway. http://codereview.chromium.org/3791007/diff/1/2#newcode204 build/install-chroot.sh:204: p Since you are not actually running "sed" with the "-n" option, you really don't need to go to the trouble of calling "p" all the time. It is probably easier to just drop off the end of the script. http://codereview.chromium.org/3791007/diff/1/2#newcode209 build/install-chroot.sh:209: d' /var/lib/chroot/${target}/etc/apt/sources.list Would you mind showing me what transformation you actually want to do in your "sed" script. I think, it is currently broken, but since I can't quite figure out what it is supposed to do, I might not be giving any useful comments.
http://codereview.chromium.org/3791007/diff/1/2 File build/install-chroot.sh (right): http://codereview.chromium.org/3791007/diff/1/2#newcode14 build/install-chroot.sh:14: echo "usage: $(basename $0) [-m mirror]" On 2010/10/14 21:54:08, Markus (顧孟勤) wrote: > Instead of $(basename $0) you could write ${0##*/}. That should always work, is > slightly more efficient as it doesn't require an external tool, and doesn't get > hung up on filenames with spaces or leading hyphens. > > If you insist on using basename, you should instead write > $(basename -- "$0"), but that might not be portable. Done. http://codereview.chromium.org/3791007/diff/1/2#newcode19 build/install-chroot.sh:19: process_opts() { On 2010/10/14 21:54:08, Markus (顧孟勤) wrote: > Add the following line: > > local OPTNAME OPTIND OPTERR OPTARG Done. http://codereview.chromium.org/3791007/diff/1/2#newcode20 build/install-chroot.sh:20: while getopts ":m:h" OPTNAME On 2010/10/14 21:54:08, Markus (顧孟勤) wrote: > The rest of the code is usually written so that it puts the "do" on the same > line as the "while". I don't particularly care either way. Just pointing out the > small difference in style. Done. http://codereview.chromium.org/3791007/diff/1/2#newcode22 build/install-chroot.sh:22: case $OPTNAME in On 2010/10/14 21:54:08, Markus (顧孟勤) wrote: > ${OPTNAME} can evaluate to a question mark. Better make sure you put double > quotes around the variable. Done. http://codereview.chromium.org/3791007/diff/1/2#newcode23 build/install-chroot.sh:23: m ) On 2010/10/14 21:54:08, Markus (顧孟勤) wrote: > The rest of the code doesn't have a space between the pattern and the > parenthesis. Again, not really a big deal either way. Done. http://codereview.chromium.org/3791007/diff/1/2#newcode24 build/install-chroot.sh:24: mirror="$OPTARG" On 2010/10/14 21:54:08, Markus (顧孟勤) wrote: > if [ -n "${mirror}" ]; then > echo "You can only specify exactly one mirror location" > usage > exit 1 > fi Done. http://codereview.chromium.org/3791007/diff/1/2#newcode41 build/install-chroot.sh:41: done On 2010/10/14 21:54:08, Markus (顧孟勤) wrote: > if [ $# -ge ${OPTIND} ]; then > eval echo "Unexpected command line argument: \${${OPTIND}}" > usage > exit 1 > fi Done. http://codereview.chromium.org/3791007/diff/1/2#newcode200 build/install-chroot.sh:200: [ "${alt_repos}" = "y" -a -r /var/lib/chroot/${target}/etc/apt/sources.list ] && On 2010/10/14 21:54:08, Markus (顧孟勤) wrote: > I would tend to write > [ -n "${alt_repos}" ] > instead of > [ "${alt_repos}" = "y" ] > But shell scripts don't really have standard way to encode boolean values, so > your approach is OK, too. > > Ideally, we should also have quotes around the path, but I realize that I have > been a little sloppy about that, too. I guess I assumed that "$target" would > never have special characters. That's probably true, but still not good coding > style. Done. http://codereview.chromium.org/3791007/diff/1/2#newcode201 build/install-chroot.sh:201: sudo sed -i '/\( [^ -]\+\) main$/p On 2010/10/14 21:54:08, Markus (顧孟勤) wrote: > While it is not an error to use \(...\) in an address, it also doesn't actually > do anything useful. Oops, copy+paste cruft. Done. > Also, this pattern won't necessarily match. For example, on Maverick, my Is that true for a debootstrap install? The sed expression below assumes it ends with "main". http://codereview.chromium.org/3791007/diff/1/2#newcode202 build/install-chroot.sh:202: t1 Oops, I was originally doing a substitution, and didn't clean this up when I changed that. Done. http://codereview.chromium.org/3791007/diff/1/2#newcode209 build/install-chroot.sh:209: d' /var/lib/chroot/${target}/etc/apt/sources.list Sorry. What I was trying to do was: - Find a line that ends like "hardy main" (but not "hardy-backports" main), then - Duplicate that line and change it to "hardy-updates main", then - Duplicate that line and change it to "hardy-security main" So for any 'undecorated' distro, we end up with "distro", "distro-updates", and "distro-security" repositories.
Yes, this all looks good now. My only minor concern would be to check if we already have "-updates" and "-security" entries. In that case, we shouldn't add them. Oh, and lines that are commented out should be ignored. I am not sure if either concern matters, but it would make the script a little more robust if you checked for this.
On Thu, Oct 14, 2010 at 3:38 PM, <markus@chromium.org> wrote: > Yes, this all looks good now. > > My only minor concern would be to check if we already have "-updates" and > "-security" entries. In that case, we shouldn't add them. I wasn't too worried about it for this purpose because it seems that debootstrap never adds them (though I suppose that could change in a future version), and dupes don't hurt apt either way. > Oh, and lines that are commented out should be ignored. OK, I made it only match lines starting with "deb", like the sed below.
http://codereview.chromium.org/3791007/diff/11001/12001 File build/install-chroot.sh (right): http://codereview.chromium.org/3791007/diff/11001/12001#newcode211 build/install-chroot.sh:211: s/\(^deb .* [^ -]\+\) main/\1-security main/ Have you tested this? I vaguely recall you can't have the "^" marker inside of a group. Better pull it outside.
http://codereview.chromium.org/3791007/diff/11001/12001 File build/install-chroot.sh (right): http://codereview.chromium.org/3791007/diff/11001/12001#newcode211 build/install-chroot.sh:211: s/\(^deb .* [^ -]\+\) main/\1-security main/ On 2010/10/14 23:18:17, Markus (顧孟勤) wrote: > Have you tested this? I vaguely recall you can't have the "^" marker inside of a > group. Better pull it outside. Works for me, though I suppose that could be specific to GNU sed. I moved it outside.
LGTM |
