Provide more granular OS version information in mozinfo

RESOLVED FIXED in mozilla32

Status

Testing
Mozbase
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: RyanVM, Assigned: marco)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

4 years ago
It would be nice if we could disable tests on a more granular level (i.e. only Windows 8) instead of across an entire platform. That information is not currently exposed in a usable way.
Does mozinfo.version not work for you?

{'version': '6.1.7601', 'os': 'win', 'service_pack': 'Service Pack 1', 'bits': 64, 'processor': 'x86_64'}

We might have to do something like startswith('6.1') but as I can see the information is available in mozinfo.

On the other hand we could split out the build number so we get something like:

{'version': '6.1', 'build': '7601', 'os': 'win', 'service_pack': 'Service Pack 1', 'bits': 64, 'processor': 'x86_64'}
This info is probably not super useful as it is currently formatted, I think we could fix it a little bit and make it useful. I think your suggestion to split out the build number would be helpful for Windows. On Linux we apparently stick the distribution name+version in the version field which seems odd:
>>> mozinfo.version
'Ubuntu 12.04'

and on OS X we do the same, which is odd:
>>> mozinfo.version
'OS X 10.8.5'

If we make these a little more sane I think this could be useful. I might also suggestion renaming "version" to "os_version" to be clearer. Here's my concrete suggestion:

For Windows, drop the build number so we have simply:
{'os': 'win', 'os_version': '6.1', ... }

For Linux, split the distro name off to a separate field so we have:
{'os': 'linux', 'os_version': '12.04', 'linux_distro': 'Ubuntu', ...}

For OS X, split off the patch level and drop the "OS X" bit so we have:
{'os': 'mac', 'os_version': '10.8', ...}

If we need patchlevel information we can split that to a separate field, but that seems less likely to be useful.

Ryan: does that sound sane?
Flags: needinfo?(ryanvm)
Also, we'll need to be careful about how we treat this info for Android/B2G test harnesses, since this will be the info for the *host* system running the test harness script, not the actual system under test.
(Reporter)

Comment 4

4 years ago
os_version as described sounds perfect, thanks!
Flags: needinfo?(ryanvm)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 993711
(Assignee)

Comment 6

4 years ago
Created attachment 8405359 [details] [diff] [review]
WIP

I haven't tested yet on Mac and Windows and I guess I need to fix all the places where mozinfo.version is used.
Attachment #8405359 - Flags: feedback?(ted)
(Assignee)

Comment 7

4 years ago
Looks like the two users of mozinfo.version are:

 - http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/moztest/moztest/results.py#24
   Here I think we can simply use mozinfo.os_version

 - http://mxr.mozilla.org/mozilla-central/source/testing/tps/tps/testrunner.py#456
   Here I'm not sure, because I don't know anything about TPS. I guess it doesn't really need the detailed information contained in mozinfo.version and it could work with mozinfo.os_version.
What about keeping mozinfo.version to be backward compatible, and introduce new properties for more detailed information? Thing is we have other customers of this code outside of m-c too.
(Assignee)

Comment 9

4 years ago
Created attachment 8405652 [details] [diff] [review]
WIP v2

