+++ This bug was initially created as a clone of Bug #1237706 +++ As started on bug 1237706 we want to allow mozharness to use the internal ZipFile and TarFile classes, also in case the external unzip command is not available. Lots of work has already been done for it on the other bug. This applies to attachment 8708255 [details] [diff] [review] as well as attachment 8709100 [details] [diff] [review], whereby the latter already got r+. What's missing is to address the latest review comments (bug 1237706 comment 22 ff) and adding some unit tests. I will try to find some time soon to finish up my work but maybe it could also happen in May after my return while being away all April.

I would just use the Python built-in APIs instead of invoking a new process because those APIs have a greater chance of working (e.g. no cross-platform compatibility issues). I don't think performance will be an issue, as the performance critical code will be decompression and that is implemented in C.

Code which is not ready yet. I just rebased to current HEAD and combined both remaining patches. Review commit: https://reviewboard.mozilla.org/r/65256/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65256/

Can we expect performance gains from this? or mainly better code?

Review commit: https://reviewboard.mozilla.org/r/65334/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65334/

Comment on attachment 8772447 [details] Bug 1258539 - [mozharness] Use ZipFile and TarFile classes for unpacking archives. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65256/diff/1-2/

https://reviewboard.mozilla.org/r/65254/#review62356 ::: testing/mozharness/mozharness/base/script.py:1424 (Diff revision 2) > + raise IOError('Could not find file to extract: %s' % filename) > + > + level = FATAL if halt_on_failure else error_level > + > + # Is it a valid tar file? Also includes gz and bz2 compression types > + if tarfile.is_tarfile(filename): Beware of tarfile.is_tarfile() https://bugzilla.mozilla.org/show_bug.cgi?id=1211882 In the tests we might want to also test against a .dmg to see what happens.

Comment on attachment 8772585 [details] Bug 1258539 - [mozharness] Refactor name and arguments of download and unpack methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65334/diff/1-2/

