Hi,
anonym wrote (30 Nov 2014 20:42:02 GMT) :
> On 31/10/14 13:55, intrigeri wrote:
>> A general note: there are quite a few "2>/dev/null". I'm not sure what
>> it buys us, and I suspect it may make debugging more difficult than it
>> could be in the future.
> Sure. I kept it in the `try_for()` blocks since it may flood stdout/err
> with errors.
Ah, OK. We'll see if it ends up being painful.
>> The design doc should be updated, at least to point to the new shell
>> library file.
> Indeed.
I see some good progress in the branch.
How about pointing to i2p.sh in contribute/design/I2P_Browser?
>> I'm surprised that we can now use 7z to unpack omni.ja (see fe54c8ca
>> for why we had to use unzip in the first place), but it's good news!
> Perhaps that was an issue affecting Squeeze's 7z, but not Wheezy's?
Yeah, perhaps. Or perhaps Mozilla have stopped using custom extensions
to the zip format in .ja, or TB's folks are patching it already and
generating a standard zip file for us. Anyway :)
>> Commit c5eab441 would benefit from more explanations regarding why
>> patching torbutton@??? isn't needed anymore.
> I do not understand. The set_chroot_torbutton_browser_name() function
> does the patching, and it's called for the I2P browser so nothing is
> lost. If you wonder why it's not called for the Unsafe browser, that's
> because it doesn't have Torbutton installed.
> To be clear: if Torbutton is installed, it overrides the browser
> banding's name. That's why we rename the browser via Torbutton in the
> I2P browser, since it has Torbutton installed, and why we rename it with
> via the browser's branding in the Unsafe browser.
Thanks, now it's way clearer to me than what the commit message says :)
No need to rewrite history for that.
>> Regarding commit 9800961, it's unclear why this can be done for the
>> I2P browser, while the "the name is dictated completely by Torbutton"
>> assumption would not be verified for the Unsafe browser too.
> Please clarify what you mean here if you still think something is unclear.
My comment was relying on wrong assumptions, that you've clarified
above, so forget it :)
>> Style nitpicking: I find it weird to on the one hand quote constant
>> strings with double-quotes consistently (e.g.
>> BROWSER_NAME="i2p-browser"), while on the other hand using constructs
>> like /my/string/"$variable" in many places. I would either *not* quote
>> constant strings ever, or write the latter "/my/string/$variable"
>> instead. Preferably the latter. What do you think?
> Well, sure.
Good. I've gone a bit further with c9599e6.
>> Regarding commit a1ad08b95 ("Optimistically try to tear down chroot if
>> killing failed"): why? :)
> Why not? With some luck, whatever process we failed to kill may die just
> a bit later, and even it doesn't we may at least partially tear down the
> chroot, which seems beneficial. If you think it doesn't make sense, I
> can revert it.
OK.
>> I'm not sure whether setup_browser_chroot() is the best place to set
>> up signal handlers, but not sure where it should live either.
> Me neither, so I'm not sure I can improve on it without further hints or
> suggestions.
I'm lacking inspiration here. Let's leave it at is.
>> I don't like commit 6a0e1448 ("Chain commands and report relevant
>> errors upon chain failure") very much. I suggest doing "|| error ..."
>> after each step, instead of building an ugly chain of commands with &&
> I don't get what is ugly about it, and I actually like those constructs
> a lot, so I wouldn't mind being enlightened why they're bad.
> Any way, doing `|| error ...` after each step will be very verbose, and
> will flood the translators with new strings without adding much benefit.
> We could sidestep the latter by doing something like the following for
> each of the calls in each chain:
> configure_chroot_dns_servers "${CHROOT}" "${IP4_NAMESERVERS}" || \
> FUN=configure_chroot_dns_servers() && \
> error "`gettext \"Failed to configure browser (in \${FUN}).\"`"
> Is this really any better? Do you have something nicer in mind? IMHO the
> current situation with the chains is a pretty sweet tradeoff between
> error reporting accuracy and script verboseness/complexity.
OK.
>> (which is a bit confusing given we're running under "set -e").
> I don't get why it's more confusing together with `set -e`, in fact `set
> -e` is irrelevant in the &&-chain + ||-error-catching construct. What am
> I missing?
I meant that using "&&" constructs is rarely needed under "set -e",
since the N+1'th command won't be run if the N'th one fails anyway.
So, having such a chain of commands with "&&" in between may let
someone working on this code later miss the fact we're under "set -e",
and then introduce bugs. No big deal anyway, forget it.
> In any case, after commit 5d16b59 all this is moot, except one instance
> of chaining (for `copy_extra_tbb_prefs()`).
OK.
>> I'm not sure if it's really useful that language_code_from_locale
>> takes an argument ($LANG), instead of taking it the information it
>> needs from the environment.
> I could imagine there being situations where you may want to do it for
> another LANG than what you have set, perhaps at build time when we are
> juggling with all supported languages.
OK.
>> Especially since it's called "from_locale".
> So you mean that for you "locale" always refers to the specific, set
> locale (in the environment), not just *a* locale?
Not really. I'm convinced by your above argument anyway.
>> Regarding commit 8051820d, this is an extra safety measure, useful
>> only if the checking added in previous commit fails to error out,
>> right? If so, I think adding a comment would be helpful.
>>
>> I see an undocumented set_key() function is added to common.sh.
>> I think its name is much too generic for common.sh.
> Hmm. True. What about `set_simple_config_key()`?
Fine with me.
>>> + # We will use the detected language even if I2P supports it; it
>>> + # will default to English in that case.
>>
>> Rather "even if I2P doesn't support it", right?
> Right, thanks!
I now see "if I2P doesn't supports it" in the branch, which isn't
correct grammatically. Fixed in 7551b02.
>> Not sure why the tails-i2p initscript lives in /usr/local/sbin, but
>> oh well.
> There wasn't much thought behind that, I think. The old I2P start script
> was also located there, probably because it wasn't started during init,
> but via a GNOME menu entry. That was still the case when we "migrated"
> to the tails-i2p script, and only a release after that did we make it
> start via a NetworkManager hook. I don't really see it as an init script
> any way, more like wrapper since we want to wrap extra functionality
> around it.
OK.
I've deleted other quotes that I agree have been addressed in
the branch. Congrats!
So, only 2 small things left to improve :)
Cheers!
--
intrigeri
