Closed Bug 1080338 Opened 5 years ago Closed 2 years ago

preflight_cppunittest in desktop_unittest.py needs to be made aware of the new GreD on OSX

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

All
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: spohl, Unassigned)

References

Details

Attachments

(1 file)

The preflight_cppunittest in desktop_unittest.py is currently copying TestStartupCacheTelemetry.js and TestStartupCacheTelemetry.manifest to Contents/MacOS in the app dir. With the v2 signing changes however, the expected location is Contents/Resources. We need to adjust the directory accordingly before moving the files.

This blocks bug 1077282 from landing.
Attached patch Patch (wip)Splinter Review
Something like this would do, but I'm not sure if the import of mozinfo works as expected. Ben, is there a way to test this? Or is there another (preferred) way to check whether we're running on OSX? Thanks!
Attachment #8502246 - Flags: feedback?(bhearsum)
Summary: abs_app_dir in preflight_cppunittest (desktop_unittest.py) needs to be made aware of the new GreD on OSX → preflight_cppunittest in desktop_unittest.py needs to be made aware of the new GreD on OSX
Comment on attachment 8502246 [details] [diff] [review]
Patch (wip)

I don't think I'm a great reviewer for this one...Jordan, maybe you are (or know who is?)
Attachment #8502246 - Flags: feedback?(bhearsum) → feedback?(jlund)
(In reply to Ben Hearsum [:bhearsum] from comment #4)
> Comment on attachment 8502246 [details] [diff] [review]
> Patch (wip)
> 
> I don't think I'm a great reviewer for this one...Jordan, maybe you are (or
> know who is?)

oh cool, this *should* work as we can reach mozinfo from elsewhere in mozharness. I won't block if you feel strongly but I tend to lean on the side of making things configurable. Maybe we can add an item to the config used for osx cppunittests. This way, if we need to modify this for osx or any other platform in the future, it's just a change to a config not the script. Also, I suppose we should change the name of this var since it won't be the actual abs_app_dir if it's osx now.

i.e. this is the call we do for cppunitests osx: /tools/buildbot/bin/python scripts/scripts/desktop_unittest.py --cfg unittests/mac_unittest.py --cppunittest-suite cppunittest --blob-upload-branch mozilla-central --download-symbols ondemand

so we could add something like the following to unittests/mac_unittest.py:

'cppunittests_telemetry_dest': 's(app_dir_dirname)%/Resources' # or whatever key/name you want

then in the script:
    telemetry_dest = self.query_abs_app_dir()
    if self.config.get('cppunitests_telemetry_dest'):
       telemetry_dest = self.config['cppunitests_telemetry_dest'] % {
            'app_dir_dirname': os.path.dirname(abs_app_dir)
       }

I am going to ping dminor here as he as recently worked on cppunittests, specifically within the same method as this patch.

dminor, do you see any issue changing the logic in preflight_cppunittest() regardless how we do it?

spohl, in the meantime you can test this patch or my suggestion anytime without review by landing on a ash-mozharness (somewhat of a mozharness try repo). Essentially, you will land this patch there, and then you can re-trigger individual jobs on the 'ash' branch (they will pick up the latest ash-mozharness rev everytime), or push to ash with a merge of m-c -> ash and that will trigger every job across every platform. backout the patch after if it gets an r- or breaks ash.

For more instructions, see: https://wiki.mozilla.org/ReleaseEngineering/SpecialBranches#Ash

does this help?
Flags: needinfo?(dminor)
Attachment #8502246 - Flags: feedback?(jlund) → feedback+
I'm going to be out all of next week, so if someone else could drive this from here, I'd appreciate it.

Bug 1077282 landed with a workaround for this bug (see https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a3560432b0). While testing a patch for mozharness here, we should revert the workaround. It can be reverted all together in the tree after a patch for this bug has landed.
I don't see any problems here.
Flags: needinfo?(dminor)
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.