1455143 - Improvements to upload make target

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

The "upload" make target is taking a few minutes on Windows. We can do better.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1455143 - Make hashlib required;

A missing or unimportable hashlib is only possible on very old or very
busted Pythons. Remove code for conditionally handling a failure to
import it.

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1455143 - Remove hash name validation;

Garbage in garbage out. Let's make this code simpler.

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

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1455143 - Use a global logger instance;

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

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1455143 - Handle directory inputs earlier;

This makes the logic in process_files() simpler.

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

Comment 5

[Gregory Szorc [:gps]]

Attachment: Bug 1455143 - Remove code for failing to obtain a hash;

digest_file() should always return something.

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

Comment 6

[Gregory Szorc [:gps]]

Attachment: Bug 1455143 - Use a reasonable buffer size for reading files;

Reading in 1kb chunks in 2018 is insane.

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

Comment 7

[Gregory Szorc [:gps]]

Attachment: Bug 1455143 - Use .write() instead of print >>;

.write() is the preferred mechanism to write to a file object.

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

Comment 8

[Gregory Szorc [:gps]]

Attachment: Bug 1455143 - Refactor checksumming to occur after upload.py;

checksums.py is now run after upload.py, as part of the "upload" make
target.

As part of the refactor, checksums.py now takes as arguments a list of
directories containing files to checksum. It will recursively checksum
all files in listed directories.

This means we no longer have to pass an explicit list of files into
checksums.py. Instead, we can pass the artifact directory and
automatically checksum everything within. This will allow us to
generate files directly into the artifact directory instead of
having to run upload.py to copy files there.

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

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8969119 [details]
Bug 1455143 - Refactor checksumming to occur after upload.py;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237824/diff/1-2/

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8969112 [details]
Bug 1455143 - Make hashlib required;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237810/diff/1-2/

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8969113 [details]
Bug 1455143 - Remove hash name validation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237812/diff/1-2/

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8969114 [details]
Bug 1455143 - Use a global logger instance;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237814/diff/1-2/

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8969115 [details]
Bug 1455143 - Handle directory inputs earlier;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237816/diff/1-2/

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8969116 [details]
Bug 1455143 - Remove code for failing to obtain a hash;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237818/diff/1-2/

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8969117 [details]
Bug 1455143 - Use a reasonable buffer size for reading files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237820/diff/1-2/

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8969118 [details]
Bug 1455143 - Use .write() instead of print >>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237822/diff/1-2/

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8969119 [details]
Bug 1455143 - Refactor checksumming to occur after upload.py;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237824/diff/2-3/

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8969119 [details]
Bug 1455143 - Refactor checksumming to occur after upload.py;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237824/diff/3-4/

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 8969112 [details]
Bug 1455143 - Make hashlib required;

https://reviewboard.mozilla.org/r/237810/#review243576

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 8969113 [details]
Bug 1455143 - Remove hash name validation;

https://reviewboard.mozilla.org/r/237812/#review243588

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 8969114 [details]
Bug 1455143 - Use a global logger instance;

https://reviewboard.mozilla.org/r/237814/#review243920

Comment 22

[Gregory Szorc [:gps]]

Attachment: Bug 1455143 - Don't remove common test archive;

This target is a prereq for all test archives. Why we only remove a
single test archive, I don't know. We always build this test archive.
So it should always get overwritten and we don't need to remove an
existing file.

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

Comment 23

[Gregory Szorc [:gps]]

Attachment: Bug 1455143 - Generate test archives into final path;

We currently generate these to dist/ and then use upload.py to copy
them into UPLOAD_PATH. This is wasteful. Let's generate the test
archives into UPLOAD_PATH directly, if the variable is defined.

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

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8969112 [details]
Bug 1455143 - Make hashlib required;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237810/diff/2-3/

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 8969113 [details]
Bug 1455143 - Remove hash name validation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237812/diff/2-3/

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8969114 [details]
Bug 1455143 - Use a global logger instance;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237814/diff/2-3/

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8969115 [details]
Bug 1455143 - Handle directory inputs earlier;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237816/diff/2-3/

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8969116 [details]
Bug 1455143 - Remove code for failing to obtain a hash;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237818/diff/2-3/

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8969117 [details]
Bug 1455143 - Use a reasonable buffer size for reading files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237820/diff/2-3/

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8969118 [details]
Bug 1455143 - Use .write() instead of print >>;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237822/diff/2-3/

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8969119 [details]
Bug 1455143 - Refactor checksumming to occur after upload.py;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237824/diff/4-5/

