Create .tar.gz package deterministically using Python's "tarfile" #12244

tonyo · 2016-07-04T21:43:22Z

A development of #12108, creates a .tar.gz package using the tarfile and gzip modules, without external dependencies. Fixes #11981.
Also this fixes the issue when the existing resources/ directory didn't allow to create a new package and failed with "File exists" error.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Create Linux .tar.gz package archive deterministically #11981 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because "more general approach to reproducibility testing is needed"

This change is

highfive · 2016-07-04T21:43:27Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

highfive · 2016-07-04T21:43:28Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/servo/command_base.py, python/servo/package_commands.py

jdm · 2016-07-06T14:21:03Z

r? @aneeshusa

aneeshusa · 2016-07-18T03:39:53Z

python/servo/command_base.py

+        tarinfo.mtime = 0
+        return tarinfo
+
+    with cd(dir_to_package):


How hard would it be to get rid of this with cd(...) block? I've never been a fan of cding during builds, but it's OK to keep if it's complicated otherwise.

One advantage of with cd() is that we don't need to deal with relative and non-relative path prefixes which we need to remove before adding the paths to the archive. I guess doing it without cd() is possible, but will look quite ugly.
Why do you not like cd() during builds, if I may ask?

I didn't realize we'd need to strip the prefixes, it does sound like cd is the better option here.

Using cd during builds adds another piece of state to keep track of, which makes it hard to interactively run commands from the middle of a script to debug/test them, since you may have changed directories much earlier; it's also easy to forget to cd back. Arguably, this is mostly a problem for shell scripts, and not as much of an issue for Python, both due to the easy-to-see and automatic (safe) scoping of with cd() and because I'm less likely to interactively run things like with gzip.GzipFile(..):.

aneeshusa · 2016-07-18T04:13:01Z

Looks pretty good so far, I didn't think to use native Python facilities like sorting and just looks at the options to gzip and tarfile earlier. Thanks for taking the time to rewrite this PR!

tonyo · 2016-07-18T21:19:29Z

@aneeshusa updated, except for a few comments (about with cd() and the temporary package name).

aneeshusa · 2016-07-20T02:53:42Z

See my comment above, I think we can get the temporary package name fix working.

aneeshusa · 2016-07-20T20:05:18Z

I did some testing locally and it seems that passing a relative path for dest_archive causes the archive to be created in the dir_to_package, not the PWD when archive_deterministically is called. Looks like this is being caused by the cd() - please fix in whichever way is most ergonomic.

tonyo · 2016-07-21T21:07:55Z

@aneeshusa updated, fixed the issue with cd() and temporary package handling.

bors-servo · 2016-07-25T23:46:16Z

☔ The latest upstream changes (presumably #11967) made this pull request unmergeable. Please resolve the merge conflicts.

tonyo · 2016-07-27T20:25:13Z

Rebased the changes.

Ms2ger · 2016-07-28T13:29:15Z

@aneeshusa I think this is waiting on you?

aneeshusa · 2016-08-04T16:24:51Z

python/servo/command_base.py

@@ -6,13 +6,16 @@
 # <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
 # option. This file may not be copied, modified, or distributed
 # except according to those terms.
-
+import gzip


nit: Please put the empty line back.

aneeshusa · 2016-08-04T16:28:32Z

Sorry about the delay, lgtm aside from a few nits.

Fixes servo#11981.

aneeshusa · 2016-08-04T22:54:32Z

Thanks! @bors-servo r+

bors-servo · 2016-08-04T22:54:33Z

📌 Commit 0a056a9 has been approved by aneeshusa

bors-servo · 2016-08-04T23:15:50Z

⌛ Testing commit 0a056a9 with merge 1086df6...

Create .tar.gz package deterministically using Python's "tarfile"  A development of #12108, creates a .tar.gz package using the `tarfile` and `gzip` modules, without external dependencies. Fixes #11981. Also this fixes the issue when the existing `resources/` directory didn't allow to create a new package and failed with "File exists" error. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11981 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because "more general approach to reproducibility testing is needed"   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12244)

bors-servo · 2016-08-05T00:19:39Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

Summary: We plan to promote TensorBoard’s Pip packages to Bazel targets, but the filenames of these targets can’t actually be statically determined: they must include the TensorBoard version number, which is defined in a Python module. Instead, we’ll build a tarball containing the two wheels. Building tarballs deterministically with system tools is famously troublesome (you can do it with GNU `tar` and a lot of flags, but not portably), and it seems that the standard approach is to use Python’s built-in `tarfile` library. Implementation based off of <servo/servo#12244>, but with changes to make it Python 3-compatible, to use our code style, and to clean up some minor issues and fix some TODOs from the original. The original source is Apache-2 licensed. Test Plan: End-to-end tests included. Replacing the body of `_run_tool` with ```python return subprocess.check_output([ "/bin/sh", "-c", 'x="$(readlink -f "$1")" && cd "$2" && tar czf "$x" .', "unused", ] + args) ``` causes the `test_invariant_under_mtime` test case to fail for the right reason. wchargin-branch: deterministic-tgz

