Hi,
anonym wrote (13 Oct 2014 15:54:06 GMT) :
> 11/10/14 12:40, intrigeri wrote:
>> 1. install_debian_extensions() could take the list of packaged
>> extensions as arguments (after the tbb_install one); sure,
>> that's cosmetic.
> Done in commit a41768b.
It's counter intuitive to me that the list of packages must be passed
as a single argument. This construction is rarely seen elsewhere when
passing lists of arguments. I think I would instead:
# arguments: the destination directory, followed by a list of
# extension packages
install_debian_extensions() {
local destination
destination="${1}"
shift
apt-get install --yes "$@"
[...]
What do you think?
>> 2. BUNDLES should be moved to a configuration file, which first would
>> more clearly separate code from configuration, and second would
>> simplify the release process (after verifying sha256sums.txt, just
>> copy it to the right place).
> I separated all data (url also, since it can be expected to change
> frequently) from the hook in commit d46be96.
Cool. I'm happy with the data separation, but I'm not happy with the
implementation. Notes follow:
> +install_debian_extensions "${DEBIAN_EXT}" "${TBB_EXT}"
This feels hard to understand while reading the code: $DEBIAN_EXT is
a list of Debian package names, and $TBB_EXT is a filesystem path.
I suggest renaming these variables to $DEBIAN_EXT_PKGS and
$TBB_EXT_DIR. IMO that's a blocker.
> +TBB_SHA256SUMS=/usr/share/tails/tbb-sha256sums.txt
> [...]
> +TBB_DIST_URL=/usr/share/tails/tbb-dist-url.txt
This feels very obscure too. The first time I read how one uses such
variables:
> +TBB_BUNDLES_BASE_URL="$(cat "${TBB_DIST_URL}")/${VERSION}"
... I thought "What?! cat'ing a URL?" => I think that a variable whose
value is the path to a file that itself contains some data should
instead be called e.g. TBB_DIST_URL_FILE, or whatever you think is
better, but certainly not TBB_DIST_URL (especially since it's not
consistent with how other very similar variables are called and used).
I think that's definitely a blocker.
Also, TBB_BUNDLES feels weird, as TBB means "Tor Browser Bundle".
TBB_TARBALLS, or TBB_ARCHIVES, instead? (I don't see that one as
a blocker, but I suppose that while you're at it and testing things
after fixing the rest, you can as well take care of this one -- if
not, no big deal :)
>> 3. Please apply tails-greeter_fix-browser-localization-path.patch to
>> the Greeter's master branch, so that [...]
> Done in commit ca74f94.
Great!
> (BTW: For conflict resolution with the stuff from
> feature/tor-browser-4.0, have a look at how it was done for experimental
> in commit 1e9e1442e4b3ad8e82492ebd2d856ec45c02a884)
I suspect I'll be able to merge feature/tor-browser-4.0 first. If this
happens, then I'll let you merge the resulting testing branch into the
refactoring one, and deal with the conflict resolution.
Cheers!
--
intrigeri
