Remove the timestamps from the zip archives in remove_build_metadata.py

Remove the timestamps from the zip archives in remove_build_metadata.py

The whole script has been refactored.

BUG=314403
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=292869

[Sébastien Marchand, 2014-11-06 17:44:13]

sebmarchand@chromium.org changed reviewers:
+ maruel@chromium.org

[Sébastien Marchand, 2014-11-06 17:51:11]

Patchset #1 (id:1) has been deleted

[Sébastien Marchand, 2014-11-06 17:51:18]

PTAL.

[M-A Ruel, 2014-11-06 18:38:39]

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py
(right):

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:26: if
not os.path.isfile(f):
if not os.path.isfile(f) or os.path.basename(f).startswith('.'):

(just to save 2 lines :)

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:31: if
ext in non_x_ok_exts:
return (ext in non_x_ok_exts) or (ext in allowed and os.access(f, os.X_OK))

to save another 2 lines :)

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:64: if
os.path.exists(filename + '.pdb'):
is this check still necessary? e.g. zap_timestamp wasn't updated yet.

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:69: def
remove_archive_timestamps(filename):
remove_apk_medata so it fits with remote_pe_metadata

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:72: zf =
zipfile.ZipFile(filename, 'r')
with zipfile.ZipFile(filename, 'r') as zf:

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:74:
out_file, out_filename = tempfile.mkstemp()
prefix='remote_apk_timestamp'
put in try/finally

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:76: zf_o
= zipfile.ZipFile(out_filename, 'w')
with ..

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:80:
info.date_time = (2008, 9, 2, 8, 0 , 0)
I'd prefer 0 than an arbitrary value.

[Sébastien Marchand, 2014-11-06 19:37:32]

Thanks, PTAnL.

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py
(right):

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:26: if
not os.path.isfile(f):
On 2014/11/06 18:38:39, M-A Ruel wrote:
> if not os.path.isfile(f) or os.path.basename(f).startswith('.'):
> 
> (just to save 2 lines :)

Done.

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:31: if
ext in non_x_ok_exts:
On 2014/11/06 18:38:39, M-A Ruel wrote:
> return (ext in non_x_ok_exts) or (ext in allowed and os.access(f, os.X_OK))
> 
> to save another 2 lines :)

Done.

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:64: if
os.path.exists(filename + '.pdb'):
On 2014/11/06 18:38:39, M-A Ruel wrote:
> is this check still necessary? e.g. zap_timestamp wasn't updated yet.

Yes it is, sorry, I'll remove it once we update zap_timestamp.

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:69: def
remove_archive_timestamps(filename):
On 2014/11/06 18:38:39, M-A Ruel wrote:
> remove_apk_medata so it fits with remote_pe_metadata

Done, but it also works for the zip files :)

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:72: zf =
zipfile.ZipFile(filename, 'r')
On 2014/11/06 18:38:39, M-A Ruel wrote:
> with zipfile.ZipFile(filename, 'r') as zf:

Done.

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:74:
out_file, out_filename = tempfile.mkstemp()
On 2014/11/06 18:38:38, M-A Ruel wrote:
> prefix='remote_apk_timestamp'
> put in try/finally

Done.

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:76: zf_o
= zipfile.ZipFile(out_filename, 'w')
On 2014/11/06 18:38:39, M-A Ruel wrote:
> with ..

Done.

https://codereview.chromium.org/708803003/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:80:
info.date_time = (2008, 9, 2, 8, 0 , 0)
On 2014/11/06 18:38:39, M-A Ruel wrote:
> I'd prefer 0 than an arbitrary value.

Doesn't work with 0... It looks like the minimal accepted value is (1980, 1, 1,
0, 0, 0):
https://github.com/enthought/Python-2.7.3/blob/master/Lib/zipfile.py#L277

[M-A Ruel, 2014-11-06 19:44:09]

https://codereview.chromium.org/708803003/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py
(right):

https://codereview.chromium.org/708803003/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:72:
finally:
No, I meant in finally at end of function:

if os.path.isfile(out_filename):
  os.remove(out_filename)

[Sébastien Marchand, 2014-11-06 20:40:12]

Thanks, PTAnL.

https://codereview.chromium.org/708803003/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py
(right):

https://codereview.chromium.org/708803003/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:72:
finally:
On 2014/11/06 19:44:09, M-A Ruel wrote:
> No, I meant in finally at end of function:
> 
> if os.path.isfile(out_filename):
>   os.remove(out_filename)

Of course ! Sorry :)

[M-A Ruel, 2014-11-06 20:44:44]

lgtm with fix

https://codereview.chromium.org/708803003/diff/60001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py
(right):

https://codereview.chromium.org/708803003/diff/60001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/isolate/resources/remove_build_metadata.py:71:
out_file, out_filename = tempfile.mkstemp(prefix='remote_apk_timestamp')
Move lines 71-72 before line 70.

[Sébastien Marchand, 2014-11-06 20:48:07]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-11-06 20:48:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/708803003/80001

[commit-bot: I haz the power, 2014-11-06 20:49:06]

Committed patchset #4 (id:80001) as 292869