Summary: We plan to promote TensorBoard’s Pip packages to Bazel targets, but the filenames of these targets can’t actually be statically determined: they must include the TensorBoard version number, which is defined in a Python module. Instead, we’ll build a tarball containing the two wheels. Building tarballs deterministically with system tools is famously troublesome (you can do it with GNU `tar` and a lot of flags, but not portably), and it seems that the standard approach is to use Python’s built-in `tarfile` library. Implementation based off of <servo/servo#12244>, but with changes to make it Python 3-compatible, to use our code style, and to remove some unneeded machinery. Original is Apache-2 licensed. Test Plan: End-to-end tests included. Replacing the body of `_run_tool` with ```python return subprocess.check_output(["tar", "czf"] + args) ``` causes the `test_invariant_under_mtime` test case to fail for the right reason. wchargin-branch: deterministic-tgz

highfive assigned jdm Jul 4, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 4, 2016

tonyo mentioned this pull request Jul 4, 2016

Create .tar.gz package deterministically using external commands #12108

Closed

5 tasks

highfive assigned aneeshusa and unassigned jdm Jul 6, 2016

aneeshusa reviewed Jul 18, 2016
View reviewed changes

aneeshusa added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 18, 2016

tonyo force-pushed the package-repro-pure branch from 527ba89 to 4b511ae Compare July 18, 2016 21:17

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jul 18, 2016

aneeshusa added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 20, 2016

tonyo force-pushed the package-repro-pure branch from 4b511ae to 168755f Compare July 21, 2016 20:51

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jul 21, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Jul 25, 2016

tonyo force-pushed the package-repro-pure branch from 168755f to c9518df Compare July 27, 2016 18:08

jdm removed the S-needs-rebase There are merge conflict errors. label Jul 27, 2016

aneeshusa reviewed Aug 4, 2016
View reviewed changes

aneeshusa added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 4, 2016

Create .tar.gz package deterministically

0a056a9

Fixes servo#11981.

tonyo force-pushed the package-repro-pure branch from c9518df to 0a056a9 Compare August 4, 2016 19:51

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Aug 4, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 4, 2016

bors-servo merged commit 0a056a9 into servo:master Aug 5, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 5, 2016

bors-servo mentioned this pull request Aug 5, 2016

Add servo version to mac about servo window. #12250

Merged

3 tasks

wchargin mentioned this pull request Jun 19, 2019

build: add tool to create deterministic tarballs tensorflow/tensorboard#2362

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create .tar.gz package deterministically using Python's "tarfile" #12244

Create .tar.gz package deterministically using Python's "tarfile" #12244

tonyo commented Jul 4, 2016 •

edited by larsbergstrom

Loading

highfive commented Jul 4, 2016

highfive commented Jul 4, 2016

jdm commented Jul 6, 2016

aneeshusa Jul 18, 2016

tonyo Jul 18, 2016

aneeshusa Jul 20, 2016

aneeshusa commented Jul 18, 2016

tonyo commented Jul 18, 2016

aneeshusa commented Jul 20, 2016

aneeshusa commented Jul 20, 2016

tonyo commented Jul 21, 2016

bors-servo commented Jul 25, 2016

tonyo commented Jul 27, 2016

Ms2ger commented Jul 28, 2016

aneeshusa Aug 4, 2016

aneeshusa commented Aug 4, 2016

aneeshusa commented Aug 4, 2016

bors-servo commented Aug 4, 2016

bors-servo commented Aug 4, 2016

bors-servo commented Aug 5, 2016

Create .tar.gz package deterministically using Python's "tarfile" #12244

Create .tar.gz package deterministically using Python's "tarfile" #12244

Conversation

tonyo commented Jul 4, 2016 • edited by larsbergstrom Loading

highfive commented Jul 4, 2016

highfive commented Jul 4, 2016

jdm commented Jul 6, 2016

aneeshusa Jul 18, 2016

Choose a reason for hiding this comment

tonyo Jul 18, 2016

Choose a reason for hiding this comment

aneeshusa Jul 20, 2016

Choose a reason for hiding this comment

aneeshusa commented Jul 18, 2016

tonyo commented Jul 18, 2016

aneeshusa commented Jul 20, 2016

aneeshusa commented Jul 20, 2016

tonyo commented Jul 21, 2016

bors-servo commented Jul 25, 2016

tonyo commented Jul 27, 2016

Ms2ger commented Jul 28, 2016

aneeshusa Aug 4, 2016

Choose a reason for hiding this comment

aneeshusa commented Aug 4, 2016

aneeshusa commented Aug 4, 2016

bors-servo commented Aug 4, 2016

bors-servo commented Aug 4, 2016

bors-servo commented Aug 5, 2016

tonyo commented Jul 4, 2016 •

edited by larsbergstrom

Loading