Allow artifact builds to consume generated test files from origin builds

RESOLVED FIXED in Firefox 54

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: cmanchester, Assigned: cmanchester)

Tracking

(Blocks 2 bugs)

unspecified
mozilla54
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments)

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.
Duplicate of this bug: 1317769
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.
Attachment #8826383 - Flags: review?(gps) → review+
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.
Attachment #8826384 - Flags: review?(gps) → review-
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 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
gps, can you give me a better idea of how to address the review in comment 5, or adjust it based on comment 7?
Flags: needinfo?(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)`.
Flags: needinfo?(gps)
(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.
Flags: needinfo?(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.
Attachment #8826384 - Flags: review- → review+
Pushed by cmanchester@mozilla.com:
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
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!
Flags: needinfo?(cmanchester)
See Also: → 1335161
Depends on: 1335164
This should be able to re-land as is once the patch from bug 1335164 is reviewed.
Flags: needinfo?(gps)
Flags: needinfo?(cmanchester)
Pushed by cmanchester@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/054ec084afc5
https://hg.mozilla.org/mozilla-central/rev/6900282d3c99
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.