Opened 2 years ago

Closed 2 years ago

#20147 closed enhancement (fixed)

[PATCH] (re-)dzip.sh: various improvements

Reported by: rustybird Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-gitian, TorBrowserTeam201612R
Cc: mcs, boklm, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

- 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.

Child Tickets

Attachments (5)

0001-re-dzip.sh-various-improvements.patch (2.2 KB) - added by rustybird 2 years ago.
v2-0001-re-dzip.sh-various-improvements.patch (2.3 KB) - added by rustybird 2 years ago.
build_zip.log (182.0 KB) - added by gk 2 years ago.
v3-0001-Bug-20147-re-dzip.sh-various-improvements.patch (2.4 KB) - added by rustybird 2 years ago.
build_precise.log (248.0 KB) - added by gk 2 years ago.

Download all attachments as: .zip

Change History (26)

Changed 2 years ago by rustybird

comment:1 Changed 2 years ago by boklm

Keywords: tbb-gitian TorBrowserTeam201609R added
Status: newneeds_review

comment:2 Changed 2 years ago by mcs

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" .)

Changed 2 years ago by rustybird

comment:3 Changed 2 years ago by 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-dzip was probably too... timid. I've named the variable TEMPDIR in the v2 patch, on the off chance that https://en.wikipedia.org/wiki/TMPDIR might somehow affect unzip or zip.

The commands have also been tweaked to fail early (instead of late) when $1 has 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 calls find . …. But using a consistent style is better, so v2 has the explicit dot.

comment:4 Changed 2 years ago by mcs

Cc: mcs boklm gk added

Thanks for the updated patch. It looks good to me.
boklm or gk should also review these changes.

comment:5 Changed 2 years ago by gk

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201609R removed

Moving review tickets to October.

comment:6 Changed 2 years ago by gk

Keywords: TorBrowserTeam201611R added; TorBrowserTeam201610R removed

Moving review tickets to November.

comment:7 Changed 2 years ago by gk

Keywords: TorBrowserTeam201612R added; TorBrowserTeam201611R removed

Moving our review tickets to December.

comment:8 Changed 2 years ago by gk

Keywords: TorBrowserTeam201612R removed
Status: needs_reviewneeds_revision

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).

Last edited 2 years ago by gk (previous) (diff)

comment:10 Changed 2 years ago by gk

Keywords: TorBrowserTeam201612R added
Resolution: fixed
Status: needs_revisionclosed

Ah, indeed, thanks for the links. I stand corrected. I applied your patch to master and hardened-builds (commits a38d827c12ce75d13e18f19fc8f5bac8aca28c55 and c74da40d46b10c7a757f8835f8177602ed77f8e4).

comment:11 Changed 2 years ago by gk

Resolution: fixed
Status: closedreopened

This breaks our builds it seems. In particular the re-dzip.sh step dealing with Browser/omni.ja in 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.js

is 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).

Last edited 2 years ago by gk (previous) (diff)

comment:12 Changed 2 years ago by gk

Keywords: TorBrowserTeam201612R removed
Status: reopenedneeds_revision

I've backed the patch out of master and hardened-builds (commit 55ba062a0ecda854d55f47f1e660746ad099581d and a6c4a3d715b863a2e4ff2b6d2129d090a7a6e32a).

comment:13 Changed 2 years ago by rustybird

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".

comment:14 Changed 2 years ago by gk

Attached is all what we get when rezipping omni.ja. I am still a bit puzzled, though, why I don't see a similar behavior outside of Gitian/Debian Wheezy. Maybe unzip is sufficiently newer on Debian Stretch? I have not looked further into it.

Changed 2 years ago by gk

Attachment: build_zip.log added

comment:15 Changed 2 years ago by gk

Yeah, doing it locally having omni.ja in 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.

comment:16 Changed 2 years ago by rustybird

I just tested it on stretch with the omni.ja from 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.sh with an omni.ja that 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 unzip exit status 1 and 2 (i.e. everything below "severe" errors)

comment:17 in reply to:  16 Changed 2 years ago by gk

Keywords: TorBrowserTeam201612R added
Resolution: fixed
Status: needs_revisionclosed

Replying to rustybird:

I just tested it on stretch with the omni.ja from 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.sh with an omni.ja that 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 unzip exit status 1 and 2 (i.e. everything below "severe" errors)

I tested it better this time and applied it to master and hardened-builds (commit ea31d520646c07f1565460565f5ead9ad7741b84 and 3efcbb345fb2cb701226d3c9c659457e7b6ef7bc), thanks!

comment:18 Changed 2 years ago by gk

Resolution: fixed
Status: closedreopened

Seems I tested not good enough. :( For some reason this breaks on Ubuntu Precise which we need to build our Windows bundles on. Attached is the relevant part of the build log. I can see no obvious issue, though.

I backed the patch out again.

Changed 2 years ago by gk

Attachment: build_precise.log added

comment:19 Changed 2 years ago by gk

Keywords: TorBrowserTeam201612R removed
Status: reopenedneeds_revision

comment:20 Changed 2 years ago by 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

Anyway, this seems to have uncovered a legit (though benign) bug in windows/gitian-firefox.yml: I've extracted torbrowser-install-6.0.7_en-US.exe and, in contrast to Linux and Mac, there simply is no Browser/webapprt/omni.ja on 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.sh ignored the unzip, zip, and mv failures and exited "successfully", because the last thing it did was remove the temporary directory.)

comment:21 in reply to:  20 Changed 2 years ago by gk

Keywords: TorBrowserTeam201612R added
Resolution: fixed
Status: needs_revisionclosed

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 extracted torbrowser-install-6.0.7_en-US.exe and, in contrast to Linux and Mac, there simply is no Browser/webapprt/omni.ja on 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.sh ignored the unzip, zip, and mv failures 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.

Note: See TracTickets for help on using tickets.