1213816 - mochitest fails on Arch Linux because Python2's `platform.linux_distribution()` returns bogus values.

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jose Diez]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.60 Safari/537.36

Steps to reproduce:

On an up-to-date install of Arch Linux, mochitest fails to run because `testing/mozbase/mozinfo/mozinfo/mozinfo.py` is unable to detect the distribution, version and OS version.

To reproduce:

    $ ./mach mochitest


Actual results:

mochitest fails to parse the `skip-if` strings because `os_version` is bound to `StringVersion(" ")`. 


Expected results:

mochitest should run correctly.

Comment 1

[Jose Diez]

Attachment: Fixes #1213816

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8672584 [details] [diff] [review]
Fixes #1213816

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

platform.linux_distribution also came up recently in bug 1117543.

r+ with requested changes made. Since this is your first patch, please upload a new version and it should be a rubber stamp approval from me.

::: testing/mozbase/mozinfo/mozinfo/mozinfo.py
@@ +100,5 @@
> +        if "ARCH" in release:
> +            distro = 'arch'
> +
> +        version = release
> +        os_version = release

I'd feel better if these 2 lines were also indented and only applied to the Arch workaround.

The reason is that if platform.linux_distribution() starts returning weird values on other distros, the workaround for Arch may not be appropriate and we may set version and os_version to an inappropriate value on these distros. I favor having an explicit human review when things break in the future. And that means making the workaround as narrow as possible.

Along that vein, is the new value of "release" overwriting an earlier set value? Again, I'd prefer we only do this for Arch if it is.

Comment 3

[Jose Diez]

Attachment: Changes requested by :gps

Comment 4

[Jose Diez]

(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 8672584 [details] [diff] [review]
> Fixes #1213816
> 
> Review of attachment 8672584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> platform.linux_distribution also came up recently in bug 1117543.
> 
> r+ with requested changes made. Since this is your first patch, please
> upload a new version and it should be a rubber stamp approval from me.
> 

Done.

> ::: testing/mozbase/mozinfo/mozinfo/mozinfo.py
> @@ +100,5 @@
> > +        if "ARCH" in release:
> > +            distro = 'arch'
> > +
> > +        version = release
> > +        os_version = release
> 
> I'd feel better if these 2 lines were also indented and only applied to the
> Arch workaround.
> 
> The reason is that if platform.linux_distribution() starts returning weird
> values on other distros, the workaround for Arch may not be appropriate and
> we may set version and os_version to an inappropriate value on these
> distros. I favor having an explicit human review when things break in the
> future. And that means making the workaround as narrow as possible.

That's fair enough. The error I run into was fairly cryptic (a ParseError in mochitest) and took some digging to find the source of the problem; that's why I originally wanted to return at least some value if platform.linux_distribution() starts acting up.

Perhaps the Right Way is to make mozinfo.py throw a warning/exception if any of the values isn't present, or to handle the error more gracefully in mochitest.   

> 
> Along that vein, is the new value of "release" overwriting an earlier set
> value? Again, I'd prefer we only do this for Arch if it is.

Turns out `release` was set to the right value near the beginning of mozinfo.py[1], so I was in fact overwriting it (but with the same value!) It's cleaner now.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozinfo/mozinfo/mozinfo.py#61

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8672736 [details] [diff] [review]
Changes requested by :gps

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

Sorry for the review lag: I haven't been feeling well the past few days.

I'll land this for you. Congratulations on your first contribution to Firefox!

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/9159ed3ede29dc7ab6cf0b28f56425a92f6d394d
Bug 1213816 - Properly set OS properties on Arch; r=gps

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9159ed3ede29