[PATCH] (re-)dzip.sh: various improvements
- Don't ignore errors
- Quote $@ and $1
- Work with absolute filenames and filenames starting with a dash
- Pass many files per chmod invocation (much faster)
- Pass $UNZIPOPTS, like $ZIPOPTS
- Reuse dzip.sh in re-dzip.sh
The (re)generated zip files are identical.Note: I've tested this on several directories and zip files, including omni.ja, but did not attempt to run a full Tor Browser build on Qubes somehow.
Trac:
Username: rustybird
Activity
Trac:
Username: rustybird
I reviewed this and have a couple of comments on the re-dzip.sh changes.
It would be better to use a temporary directory name that is likely to be unique, e.g.,
TMPDIR=tmp-re-dzip-$$Also, shouldn't this line:
(cd tmp_dzip; dzip.sh ./"$ZIPFILE_BASENAME")include a dot like so:(cd tmp_dzip; dzip.sh ./"$ZIPFILE_BASENAME" .)-
Trac:
Username: rustybird -
Thanks for the review!
It would be better to use a temporary directory name that is likely to be unique, e.g.,
TMPDIR=tmp-re-dzip-$$Sounds good. Keeping it as
tmp-dzipwas probably too... timid. I've named the variableTEMPDIRin the v2 patch, on the off chance that https://en.wikipedia.org/wiki/TMPDIR might somehow affectunziporzip.The commands have also been tweaked to fail early (instead of late) when
$1has not been given.Also, shouldn't this line:
(cd tmp_dzip; dzip.sh ./"$ZIPFILE_BASENAME")include a dot like so:(cd tmp_dzip; dzip.sh ./"$ZIPFILE_BASENAME" .)Those are equivalent: The first calls
find …(because"$@"in the absence of arguments expands to nothing at all), while the second callsfind . …. But using a consistent style is better, so v2 has the explicit dot.Trac:
Username: rustybird 
Sorry for the long time before I got to your patch. Overall it looks good to me. It seems you are using a bunch of bashisms. This is fine with me in general but please don't declare this as a script for the Bourne shell then (which we/you do with
#!/bin/sh).Trac:
Keywords: TorBrowserTeam201612R deleted, N/A added
Status: needs_review to needs_revision-
What what what, bashisms? :) If you're referring to
$(command)or${parameter:?}, these are POSIX/bin/sh:http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_03 http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02
The 1997 version: http://pubs.opengroup.org/onlinepubs/007908799/xcu/chap2.html
Trac:
Username: rustybird -
Ah, indeed, thanks for the links. I stand corrected. I applied your patch to
masterandhardened-builds(commits a38d827c12ce75d13e18f19fc8f5bac8aca28c55 and c74da40d46b10c7a757f8835f8177602ed77f8e4).Trac:
Keywords: N/A deleted, TorBrowserTeam201612R added
Status: needs_revision to closed
Resolution: N/A to fixed -
This breaks our builds it seems. In particular the
re-dzip.shstep dealing withBrowser/omni.jain our gitian-firefox step fails in the middle of being executed:inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services-sync/stages/enginesync.js inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services-sync/stages/declined.js inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services-sync/status.js inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services-sync/userapi.js inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services/datareporting/policy.jsm inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services/healthreport/healthreporter.jsm inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services/healthreport/profile.jsm inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services/healthreport/providers.jsm inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services/metrics/dataprovider.jsm inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services/metrics/providermanager.jsm inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/services/metrics/storage.jsm inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/sqlite/sqlite_internal.js inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/workers/PromiseWorker.js inflating: tmp-re-dzip-27552/jsloader/resource/gre/modules/workers/require.jsis the last bit of log our nightly builds emit. Doing that step manually outside of a Gitian context works. Tested on a Debian testing machine (while we are using Wheezy for Tor Browser in Gitian).
Trac:
Resolution: fixed to N/A
Status: closed to reopened -
Damn, I'm very sorry for breaking the build! Apparently,
unzip(1)has some really weird exit statuses:0 normal; no errors or warnings detected. 1 one or more warning errors were encountered, but process- ing completed successfully anyway. [Okay...] This includes zip- files where one or more files was skipped [Uh?] due to unsup- ported compression method or encryption with an unknown password. 2 a generic error in the zipfile format was detected. Pro- cessing may [WTF] have completed successfully anyway; some bro- ken zipfiles created by other archivers have simple work- arounds. 3 a severe error in the zipfile format was detected. Pro- cessing probably [WTF!] failed immediately.Is there a full log somewhere? Maybe it would include such a "warning error".
Trac:
Username: rustybird -
Trac:
build_zip.log -
Yeah, doing it locally having
omni.jain the current working directory does not result in a similar error:gitian/build-helpers/re-dzip.sh omni.ja Archive: omni.ja creating: tmp-re-dzip-7111/chrome/ inflating: tmp-re-dzip-7111/chrome.manifest inflating: tmp-re-dzip-7111/chrome/chrome.manifest creating: tmp-re-dzip-7111/chrome/en-US/ creating: tmp-re-dzip-7111/chrome/en-US/locale/ creating: tmp-re-dzip-7111/chrome/en-US/locale/en-US/ creating: tmp-re-dzip-7111/chrome/en-US/locale/en-US/alerts/etc.
-
Trac:
Username: rustybird -
I just tested it on stretch with the
omni.jafrom stock Firefox ESR 45.5.1 and got the same warning+error on stderr, as well as exit status 2.(When you tested it on stretch, did you maybe run
re-dzip.shwith anomni.jathat already had been re-dzipped by an older version, so the problematic extra bytes were no longer present? That was how I had previously tested it...)Attached: v3 patch, which ignores
unzipexit status 1 and 2 (i.e. everything below "severe" errors)Trac:
Username: rustybird -
Replying to rustybird:
I just tested it on stretch with the
omni.jafrom stock Firefox ESR 45.5.1 and got the same warning+error on stderr, as well as exit status 2.(When you tested it on stretch, did you maybe run
re-dzip.shwith anomni.jathat already had been re-dzipped by an older version, so the problematic extra bytes were no longer present? That was how I had previously tested it...)Yeah, seems I made the same mistake.
Attached: v3 patch, which ignores
unzipexit status 1 and 2 (i.e. everything below "severe" errors)I tested it better this time and applied it to
masterandhardened-builds(commit ea31d520646c07f1565460565f5ead9ad7741b84 and 3efcbb345fb2cb701226d3c9c659457e7b6ef7bc), thanks!Trac:
Keywords: N/A deleted, TorBrowserTeam201612R added
Status: needs_revision to closed
Resolution: N/A to fixed -
Trac:
build_precise.log -
What a nightmare! It's making me ill that this has turned into such a time sink for you, so sorry. I've found some instructions for (kind of) getting Gitian to work on Qubes in a Bitcoin context, maybe I can adapt these to Tor Browser and really test my shit in the future: https://groups.google.com/d/msg/qubes-users/tnJnjlZgpGs/0bGDaHwiAgAJ
Anyway, this seems to have uncovered a legit (though benign) bug in
windows/gitian-firefox.yml: I've extractedtorbrowser-install-6.0.7_en-US.exeand, in contrast to Linux and Mac, there simply is noBrowser/webapprt/omni.jaon Windows. So that line could be deleted?(If you look through a successful Windows build log, it will probably show how the old
re-dzip.shignored theunzip,zip, andmvfailures and exited "successfully", because the last thing it did was remove the temporary directory.)Trac:
Username: rustybird -
Replying to rustybird:
What a nightmare! It's making me ill that this has turned into such a time sink for you, so sorry. I've found some instructions for (kind of) getting Gitian to work on Qubes in a Bitcoin context, maybe I can adapt these to Tor Browser and really test my shit in the future: https://groups.google.com/d/msg/qubes-users/tnJnjlZgpGs/0bGDaHwiAgAJ
No worries, thanks for your help!
Anyway, this seems to have uncovered a legit (though benign) bug in
windows/gitian-firefox.yml: I've extractedtorbrowser-install-6.0.7_en-US.exeand, in contrast to Linux and Mac, there simply is noBrowser/webapprt/omni.jaon Windows. So that line could be deleted?Indeed, thanks!
(If you look through a successful Windows build log, it will probably show how the old
re-dzip.shignored theunzip,zip, andmvfailures and exited "successfully", because the last thing it did was remove the temporary directory.)Yes. I've fixed that with removing the offending line in commit fcda7b592553df7fb44a59f4d9341b424f4ad372 and reapplied your patch in commit cb3acb5f6a878e9b34f60b6d9bb2bacb5194a3b8.
Trac:
Keywords: N/A deleted, TorBrowserTeam201612R added
Resolution: N/A to fixed
Status: needs_revision to closed