(In reply to Armen Zambrano [:armenzg] - Engineering productivity from comment #3) > Can we expect performance gains from this? or mainly better code? I don't know yet if we can see performance gains. It is something we will have to see once I get all the tests passing. But asking Joel it also seems like we do not track those perf values in Talos yet. What we definitely get are lesser side-effect like the issues with older unzip versions eg. on OS X. If we can get rid of unzip/tar everywhere we would even not have to install those tools on the build/test machines, but that should be far away I think. (In reply to Armen Zambrano [:armenzg] - Engineering productivity from comment #7) > > + if tarfile.is_tarfile(filename): > > Beware of tarfile.is_tarfile() > https://bugzilla.mozilla.org/show_bug.cgi?id=1211882 > > In the tests we might want to also test against a .dmg to see what happens. Thanks I will check that. So far we do not support .dmg either in mozharness proper. And I don't think I have to add support for it. We can fail in case someone tries to unpack a dmg file.

Last try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01d29341732f&selectedJob=24202990 As you can see we have issues with unpack on Windows only. This is all about "permission denied" and applies to Jit, and various Talos jobs. Looks like the extraction misses to set permissions. I thought that my latest code had this fixed, but maybe I missed a part which I have had in our firefox-ui-tests module.

Comment on attachment 8772447 [details] Bug 1258539 - [mozharness] Use ZipFile and TarFile classes for unpacking archives. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65256/diff/2-3/

Comment on attachment 8772585 [details] Bug 1258539 - [mozharness] Refactor name and arguments of download and unpack methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65334/diff/2-3/

So I really missed an extra line which only updates the permissions on platforms which actually support those. It would fail on Windows because we set `0`. The addition fixed it. I also updated the check for dmg files locally and I hope it will be fine. It will be part of the next commit. Sadly we still have test failures for talos jobs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e34e0aad0706 Those jobs are downloading http://people.mozilla.org/~jmaher/taloszips/zips/tp5n.zip and the hierarchy seems to cause problems. I will check which options we used for unzip, so that i can fix it for ZipFile.

The problem with Talos is actually Windows only and only happens because of the following file contains a question mark: "tp5n\\uol.com.br\\www.uol.com.br\\b\\ss\\uoluolprod,uoltotal\\1\\H.22.1\\?AQB=1&ndh=1&AQE=1". I will have to check what unzip actually does. If it removes not allowed characters or even skip the file. The latter would be strange. Joel, maybe you could also update that filename in case the archive is not getting auto-generated?

The talos related tp5n.zip indeed contains invalid filenames. I filed bug 1288135 to get this fixed. There is another question which comes up now. How should we handle those cases? Should we mimic the behavior of unzip and renaming the file? So that an invalid characters becomes `_`? Or should we skip it and log a warning? In case we don't want to abort abruptly after the first invalid file, we need a try/except around individual archive entries. Greg, what would you suggest?

I think we should abort if the archive contains invalid filenames. I believe automation should fail fast. Silently failing is just a recipe for disaster. I could be convinced to do unzip-compatible behavior - but only if fixing tp5n.zip is too difficult. Hopefully we can establish sanity then guarantee it going forward by failing fast.

(In reply to Armen Zambrano [:armenzg] - Engineering productivity from comment #3) > Can we expect performance gains from this? or mainly better code? I haven't looked at the patches, but the final state should see performance gains. Currently we're writing files to disk then running a process to uncompress them. If we keep everything in memory, we'll avoid I/O and that should result in performance gains.

(In reply to Gregory Szorc [:gps] from comment #16) > I could be convinced to do unzip-compatible behavior - but only if fixing > tp5n.zip is too difficult. Hopefully we can establish sanity then guarantee > it going forward by failing fast. Fixing that archive should be fine given that only two files are affected. Sadly Joel will not be around the next day at least so it might still take a while until we have this properly fixed. I will take that extra time to write some tests then.

Comment on attachment 8772447 [details] Bug 1258539 - [mozharness] Use ZipFile and TarFile classes for unpacking archives. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65256/diff/3-4/

Comment on attachment 8772585 [details] Bug 1258539 - [mozharness] Refactor name and arguments of download and unpack methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65334/diff/3-4/

The patch is now ready to be reviewed. But please keep in mind that it will still fail due to the invalid filenames for Talos tests. It means we have to fix its tp5n.zip archive first before we can push this patch. Once that is done and the patch fully reviewed, I will trigger another full try run to ensure nothing is broken.

https://reviewboard.mozilla.org/r/65256/#review63036 this looks awesome! really cleaned up. couple questions in line. ::: testing/mozharness/mozharness/base/script.py:477 (Diff revision 4) > """ > dirs = self.query_abs_dirs() > zipfile = self.download_file(url, parent_dir=dirs['abs_work_dir'], > error_level=FATAL) > > - command = self.query_exe('unzip', return_type='list') > + self.unpack(zipfile, parent_dir, target_unzip_dirs) we should change the `zipfile` identifier because it re-assigns the imported module by the same name within this scope. ::: testing/mozharness/mozharness/base/script.py:1414 (Diff revision 4) > + > + """ > + def _filter_entries(namelist): > + """Filter entries of the archive based on the specified list of to extract dirs.""" > + filter_partial = functools.partial(fnmatch.filter, namelist) > + for entry in itertools.chain(*map(filter_partial, extract_dirs or ['*'])): if we unpack and thus force the exhaustion of the generator outputted by `map` as arguments to `chain`, don't we lose the effeciency gained by using func and itertools? would somethign like this be more performant? ```python for entries in map(filter_partial, extract_dirs or ['*'])): for entry in entries: yield entry ``` ::: testing/mozharness/mozharness/base/script.py:1420 (Diff revision 4) > + yield entry > + > + if not os.path.isfile(filename): > + raise IOError('Could not find file to extract: %s' % filename) > + > + level = FATAL if halt_on_failure else error_level since you only want to self.fatal() log if halt_on_failure=True, I don't think we need both params: `halt_on_failure` and `error_level` instead you could just: ```python # if we want halt_on_failure=False behaviour unpack(filepath, dest, error_level=ERROR) # if we want halt_on_failure=True behaviour unpack(filepath, dest, error_level=FATAL) ``` ::: testing/mozharness/test/helper_files/create_archives.sh:3 (Diff revision 4) > +#!/bin/bash > + > +cd archives where is create_archives.sh used?

https://reviewboard.mozilla.org/r/65256/#review63036 Thanks. One thing I just noticed is that I actually missed a test for permissions of an extracted file. I will add it with my next update. > we should change the `zipfile` identifier because it re-assigns the imported module by the same name within this scope. Good catch! Totally missed that. > if we unpack and thus force the exhaustion of the generator outputted by `map` as arguments to `chain`, don't we lose the effeciency gained by using func and itertools? > > would somethign like this be more performant? > > ```python > for entries in map(filter_partial, extract_dirs or ['*'])): > for entry in entries: > yield entry > ``` This is roughly the same. Just have a look at the example here: https://docs.python.org/2/library/itertools.html#itertools.chain. I also did a quick check with cProfile but there is no noticable difference. It's below 50ms compared to the overall 2.5s extracting the mochitest.zip file. Tell me what you like best. > since you only want to self.fatal() log if halt_on_failure=True, I don't think we need both params: `halt_on_failure` and `error_level` > > instead you could just: > > ```python > # if we want halt_on_failure=False behaviour > unpack(filepath, dest, error_level=ERROR) > > # if we want halt_on_failure=True behaviour > unpack(filepath, dest, error_level=FATAL) > ``` I think that makes sense. The remaining question would be what should be the default value for `unpack()`. I suggest ERROR similar to `download_file()` as a low-level method. `download_unpack()` as a helper method should get FATAL because that was the default due to halt_on_failure=True. > where is create_archives.sh used? You would have to manually run it to create the test archives in case you are making changes. If you think we don't need it or move it to a readme with instructions, let me know.

Comment on attachment 8772447 [details] Bug 1258539 - [mozharness] Use ZipFile and TarFile classes for unpacking archives. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65256/diff/4-5/

Comment on attachment 8772585 [details] Bug 1258539 - [mozharness] Refactor name and arguments of download and unpack methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65334/diff/4-5/

https://reviewboard.mozilla.org/r/65256/#review63036 looks good. will r+ once other issues are fixed > This is roughly the same. Just have a look at the example here: https://docs.python.org/2/library/itertools.html#itertools.chain. > > I also did a quick check with cProfile but there is no noticable difference. It's below 50ms compared to the overall 2.5s extracting the mochitest.zip file. > > Tell me what you like best. ah okay. no difference wfm > I think that makes sense. The remaining question would be what should be the default value for `unpack()`. I suggest ERROR similar to `download_file()` as a low-level method. `download_unpack()` as a helper method should get FATAL because that was the default due to halt_on_failure=True. what you suggest make sense to me! > You would have to manually run it to create the test archives in case you are making changes. If you think we don't need it or move it to a readme with instructions, let me know. ah gotcha. maybe put a comment in the sh file itself?

https://reviewboard.mozilla.org/r/65256/#review63036 hm, wait, looks like you fixed it in the other 65334 mozreview changeset: https://reviewboard.mozilla.org/r/65334/diff/4-5/

Comment on attachment 8772447 [details] Bug 1258539 - [mozharness] Use ZipFile and TarFile classes for unpacking archives. https://reviewboard.mozilla.org/r/65256/#review63318

Comment on attachment 8772585 [details] Bug 1258539 - [mozharness] Refactor name and arguments of download and unpack methods. https://reviewboard.mozilla.org/r/65334/#review63320 from eyeballing it, this looks good. guess you'll land once try is green and https://bugzilla.mozilla.org/show_bug.cgi?id=1288135 is resolved?

Comment on attachment 8772447 [details] Bug 1258539 - [mozharness] Use ZipFile and TarFile classes for unpacking archives. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65256/diff/5-6/

Comment on attachment 8772585 [details] Bug 1258539 - [mozharness] Refactor name and arguments of download and unpack methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65334/diff/5-6/

(In reply to Jordan Lund (:jlund) from comment #29) > from eyeballing it, this looks good. guess you'll land once try is green and > https://bugzilla.mozilla.org/show_bug.cgi?id=1288135 is resolved? Great. Thanks a lot. And yes, we will have to wait until the tp5n.zip is fixed and available via tooltool. Joel will try to get to it later this week.

Now that the blocking bug has been fixed I retriggered the affected talos jobs in teh try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e34e0aad0706&selectedJob=24216171 I will check later or on Monday, and if all is green we can get this patch landed.

Jordan, I will be away until next Wednesday. Maybe you can check the before mentioned try build later today and land the patch in case of all is passing now for talos jobs? Thanks.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cc2996a53b71 [mozharness] Use ZipFile and TarFile classes for unpacking archives. r=jlund https://hg.mozilla.org/integration/autoland/rev/8322ffecd9d9 [mozharness] Refactor name and arguments of download and unpack methods. r=jlund

(In reply to Henrik Skupin (:whimboo) from comment #33) > Now that the blocking bug has been fixed I retriggered the affected talos > jobs in teh try build: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=e34e0aad0706&selectedJob=24216171 > > I will check later or on Monday, and if all is green we can get this patch > landed. hm, looks like you've landed but try push looks sad still on windows :\ ...

No, I only re-triggered the failing Talos jobs and those are successful now.

I had to back this out for adding what appears to be a permafailing job: https://treeherder.mozilla.org/logviewer.html#?job_id=1172468&repo=autoland https://hg.mozilla.org/integration/autoland/rev/ff43d3e4c4fc

It seems that it failed on the Mozharness job. I don't know why it did not fail on the try push.

I will check that now after I had a rough day fixing other breakage.

So I was using an one click loaner to get this investigated because I really was not able to get it reproduced locally. Lets see what's wrong. First interesting thing is the failure output: > Traceback (most recent call last): > test/test_base_script.py line 278 in test_unpack > self.assertEqual(file_stats.st_mode, orig_fstats.st_mode) > AssertionError: 33277 != 33261 > $ python -c "import os; print os.stat('test/helper_files/archives/reference/bin/script.sh').st_mod e" > 33261 Unpacking the zip file und running the same command: > $ python -c "import os; print os.stat('bin/script.sh').st_mode" > 33277 So it looks like that included archive.zip package has this file with the wrong permissions included. Not sure why but maybe I missed to add it for the latest changes. I will update the mozreview patch by using the following permissions `-rwxr-xr--` now, so that we have a variety between different groups.

Comment on attachment 8772447 [details] Bug 1258539 - [mozharness] Use ZipFile and TarFile classes for unpacking archives. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65256/diff/6-7/

Comment on attachment 8772585 [details] Bug 1258539 - [mozharness] Refactor name and arguments of download and unpack methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65334/diff/6-7/

Hm, whatever I do it looks like that script.sh always gets the following permission: > new file mode 100755 > --- /dev/null > +++ b/testing/mozharness/test/helper_files/archives/reference/bin/script.sh Locally I clearly have: > -rwxr-xr-- which corresponds to 754. Mike, could this be an issue with cinnabar? As I can see git doesn't even notice when I change the permissions of a file and run "git status".

Comment on attachment 8772447 [details] Bug 1258539 - [mozharness] Use ZipFile and TarFile classes for unpacking archives. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65256/diff/7-8/

Comment on attachment 8772585 [details] Bug 1258539 - [mozharness] Refactor name and arguments of download and unpack methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65334/diff/7-8/

For now I will go with `755` which should also be fine. I triggered a new try build for Linux64 only covering all test suites.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/384ff6b4b109 [mozharness] Use ZipFile and TarFile classes for unpacking archives. r=jlund https://hg.mozilla.org/integration/autoland/rev/4939f5d5c16c [mozharness] Refactor name and arguments of download and unpack methods. r=jlund

Neither git nor mercurial can store a permission other than 644 or 755.

https://hg.mozilla.org/mozilla-central/rev/384ff6b4b109 https://hg.mozilla.org/mozilla-central/rev/4939f5d5c16c

I had to back this out because it apparently made Windows XP PGO talos(g2) runs permafail (and the e10s variant very-nearly-permafail) like https://treeherder.mozilla.org/logviewer.html#?job_id=1445097&repo=autoland https://hg.mozilla.org/mozilla-central/rev/6b65dd49d4f0

this also created a performance regression in talos for the tsvgx test suite, here are the subtests which failed: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=autoland&originalRevision=d6ee6a38aa43bae545a100d60296508878fde95c&newProject=autoland&newRevision=4939f5d5c16c957ae0da6d509a26482983fc894a&originalSignature=79acd47ffc9e36cd6aea558b46ed6667986e21f2&newSignature=79acd47ffc9e36cd6aea558b46ed6667986e21f2&framework=1 this is both on win7 and win8 (not sure if I have enough data for winxp). For completeness here are all the alerts we have associated with this: https://treeherder.mozilla.org/perf.html#/alerts?id=2261

Here the Treeherder results for autoland for all changesets where my patch was on autoland specifically for Talos jobs: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=4939f5d5c16c&tochange=6b65dd49d4f0&filter-searchStr=talos I actually do not see how this patch could have been caused a timeout exception in the Talos tests as referenced by Wes. Here the relevant log output from a relevant job: > 14:59:48 INFO - PROCESS | 3928 | Loaded: http://localhost:1504/tests/tp5n/dailymail.co.uk/www.dailymail.co.uk/ushome/index.html > 15:00:18 INFO - PROCESS | 3928 | 1470348018251 addons.productaddons ERROR Request failed certificate checks: [Exception... "SSL is required and URI scheme is not https." nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: resource://gre/modules/CertUtils.jsm :: checkCert :: line 145" data: no] > 15:56:16 INFO - TEST-UNEXPECTED-ERROR | tps | timeout As you can see we stalled 56 minutes for doing some checks for "dailymail.co.uk". Maybe that is a regression as came in by updating tp5n.zip via bug 1288135? Please also note that the Try build was all successful. Also while the patch was still active in autoland and does not produce any troubles regarding those bustage as listed above. Anyway I triggered a new set of jobs on Try for this particular job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3d431df5715 Wes, can you please clarify your assumptions why this patch should have caused such bustage? Also since the backout I can still see Talos g2 jobs failing but less frequently. Maybe I had a really bad timing in landing which was paved over with some networking issues? Regarding the talos performance regressions I would have to check details. Joel, what is taken into account for the timing? The whole job run time, or the execution times of the tests?

given the data about timeouts and perf differences- we must be doing something different with the way we are serving the files. Given the nature of this patch, it seems to me that possibly we are putting the files in a different location or unpacking them differently such that the python webserver is not serving them the same they were prior to this patch? maybe permissions, directory location, etc.

(In reply to Joel Maher ( :jmaher ) from comment #54) > of this patch, it seems to me that possibly we are putting the files in a > different location or unpacking them differently such that the python > webserver is not serving them the same they were prior to this patch? maybe > permissions, directory location, etc. The target folder has not been changed and is identical to when we used the unzip command. Also if that would be the case no single test could be executed. But that is not the case here. The only issue we had on Windows was for permissions but those we should set correctly now, given that the included unit test is also passing. Given all those problems I will go ahead and redo the changes for Talos by forcing it to use the external unzip command.

Reverting the Talos unzipping to the external unzip made those tests working again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b8b045665c3 I actually didn't try to push without this change again, maybe my last push was based on a bad commit. But I think we should leave Talos out for now, and fix it when getting rid of unzip in all the other cases. I will file a follow-up bug right now.

Comment on attachment 8772447 [details] Bug 1258539 - [mozharness] Use ZipFile and TarFile classes for unpacking archives. Jordan, I would like to have another review from you regarding the Talos changes. It should only be necessary to check the last interdiff: https://reviewboard.mozilla.org/r/65256/diff/8-9/ Thanks.

Comment on attachment 8772447 [details] Bug 1258539 - [mozharness] Use ZipFile and TarFile classes for unpacking archives. https://reviewboard.mozilla.org/r/65256/#review68170 ::: testing/mozharness/mozharness/mozilla/testing/talos.py:292 (Diff revision 9) > if self.query_pagesets_url(): > - self.info("Downloading pageset...") > + self.info('Downloading pageset...') > + dirs = self.query_abs_dirs() > src_talos_pageset = os.path.join(src_talos_webdir, 'tests') > - self.download_unzip(self.pagesets_url, src_talos_pageset) > + archive = self.download_file(self.pagesets_url, parent_dir=dirs['abs_work_dir']) > + unzip = self.query_exe('unzip') if it wfy on try it wfm! :) I'm fine landing this as is but do you forsee us requiring unzip subproc elsewhere? iow - do you think we should have a generic method for this deeper down along side python's zipfile as alternative/backup?

(In reply to Jordan Lund (:jlund) from comment #61) > if it wfy on try it wfm! :) \o/ > I'm fine landing this as is but do you forsee us requiring unzip subproc > elsewhere? iow - do you think we should have a generic method for this > deeper down along side python's zipfile as alternative/backup? Every code which uses our download_unpack() method now will work. I don't see that we would have to change other code and revert to the external unzip command. For those cases where the latter is used the code is really customized to their needs. So it would need some love (especially Talos) to get all that converted. I filed bug 1294357 to get this work done. Also as gps said, we should avoid to use the external commands, so implementing a generic method would not help us moving forward IMHO.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/07273f73c50c [mozharness] Use ZipFile and TarFile classes for unpacking archives. r=jlund https://hg.mozilla.org/integration/autoland/rev/b9c9444d6a05 [mozharness] Refactor name and arguments of download and unpack methods. r=jlund

https://hg.mozilla.org/mozilla-central/rev/07273f73c50c https://hg.mozilla.org/mozilla-central/rev/b9c9444d6a05

I am seeing a tsvgx performance regression from this recent landing: https://treeherder.mozilla.org/perf.html#/alerts?id=2394 Odd that we get the same regressions even with no real changes directly to the unpacking of talos data.

The talos regression is being tracked on bug 1295226 now.

Do we know if we use the tar extraction anywhere? http://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/mozharness/base/script.py#l1445 From a Linux log [1] I can see that we use mozinstall instead: > 08:08:15 INFO - Getting output from command: ['/builds/slave/test/build/venv/bin/mozinstall', '/builds/slave/test/build/installer.tar.bz2', '--destination', '/builds/slave/test/build/application'] [1] https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux/1471961154/mozilla-central_ubuntu32_vm_test-xpcshell-bm142-tests1-linux32-build3.txt.gz [1]

I don't think that we use TarFile anywhere at least not for the basic binaries and tests. Some test suites may still have tar files present somewhere for download. But that might need deeper investigation.