Re: [Tails-dev] Migrating to (something closer to) the regul…

Delete this message

Reply to this message
Autore: intrigeri
Data:  
To: The Tails public development discussion list
Oggetto: Re: [Tails-dev] Migrating to (something closer to) the regular Tor Browser
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