[mozversion] mozversion requires mozdevice, it shoudn't

RESOLVED FIXED in Firefox 42

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: parkouss, Assigned: parkouss)

Tracking

(Blocks: 1 bug)

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Currently mozversion dependencies are:

dependencies = ['mozdevice >= 0.44',
                'mozfile >= 1.0',
                'mozlog >= 3.0']

It does not make sense to require mozdevice here. And I am fighting pypi nightmare dependencies because of that when installing talos locally (and talos do not require mozdevice at all now!).
(Assignee)

Comment 1

3 years ago
:davehunt, do you mind if I fix this ? How is mozversion used around ?
Flags: needinfo?(dave.hunt)
mozversion uses mozdevice to determine the version information on Firefox OS devices: http://hg.mozilla.org/mozilla-central/file/32712cd01159/testing/mozbase/mozversion/mozversion/mozversion.py#l185
Flags: needinfo?(dave.hunt)
(Assignee)

Comment 3

3 years ago
Well, I am not asking to remove any code, just make the dependency optional (sorry, that was not clear enough).

We can use pip extra dependencies to still be able to require it explicitly from a pip install command, something like:

> pip install mozversion[remote]

See https://pythonhosted.org/setuptools/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies.
Flags: needinfo?(dave.hunt)
I see, that makes sense. I wonder if the feature should be 'device' rather than 'remote' though? We should also update the documentation, and notify as many consumers of mozversion for Firefox OS that we're aware of. I've CC'd a few that come to mind.
Flags: needinfo?(dave.hunt)
(Assignee)

Comment 5

3 years ago
Created attachment 8642353 [details] [diff] [review]
1189847.patch

Thanks Dave!

I followed your recommendation, using "device" as a feature name. I also added some documentation, and the generation is OK locally.

I am not sure if you should be reviewer here, feel free to redirect the r? if it is not the case. :)
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8642353 - Flags: review?(dave.hunt)
(Assignee)

Updated

3 years ago
Blocks: 1190334
Comment on attachment 8642353 [details] [diff] [review]
1189847.patch

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

With the nit addressed this looks good to me.

::: testing/mozbase/docs/mozversion.rst
@@ +5,5 @@
>  provides version information such as the application name and the changesets
>  that it has been built from. This is commonly used in reporting or for
>  conditional logic based on the application under test.
>  
> +Note that mozversion can report the version of remote devices (e.g. FirefoxOS)

Nit: Firefox OS
Attachment #8642353 - Flags: review?(dave.hunt) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8642403 [details] [diff] [review]
1189847.patch

Thanks! Fixed. :)

I will push that to try later. I'm not sure what try syntax I should use, I was thinking about one desktop and one b2g device build with one mochitest and one reftest test (in case they use that). If you have a better idea, please tell me.
Attachment #8642353 - Attachment is obsolete: true
Attachment #8642403 - Flags: review+
(Assignee)

Comment 9

3 years ago
try is green.
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
See Also: → bug 1190907
https://hg.mozilla.org/mozilla-central/rev/95e44c9c8457
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Updated

3 years ago
See Also: → bug 1191197
You need to log in before you can comment on or make changes to this bug.