Comment 32

[(not currently active) Ted Mielczarek]

Comment on attachment 8969115 [details]
Bug 1455143 - Handle directory inputs earlier;

https://reviewboard.mozilla.org/r/237816/#review243922

Comment 33

[(not currently active) Ted Mielczarek]

Comment on attachment 8969116 [details]
Bug 1455143 - Remove code for failing to obtain a hash;

https://reviewboard.mozilla.org/r/237818/#review243964

Comment 34

[(not currently active) Ted Mielczarek]

Comment on attachment 8969117 [details]
Bug 1455143 - Use a reasonable buffer size for reading files;

https://reviewboard.mozilla.org/r/237820/#review243966

Comment 35

[(not currently active) Ted Mielczarek]

Comment on attachment 8969118 [details]
Bug 1455143 - Use .write() instead of print >>;

https://reviewboard.mozilla.org/r/237822/#review243968

Comment 36

[(not currently active) Ted Mielczarek]

Comment on attachment 8969119 [details]
Bug 1455143 - Refactor checksumming to occur after upload.py;

https://reviewboard.mozilla.org/r/237824/#review243972

Comment 37

[(not currently active) Ted Mielczarek]

Comment on attachment 8969115 [details]
Bug 1455143 - Handle directory inputs earlier;

https://reviewboard.mozilla.org/r/237816/#review243974

Comment 38

[(not currently active) Ted Mielczarek]

Comment on attachment 8969113 [details]
Bug 1455143 - Remove hash name validation;

https://reviewboard.mozilla.org/r/237812/#review243976

Comment 39

[(not currently active) Ted Mielczarek]

Comment on attachment 8969114 [details]
Bug 1455143 - Use a global logger instance;

https://reviewboard.mozilla.org/r/237814/#review243978

Comment 40

[(not currently active) Ted Mielczarek]

Comment on attachment 8969112 [details]
Bug 1455143 - Make hashlib required;

https://reviewboard.mozilla.org/r/237810/#review243980

Comment 41

[(not currently active) Ted Mielczarek]

Comment on attachment 8969440 [details]
Bug 1455143 - Don't remove common test archive;

https://reviewboard.mozilla.org/r/238194/#review243988

Comment 42

[(not currently active) Ted Mielczarek]

Comment on attachment 8969441 [details]
Bug 1455143 - Generate test archives into final path;

https://reviewboard.mozilla.org/r/238196/#review243990

File a followup on fixing this for everything else? Aside from setup.exe, everything we pass in `UPLOAD_FILES` gets generated after the build.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c987b999ff1
Make hashlib required; r=ted
https://hg.mozilla.org/integration/autoland/rev/37fec6042438
Remove hash name validation; r=ted
https://hg.mozilla.org/integration/autoland/rev/e6d309fde362
Use a global logger instance; r=ted
https://hg.mozilla.org/integration/autoland/rev/2bbe78c011d3
Handle directory inputs earlier; r=ted
https://hg.mozilla.org/integration/autoland/rev/c9cbbb881c78
Remove code for failing to obtain a hash; r=ted
https://hg.mozilla.org/integration/autoland/rev/1f1186400490
Use a reasonable buffer size for reading files; r=ted
https://hg.mozilla.org/integration/autoland/rev/1fa5254b9a69
Use .write() instead of print >>; r=ted
https://hg.mozilla.org/integration/autoland/rev/12cfbcd2ccf4
Refactor checksumming to occur after upload.py; r=ted
https://hg.mozilla.org/integration/autoland/rev/b604acdb1c78
Don't remove common test archive; r=ted
https://hg.mozilla.org/integration/autoland/rev/7f69dd1ba3cb
Generate test archives into final path; r=ted