(In reply to Henrik Skupin (:whimboo) from comment #8)
> What about keeping mozinfo.version to be backward compatible, and introduce
> new properties for more detailed information? Thing is we have other
> customers of this code outside of m-c too.

Sounds good to me.
Attachment #8405359 - Attachment is obsolete: true
Attachment #8405359 - Flags: feedback?(ted)
Attachment #8405652 - Flags: feedback?(ted)
Assignee: nobody → mar.castelluccio
Comment on attachment 8405652 [details] [diff] [review]
WIP v2

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

This looks fine. My only reservation is what I said in comment 3: we need to be careful about this because it won't be correct for Android/B2G usage. I suspect we'll always be writing conditions like 'skip-if = os == "mac" && os_version == "10.9"', which will be okay because "os" will be correct at least.

We should get a followup filed to get os_version set correctly for Android/B2G.

::: testing/mozbase/mozinfo/mozinfo/mozinfo.py
@@ +44,5 @@
>          processor = os.environ.get("PROCESSOR_ARCHITEW6432", processor)
>      else:
>          processor = os.environ.get('PROCESSOR_ARCHITECTURE', processor)
>      system = os.environ.get("OS", system).replace('_', ' ')
> +    (mayor, minor, _, _, service_pack, _, _, _, _) = os.sys.getwindowsversion()

spelling nit: "major"
Attachment #8405652 - Flags: feedback?(ted) → feedback+
(Assignee)

Comment 11

4 years ago
Looks like the patch works ok if you run mochitests via make, but not via mach.
For example, I've added a test with: run-if = os == "mac" && os_version == "10.9". The test isn't added to all-tests.json and so mach doesn't run it.
(Assignee)

Comment 12

4 years ago
Created attachment 8412214 [details] [diff] [review]
Patch
Attachment #8405652 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8412216 [details] [diff] [review]
Tests

I wrote these tests for local testing purposes, I don't know if we want to check them in.
(Assignee)

Comment 14

4 years ago
Created attachment 8413918 [details] [diff] [review]
Patch

This fixes the problem by using mozinfo in the TreeMetadataEmitter class.
Attachment #8412214 - Attachment is obsolete: true
Attachment #8413918 - Flags: review?(ted)
(Assignee)

Comment 15

4 years ago
I guess I need to pass the json path to find_and_update_from_json, because a try run that I've spun failed with "mozbuild.base.BuildEnvironmentNotFoundException: Could not find Mozilla source tree or build environment."
(Assignee)

Comment 16

4 years ago
Created attachment 8413932 [details] [diff] [review]
Patch
Attachment #8413918 - Attachment is obsolete: true
Attachment #8413918 - Flags: review?(ted)
Attachment #8413932 - Flags: review?(ted)
Comment on attachment 8413932 [details] [diff] [review]
Patch

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

Oh, well that was silly wasn't it?
Attachment #8413932 - Flags: review?(ted) → review+
(Assignee)

Comment 18

4 years ago
I've spun a try run, it fails only in the "Static Rooting Hazard Analysis" run: https://tbpl.mozilla.org/?tree=Try&rev=15a7a60774b6

The error is still the same ("mozbuild.base.BuildEnvironmentNotFoundException").

I've added a new catch to ignore this exception, now the try run is green: https://tbpl.mozilla.org/?tree=Try&rev=a257740a1778
(Assignee)

Comment 19

4 years ago
Created attachment 8415855 [details] [diff] [review]
Patch
Attachment #8412216 - Attachment is obsolete: true
Attachment #8413932 - Attachment is obsolete: true
Attachment #8415855 - Flags: review?(ted)
Comment on attachment 8415855 [details] [diff] [review]
Patch

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

::: testing/mozbase/mozinfo/mozinfo/mozinfo.py
@@ +154,5 @@
>              update(json_path)
>              return json_path
>      except ImportError:
>          pass
> +    except BuildEnvironmentNotFoundException:

I'm a little worried that we'll paper over real errors with this, but I guess we can deal with this.
Attachment #8415855 - Flags: review?(ted) → review+
sfink: do you know why we'd hit this error on the rooting hazard analysis runs?
Flags: needinfo?(sphink)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #21)
> sfink: do you know why we'd hit this error on the rooting hazard analysis
> runs?

It uses mach to build. It's the only build job that uses mach.

I haven't really looked at this bug, but there was some problem with mach mentioned in comment 11.
Flags: needinfo?(sphink)
Note that the hazard build is currently restricted to linux64 only. Though I hope to be adding a b2g variant before too long.
(Assignee)

Comment 24

4 years ago
Ok, this is still not correct on build machines.

For example, there were three tests that were supposed to run respectively on Windows XP, Windows 7 and Windows 8. For some reason, just the Windows 7 test was "built" and it was run on the Windows 7 machine and skipped on Windows XP and Windows 8.

Here's the Windows XP log: https://tbpl.mozilla.org/php/getParsedLog.php?id=38845488&tree=Try&full=1
You can see "test_win_61.html" was skipped. "test_win_52.html" should've run.

Any idea about what's happening? Is it possible that we're generating the list of the tests on a machine and then handing it to the other machines?
Flags: needinfo?(ted)
Flags: needinfo?(sphink)
So yes, we only do one "windows" build, and then run that on the other OS versions. You're running into bug 989583.
Flags: needinfo?(ted)
(I'm not the one to ask. I mostly only know about the hazard builds. I was going to redirect to ted, but you already asked him and he already replied.)
Flags: needinfo?(sphink)
(Assignee)

Comment 27

4 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #25)
> So yes, we only do one "windows" build, and then run that on the other OS
> versions. You're running into bug 989583.

Ok, so let's land this with the understanding that we need to fix bug 989583 to make the feature actually usable on build machines.
Depends on: 989583
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e09e3bce7c0d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
I this patch known to work on Ubuntu 14.04? I get

0:39.31 creating ./config.status
 0:39.73 Traceback (most recent call last):
 0:39.73   File "./config.status", line 978, in <module>
 0:39.73     config_status(**args)
 0:39.73   File "/opt/Projects/mozilla-html5/python/mozbuild/mozbuild/config_status.py", line 134, in config_status
 0:39.74     emitter = TreeMetadataEmitter(env)
 0:39.74   File "/opt/Projects/mozilla-html5/python/mozbuild/mozbuild/frontend/emitter.py", line 74, in __init__
 0:39.74     mozinfo.find_and_update_from_json(config.topobjdir)
 0:39.74 AttributeError: 'module' object has no attribute 'find_and_update_from_json'
 0:39.76 *** Fix above errors and then restart with\
 0:39.76                "/usr/bin/make -f client.mk build"
 0:39.76 make[2]: *** [configure] Error 1
 0:39.76 make[1]: *** [/opt/Projects/mozilla-html5/../obj-debug/Makefile] Error 2
 0:39.76 make: *** [build] Error 2
 0:39.79 0 compiler warnings present.

...even after removing the objdir and trying again.
Undoing this patch allows me to build again.
Do you have an older version of mozinfo installed globally? That method definitely exists in the in-tree mozinfo:
http://hg.mozilla.org/mozilla-central/annotate/4c4dcf7533cb/testing/mozbase/mozinfo/mozinfo/mozinfo.py#l137

It's possible there's some Python include path ordering problem since this is still during the configure step.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #32)
> Do you have an older version of mozinfo installed globally?

find / | grep mozinfo.py
finds files only inside Mozilla repos and objdirs and I've never knowingly installed mozinfo globally. (I wouldn't even know how.)
It breaks here on Fedora 20 as well.
Yes I have mozinfo installed globally, thanks pip and other Mozilla tools. *sigh*

And according to pip it is the latest version available.
Filed bug 1008648 for that.
I've heard several people complaining about this. Ted, what's the impact of backing this out while we figure out what's going on here?
Flags: needinfo?(ted)
Flags: needinfo?(ted)
Depends on: 1044414
You need to log in before you can comment on or make changes to this bug.