1330421 - Allow artifact builds to consume generated test files from origin builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

This entails changing the structure of the test archives to include these files in the common package and consuming them in the artifact code.

From local testing this will handle several of the tests that are failing when run against an artifact build.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1330421 - Change the structure of test archives include generated test support files in common.tests.zip for the benefit of artifact builds.

Generated support files for specific tests generally end up in a harness specific
test archive, but artifact builds, which might not want to generate these files
themselves, get all of their test related files from the common.tests.zip. This
effectively moves a small number of these support files from harness specific
archives to the common test archive, making it possible to run tests that
depend on these files against artifact builds.

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

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1330421 - Update artifact code to populate generated test support files from the tests archive.

This updates the client artifact code to locate test support files in the
common test archive and populate the objdir with these files appropriately.

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

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8826383 [details]
Bug 1330421 - Change the structure of test archives include generated test support files in common.tests.zip for the benefit of artifact builds.

https://reviewboard.mozilla.org/r/104298/#review106552

::: python/mozbuild/mozbuild/action/test_archive.py:469
(Diff revision 1)
> +    # Conveniently, the generated files we care about will already
> +    # exist in the objdir, so we can identify relevant files if
> +    # they're an `ExistingFile` instance.
> +    return [mozpath.join('_tests', p) for p in registry.paths()
> +            if isinstance(registry[p], ExistingFile)]

This feels a bit fragile. But I /think/ this assumption is correct.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8826384 [details]
Bug 1330421 - Update artifact code to populate generated test support files from the tests archive.

https://reviewboard.mozilla.org/r/104300/#review106554

::: python/mozbuild/mozbuild/artifacts.py:66
(Diff revision 1)
>  import zipfile
>  
>  import pylru
>  import taskcluster
>  
> +from mozbuild.action.test_archive import OBJDIR_TEST_FILES

I'm not a huge fan of importing from mozbuild.action here, as that package is supposed to only contain code for performing low-level "actions" during the build.

The fact that test_archive.py is a canonical source of useful data is a by-product of my laziness to find a better home for that data when I wrote that file. IIRC, I wanted to store the data in moz.build somehow.

Anyway, this isn't a show stopper. I just thought you'd appreciate the history and intended purpose of the mozbuild.action modules.

::: python/mozbuild/mozbuild/artifacts.py:208
(Diff revision 1)
> +                        dest = files_entry['dest']
> +                        origin_pattern = mozpath.join(dest, origin_pattern)
> +                        leaf_filename = filename[len(dest) + 1:]
> +                    if mozpath.match(filename, origin_pattern):
> +                        destpath = mozpath.join('..', files_entry['base'], leaf_filename)
> +                        mode = entry['external_attr'] >> 16

Please use an appropriate function and/or constants from the `stat` module. e.g. `stat.S_IMODE()`.

Also, there are potential security issues with files that have the setuid or setgid bits set. There are a few places in the tree where we unset those bits out of paranoia. Please also do that here.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8826384 [details]
Bug 1330421 - Update artifact code to populate generated test support files from the tests archive.

https://reviewboard.mozilla.org/r/104300/#review106554

I should have said this patch is mostly good. Just the bit-level operation nit to fix.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8826384 [details]
Bug 1330421 - Update artifact code to populate generated test support files from the tests archive.

https://reviewboard.mozilla.org/r/104300/#review106554

> Please use an appropriate function and/or constants from the `stat` module. e.g. `stat.S_IMODE()`.
> 
> Also, there are potential security issues with files that have the setuid or setgid bits set. There are a few places in the tree where we unset those bits out of paranoia. Please also do that here.

This pattern appears in a few more places in this file and throughout the tree, I'm not really sure how else to do this. We're attempting to just forward the mode bits from the zip entry, which this will do according to https://hg.python.org/cpython/file/v2.7.13/Lib/zipfile.py#l1139

Comment 8

[Chris Manchester (limited bugmail, email directly)]

gps, can you give me a better idea of how to address the review in comment 5, or adjust it based on comment 7?

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8826384 [details]
Bug 1330421 - Update artifact code to populate generated test support files from the tests archive.

https://reviewboard.mozilla.org/r/104300/#review106554

> This pattern appears in a few more places in this file and throughout the tree, I'm not really sure how else to do this. We're attempting to just forward the mode bits from the zip entry, which this will do according to https://hg.python.org/cpython/file/v2.7.13/Lib/zipfile.py#l1139

Yes, bit shifting to get the 16 bits consisting of the file mode is correct. However, it isn't very readable.

Anyway, this line is equivalent to `mode = stat.S_IMODE(entry['external_attr'])` which is equivalent to `mode = entry['external_attr'] & 07777.` That first octal is the setuid, setgid, and sticky bits, which are almost certainly never wanted. So, replace this with `mode = entry['external_attr'] & 00777` or `mode = entry['external_attr'] & (stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO)`.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 8826384 [details]
> Bug 1330421 - Update artifact code to populate generated test support files
> from the tests archive.
> 
> https://reviewboard.mozilla.org/r/104300/#review106554
> 
> > This pattern appears in a few more places in this file and throughout the tree, I'm not really sure how else to do this. We're attempting to just forward the mode bits from the zip entry, which this will do according to https://hg.python.org/cpython/file/v2.7.13/Lib/zipfile.py#l1139
> 
> Yes, bit shifting to get the 16 bits consisting of the file mode is correct.
> However, it isn't very readable.
> 
> Anyway, this line is equivalent to `mode =
> stat.S_IMODE(entry['external_attr'])` which is equivalent to `mode =
> entry['external_attr'] & 07777.` That first octal is the setuid, setgid, and
> sticky bits, which are almost certainly never wanted. So, replace this with
> `mode = entry['external_attr'] & 00777` or `mode = entry['external_attr'] &
> (stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO)`.

These don't look equivalent to me. Anyway, when I make this change, it causes test failures.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8826384 [details]
Bug 1330421 - Update artifact code to populate generated test support files from the tests archive.

https://reviewboard.mozilla.org/r/104300/#review109106

Meh.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9778c00980aa
Change the structure of test archives include generated test support files in common.tests.zip for the benefit of artifact builds. r=gps
https://hg.mozilla.org/integration/autoland/rev/89e2853bf0e8
Update artifact code to populate generated test support files from the tests archive. r=gps

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing to fetch websocketprocessbridge_requirements.txt, at least for Android reftests:

https://hg.mozilla.org/integration/autoland/rev/d434f525133357141b5c5d87fb5433f6d871a6dc
https://hg.mozilla.org/integration/autoland/rev/baf39ca711ce6394bf5d0c5029ba1ed48f058bb3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=89e2853bf0e8f520ac17d566bca20386c67fb339
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=73063415&repo=autoland

> Could not install python package: /home/worker/workspace/build/venv/bin/pip install --no-deps --timeout 120 -r /home/worker/workspace/build/tests/mochitest/websocketprocessbridge/websocketprocessbridge_requirements.txt failed after 5 tries!

Comment 14

[Chris Manchester (limited bugmail, email directly)]

This should be able to re-land as is once the patch from bug 1335164 is reviewed.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/054ec084afc5
Change the structure of test archives include generated test support files in common.tests.zip for the benefit of artifact builds. r=gps
https://hg.mozilla.org/integration/autoland/rev/6900282d3c99
Update artifact code to populate generated test support files from the tests archive. r=gps

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/054ec084afc5
https://hg.mozilla.org/mozilla-central/rev/6900282d3c99