Comment 44

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/4c987b999ff1
https://hg.mozilla.org/mozilla-central/rev/37fec6042438
https://hg.mozilla.org/mozilla-central/rev/e6d309fde362
https://hg.mozilla.org/mozilla-central/rev/2bbe78c011d3
https://hg.mozilla.org/mozilla-central/rev/c9cbbb881c78
https://hg.mozilla.org/mozilla-central/rev/1f1186400490
https://hg.mozilla.org/mozilla-central/rev/1fa5254b9a69
https://hg.mozilla.org/mozilla-central/rev/12cfbcd2ccf4
https://hg.mozilla.org/mozilla-central/rev/b604acdb1c78
https://hg.mozilla.org/mozilla-central/rev/7f69dd1ba3cb

Comment 45

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1455143 - Stop beetmoving deleted testing artifacts

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

Comment 46

[Johan Lorenzo [:jlorenzo]]

These changes broke the beetmover jobs[1] in the latest nightly. The reason is, the artifacts aren't produced anymore in the build task under the "multi locale" folder. Therefore, ChainOfTrust bails out as they don't exist.

My patch removes the artifacts offending artifacts. :gps, could you check I didn't miss one? :mtabara, this also removes the artifacts from the "ship" graphs, which is expected, I think. 

[1] Like https://tools.taskcluster.net/groups/FJ_74wd1Sl6jUpkObCBPGQ/tasks/Bph5uS4BQlinnQQ6snCcvg/runs/0/logs/public%2Flogs%2Fchain_of_trust.log#L219

Comment 47

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8969656 [details]
Bug 1455143 - Stop beetmoving deleted testing artifacts

https://reviewboard.mozilla.org/r/238450/#review244216

LGTM. I was gonna suggest a cleanup in beetmoverscript as well but I guess we still need these values within the templates mappings for the en-US counterparts.

Comment 48

[Gregory Szorc [:gps]]

Comment on attachment 8969656 [details]
Bug 1455143 - Stop beetmoving deleted testing artifacts

https://reviewboard.mozilla.org/r/238450/#review244282

Aside from robocop.apk removal, this seems reasonable.

FWIW, we plan to follow up and generate most artifacts in their final location. We'll try to keep this file in mind when we do that.

::: taskcluster/taskgraph/transforms/beetmover.py
(Diff revision 1)
>      "target.txt",
> -    "target.web-platform.tests.tar.gz",
> -    "target.xpcshell.tests.zip",
>      "target_info.txt",
> -    "mozharness.zip",
> -    "robocop.apk",

My changes shouldn't have touched robocop.apk.

Comment 49

[Intermittent Failures Robot]

11 failures in 782 pushes (0.014 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-central: 11

Platform breakdown:
* android-5-0-aarch64: 4
* android-4-2-x86: 4
* android-4-0-armv7-api16: 3

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1455143&startday=2018-04-16&endday=2018-04-22&tree=trunk

Comment 50

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8969656 [details]
Bug 1455143 - Stop beetmoving deleted testing artifacts

https://reviewboard.mozilla.org/r/238450/#review244540

::: taskcluster/taskgraph/transforms/beetmover.py
(Diff revision 1)
>      "target.txt",
> -    "target.web-platform.tests.tar.gz",
> -    "target.xpcshell.tests.zip",
>      "target_info.txt",
> -    "mozharness.zip",
> -    "robocop.apk",

Good catch. Fixed.

Comment 51

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8969656 [details]
Bug 1455143 - Stop beetmoving deleted testing artifacts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238450/diff/1-2/

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d615164b2f6f
Stop beetmoving deleted testing artifacts r=gps,mtabara

Comment 53

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d615164b2f6f

Comment 54

[Intermittent Failures Robot]

3 failures in 780 pushes (0.004 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-central: 3

Platform breakdown:
* android-5-0-aarch64: 1
* android-4-2-x86: 1
* android-4-0-armv7-api16: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1455143&startday=2018-04-23&endday=2018-04-29&tree=trunk