[mozrunner] should we make mozcrash an optional dependency of mozrunner ?

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: parkouss, Assigned: parkouss)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
mozrunner currently requires mozcrash, but in some cases we don't want/need it, e.g. in mozregression.

See bug 1185632 comment 3.

Should we make that dependency optional ?
(Assignee)

Comment 1

3 years ago
:ted, thoughts ? I *feel* that you are the right person to ask this for, but feel free to redirect the NI if it is not the case.
Flags: needinfo?(ted)
It seems like mozcrash is generally useful when running an app. The problems in that bug sound like we could work around them.
Flags: needinfo?(ted)
(Assignee)

Updated

3 years ago
Blocks: 1185602
(Assignee)

Comment 3

3 years ago
Fixing this would resolve bug 1185602, (see bug 1185602 comment 7). I am not sure if we should fix mozcrash instead, but this is a workaround that would allow to not require mozcrash when installing mozrunner, and allowing to have a try/except around mozcrash import would fix the issue.
(Assignee)

Comment 4

3 years ago
Created attachment 8664217 [details] [diff] [review]
bug-1190371.patch

So, asking for review - we'll see what you think of it Ted :)

Basically this makes mozcrash an optional deps for mozrunner. So:

pip install mozrunner

would not install mozcrash, but:

pip install mozrunner[crash]

will.

For in-tree code, I don't think it will change anything, we always install every mozbase dependencies I think. But this may impact out-of-the-tree code that rely on mozcrash when they install mozrunner (I don't think there is any, but not sure) - in that case they should upgrade their dependency to "mozrunner[crash]".
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8664217 - Flags: review?(ted)
Comment on attachment 8664217 [details] [diff] [review]
bug-1190371.patch

Review of attachment 8664217 [details] [diff] [review]:
-----------------------------------------------------------------

I don't feel very strongly about this, so this is fine if it helps that use case. Sorry for the long review delay!
Attachment #8664217 - Flags: review?(ted) → review+
(Assignee)

Comment 7

3 years ago
Thanks Ted! Yep, that will help for mozregression - we will be able to make it work for MSYS2 on Windows 8.1 x64 (even if this version is not supported by test harnesses), and it will allow to remove the mozcrash dependency by the way.

Pushed to try, just to be sure.
(Assignee)

Comment 8

3 years ago
I can't push myself right now, the tree is closed.

Try is green - thanks for checkin this in!

Leave open so I can release a new mozrunner on pypi before closing the bug.
Keywords: checkin-needed, leave-open
(Assignee)

Comment 13

3 years ago
Released on pypi:

Submitting dist/mozrunner-6.11.tar.gz to https://pypi.python.org/pypi
Server response (200): OK
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.