Bug #9371

Use 'set -u' in gettextized shell scripts

Added by anonym 2015-05-11 14:12:57 . Updated 2016-03-08 19:03:25 .

Status:
Resolved
Priority:
Elevated
Assignee:
Category:
Internationalization
Target version:
Tails_2.2
Start date:
2015-05-11
Due date:
% Done:

100%

Feature Branch:
feature/9371-set-u-in-gettextized-shell-scripts
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

So far we’ve been avoiding it, see e.g. this comment from config/chroot_local-includes/usr/local/bin/tor-browser:

# Do not "set -u", else importing gettext.sh dies
# with "ZSH_VERSION: parameter not set".


However, we can actually set -u as long as we set:

ZSH_VERSION="${ZSH_VERSION:-}"


before sourcing gettext.sh in each such script (see git grep "\. gettext\.sh" for a list of candidates).

Subtasks

History

#1 Updated by intrigeri 2015-05-11 14:16:05

#2 Updated by anonym 2015-06-04 09:56:10

  • % Done changed from 0 to 20

anonym wrote:
> However, we can actually set -u as long as we set:
>

ZSH_VERSION="${ZSH_VERSION:-}"


> before sourcing gettext.sh in each such script (see git grep "\. gettext\.sh" for a list of candidates).

Actually, this was fixed in gettext 0.18.2, but wheezy is on 0.18.1. We should just install 0.19.3-2~bpo70+1 from wheezy-backports instead of bloating our code with the fix in each instance.

#3 Updated by anonym 2015-06-04 13:18:30

  • Status changed from Confirmed to In Progress

Applied in changeset commit:4f18a689e5b9164e63f6baf59e0f56e6469c986c.

#4 Updated by anonym 2015-06-04 13:20:14

  • Category set to Internationalization
  • Assignee deleted (anonym)
  • % Done changed from 20 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to feature/9371-set-u-in-gettextized-shell-scripts

#5 Updated by intrigeri 2015-06-04 13:47:18

  • Assignee set to intrigeri

#6 Updated by intrigeri 2015-06-05 18:28:01

  • Assignee changed from intrigeri to anonym
  • Target version changed from Tails_1.4.1 to Tails_1.5
  • QA Check changed from Ready for QA to Dev Needed
  • The set -e added in config/chroot_local-includes/usr/local/bin/tails-upgrade-frontend-wrapper prevents xhost permissions to be dropped if the upgrader fails. One should double-check each affected script for similar potential issues.
  • That set -e is added in a commit that’s about set -u, and anonym says “I missed splitting out that one into 704e626”.
  • A commit message speaks of “gettext-base < 1.8.2” (typo).
  • How much of the automated test suite did this branch pass?

It’s not urgent, and we both have enough on our plate for the next weeks => postponing to 1.5.

#7 Updated by anonym 2015-08-03 11:01:28

  • Target version changed from Tails_1.5 to Tails_1.6

#8 Updated by anonym 2015-09-02 17:01:46

  • Target version changed from Tails_1.6 to Tails_1.7

Actually, let’s postpone this to the next major release since it’s not really a bugfix. I have enough on my plate already, and I suspect I’ll have enough trouble getting my stuff reviewed already.

#9 Updated by anonym 2015-10-05 13:28:21

  • Target version changed from Tails_1.7 to 246

I simply have too much on my plate, and this whole thing isn’t that important. Postponing…

#10 Updated by sajolida 2015-11-27 04:46:09

  • Target version changed from 246 to Tails_2.0

#11 Updated by bertagaz 2015-12-15 05:48:56

Our /usr/local/bin/icedove wrapper needs that kind of trick to, at least as long as the claws-mail persistence hasn’t been removed from t-p-s.

#12 Updated by anonym 2016-01-11 14:16:04

  • Priority changed from Normal to Elevated
  • Target version changed from Tails_2.0 to Tails_2.2

Postponing, and raising prio because of it — let this be the last time. However, since we now have a newer gettext package (since we’re based on Jessie) I think we can solve it differenly. Yay.

#13 Updated by anonym 2016-02-11 20:55:04

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

This branch had bitrotted quite a bit, and I found it easier to just rewrite the history (dropping the commit installing gettext-base from wheezy-backports now that we’re based on Jessie, adding set -u and the ${VAR:-} trick to new scripts, etc.). I hope that’s not a problem, but actually helps the review.

intrigeri wrote:
> * The set -e added in config/chroot_local-includes/usr/local/bin/tails-upgrade-frontend-wrapper prevents xhost permissions to be dropped if the upgrader fails. One should double-check each affected script for similar potential issues.

Yay, I just love shell awkwardness. Fixed in commits commit:9d83efb and commit:e36554c.

> * That set -e is added in a commit that’s about set -u, and anonym says “I missed splitting out that one into 704e626”.

This was split out correctly now when I rewrote the history.

> * A commit message speaks of “gettext-base < 1.8.2” (typo).

I’m not sure what the typo is supposed to be here. The problem was fixed in 1.8.2, so version lower than that is problematic, like the message says.

> * How much of the automated test suite did this branch pass?

features/i2p.feature features/unsafe_browser.feature features/torified_browsing.feature features/mac_spoofing.feature features/persistence.feature

> It’s not urgent, and we both have enough on our plate for the next weeks => postponing to 1.5.

Hopefully no more! :)

#14 Updated by intrigeri 2016-02-18 15:02:40

  • Category deleted (Internationalization)

#15 Updated by intrigeri 2016-02-18 15:03:18

  • Category set to Internationalization

#16 Updated by intrigeri 2016-02-18 15:10:52

  • % Done changed from 50 to 60

Code review passes.

#17 Updated by intrigeri 2016-02-18 16:17:00

  • Status changed from In Progress to Fix committed
  • % Done changed from 60 to 100

Applied in changeset commit:8045700161de549352dfec2e8295cc42c83938bc.

#18 Updated by intrigeri 2016-02-18 16:20:39

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

#19 Updated by anonym 2016-03-08 19:03:25

  • Status changed from Fix committed to Resolved