Closed Bug 606524 Opened 14 years ago Closed 13 years ago

new package: MozInfo

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: k0scist, Assigned: k0scist)

References

Details

(Whiteboard: [mozmill-2.0+])

Attachments

(5 files, 4 obsolete files)

We do a bunch of checks all over the code for various bits of system information.  We currently don't do this in a unified way, so the checks suffer for consistency as well as copying+pasting code that needs to be updated (see https://bugzilla.mozilla.org/show_bug.cgi?id=602460 with a fix copied from Talos that was copied from elsewhere).  We should make checks universal and consistent with a long term objective of doing things consistently across python.

This should mostly be an interface class that just unifies the various checks needed to establish how we want to do things.

There should be a singleton that contains the machine/environment info for the running process:

"""
class MozInfo(object):
  def __init__(self):
    ....

MozInfo = MozInfo()
"""

The object should contain information about different platforms.  My original thought was to do 'windows', 'macosx', and 'linux', though regarding https://bugzilla.mozilla.org/show_bug.cgi?id=543492 I am unsure if sunos is a different platform or if this should be 'windows', 'macosx', and 'unix' (I realize Mac is unix, but it is "special").  I'm not sure where the list of supported platforms for Firefox is.  I'm even less sure where the list of supported platforms for MozMill is.

The class will also contain information about the OS version, the architecture (32 bit vs 64 bit) and whatever other checks we need.
As another case of why its important to do things universally, i'm guessing what we're doing in killableprocess is not what we think we're doing. i'm guessing that http://github.com/mozautomation/mozmill/blob/master/mozprocess/mozprocess/killableprocess.py#L70 is going to do something we're not expecting on win64 and on cygwin
I don't think we have a list somewhere which shows all the supported platforms. It's really something we should come up with. At the end this list should be identical with the list of Firefox.

Yes we support Solaris and it needs its own conditions because it's not identified as Linux. The basis code of OS X is Darwin (http://en.wikipedia.org/wiki/Darwin_%28operating_system%29), which is AFAIK a fork of the BSD system.
(In reply to comment #2)
> I don't think we have a list somewhere which shows all the supported platforms.
> It's really something we should come up with. At the end this list should be
> identical with the list of Firefox.
> 
> Yes we support Solaris and it needs its own conditions because it's not
> identified as Linux. The basis code of OS X is Darwin
> (http://en.wikipedia.org/wiki/Darwin_%28operating_system%29), which is AFAIK a
> fork of the BSD system.

Does this mean that Solaris is a supported platform for Firefox?  Is there any case where we distinguish between linux support and solaris support?
I gave you the link already last week. See bug 543492.
So(In reply to comment #4)
> I gave you the link already last week. See bug 543492.

So there are two things:  if the list is to be identical with the Firefox supported platforms, the Solaris is not there:

https://wiki.mozilla.org/Firefox/4/Platforms

So either we don't want an identical list, as per bug 543492, or we don't need to support Solaris.

This also still doesn't really tell me if we should treat Solaris differently than linux or not.  The real question is, for platforms is it

- windows 
- mac
- unix

or 

- windows
- mac
- linux
- solaris
- ...

Unless we specialize any code for solaris, it seems silly that it should be its own platform.
Solaris are contributed builds, which means those are not build by ourselves. We officially host those builds here:

ftp://ftp.mozilla.org/pub/firefox/releases/4.0b6/contrib/

We had to add this code to Mozmill because Solaris doesn't identify as Linux and I don't know what else could behave differently than Linux.
a suggestion on how mozinfo can/should work.  Note that it is one file and the interesting variables are module constants (calculated once).  We can add more stuff as needed
Attachment #501487 - Flags: review?
Not really knowing what to do, i added the following keys (see https://github.com/k0s/mozmill/blob/bug-606524/mozinfo/mozinfo.py#L45 ):

os: linux
hostname: jhammel-THINK
version: Ubuntu 10.10
bits: 32
processor: x86

Our existing report mechanism uses: "system_info": {"hostname": "jhammel-THINK", "service_pack": "", "system": "Linux", "version": "Ubuntu 10.10", "bits": "32", "processor": "x86"}

In the report.py module, I wrap this appropriately...except for the system values.  I went with 'linux', 'mac', and 'win', as these seem to be the most common usage at Mozilla.  Not sure what they should be for reporting
Attachment #501487 - Flags: review? → review?(ctalbert)
(In reply to comment #8)
> hostname: jhammel-THINK

A question we should answer ourselves is, do we really expose the hostname? It's private data which probably would require a privacy policy. I would say that we simply drop it, because it really hasn't such a big value.

> In the report.py module, I wrap this appropriately...except for the system
> values.  I went with 'linux', 'mac', and 'win', as these seem to be the most
> common usage at Mozilla.  Not sure what they should be for reporting

Originally I was looking at crash stats when I have implemented the naming of the several platforms. See as example:

http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=js%3A%3Amjit%3A%3AEnterMethodJIT%28JSContext*%2C%20JSStackFrame*%2C%20void*%2C%20js%3A%3AValue*%29&version=Firefox%3A4.0b8

Why is 'linux', 'mac', and 'win' common usage at Mozilla? Which systems are using those values?

>+- version : operating system version string

What does it mean for Windows? Are we still using numbers or names (XP, Vista, ...)?

>+# TODO: it might be a good idea of adding a system name (e.g. 'Ubuntu' for
>+# linux) to the information; I certainly wouldn't want anyone parsing this
>+# information and having behaviour depend on it

Hasn't that already be done?

>+class unknown(object):
>+    """marker class for unknown information"""
>+    def __int__(self):
>+        return 0
>+    def __str__(self):
>+        return 'UNKNOWN'

What do you think about 'n/a' instead of 'UNKNOWN'? 

>+if system in ["Microsoft", "Windows"]:
>+    info['os'] = 'win'

I would be in favor to not use any short names for the platform name.

>+        processor = os.environ.get('PROCESSOR_ARCHITECTURE', processor)
>+        system = os.environ.get("OS", system).replace('_', ' ')

Oh wait. We handle os and sytem differently? Why?

>+elif system == "Darwin":
>+    (release, versioninfo, machine) = platform.mac_ver()
>+    version = "OS X " + release
>+    info['os'] = 'mac'

You should also update system. No-one of our users will know what Darwin means. Otherwise it would be another point for me to update the system info.

>+choices = {'os': ['linux', 'win', 'mac', 'unix'],

Is unix still a valid os? wow!

>+    report['system_info'] = {'hostname': mozinfo.hostname,
>+                             'system': mozinfo.os,

This will now use the short names for the OS. Can we please keep the old logic with the full names? I would really appreciate that.
Assignee: nobody → jhammel
Status: NEW → ASSIGNED
Comment on attachment 501487 [details] [diff] [review]
initial implementation of mozinfo

I like the approach.  It doesn't really change any code and it makes it easier edit in the future.  I do have a few changes I want to see.

* Keep the mozinfo.os = ['win'|'mac'|'linux'] idiom you've got going.  That's consistent with the internals of our code in other places.
* Add in the full information on the OS, for reporting we really want to be able to get out the whole Windows NT 6.11 AMD64 string (or be able to construct it from its various parts).  So this mozinfo will need a set of attributes to use specifically for reporting where we have the full names and versions of the operating environments.  Be sure the namespace the reporting stuff properly, something like: mozinfo.Report['osfullname'] would return Microsoft Windows or whatever.
* I like being able to request the entire data structure from mozinfo for reporting purposes.  So creating some kind of dict of lists or something would be useful then you could do a mozinfo.getReportingInfo() and it would hand the caller the entire body of reporting information that mozinfo has acquired.
* I don't have a problem with hostname being in the reporting information.  It's useless outside of the same network ususally, and whether or not it is truly "reported" to the world is a presentation layer problem and not a "information" layer problem.  That data is always available to any program running on the box.  So if Henrik doesn't want to report it, then that is his decision.  Whether we report it or not in the default mozmill reporter is another question, but either way we should pick up that information in mozinfo.  Especially if we want to build a pan-harness information tool that is broader than mozmill's needs.
* While I agree with Henrik that 'n/a' is better for presentation layer reporting, but this is not presentation layer reporting.  This is basic "Here is the information I can acquire for you" which will be consumed by another program.  And when talking between programs it's best to keep it simple.  I'd actually prefer using None instead of "UNKNOWN" simply to make it easier for consumers to write code around this API, then they could do:
if mozinfo.GetThatInfo():
  dosomething
else
  # no information got to do something else

Right now, they'd have to know that "UNKNOWN" is returned when it can't find info and write:
if mozinfo.GetThatInfo() == "UNKNOWN":
  dosomething
else:
  dosomethingelse
And any time someone has to know a magic value to use an API, the API has failed.

So, it's a great start, still needs some work and a better documented API.  The methods and attributes that are exposed should be properly exposed with good names and the code needs to be far better documented if we really want to make something consumable.  I'd also like to see some tests with it so that it is clear how people can use this API.
Attachment #501487 - Flags: review?(ctalbert) → review-
(In reply to comment #10)
> (In reply to comment #8)
> > hostname: jhammel-THINK
> 
> A question we should answer ourselves is, do we really expose the hostname?
> It's private data which probably would require a privacy policy. I would say
> that we simply drop it, because it really hasn't such a big value.

I'm in favor of that;  the only reason I included it was because it was in the existing report code.

> > In the report.py module, I wrap this appropriately...except for the system
> > values.  I went with 'linux', 'mac', and 'win', as these seem to be the most
> > common usage at Mozilla.  Not sure what they should be for reporting
> 

<snip/> 
> Why is 'linux', 'mac', and 'win' common usage at Mozilla? Which systems are
> using those values?

I guess the point is that there isnt convergence.  In our public-facing builds, we use http://stage.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ linux, macosx, and win (ignoring the bits, which mozinfo records separately).  I'm find calling these $(whatever) as long as we can get agreement without a long discussion about fairly arbitrary labels.

> >+- version : operating system version string
> 
> What does it mean for Windows? Are we still using numbers or names (XP, Vista,
> ...)?

That I don't know.  I haven't run it on windows.  That said, we can put whatever we want here.  You can run the script on windows and see what it gets and if that information is preserved.  On linux it is Ubuntu 10.10 on my machine.

> Hasn't that already be done?

I mean adding e.g. 'XP' as the system name and then the version being the full string.  To put it another way, if something has to be done one way on XP and another way on another windows platform, I wouldn't want people pulling 'XP' out of a full version string.  But, as I said, I'm not sure how this displays on windows.
 
> >+class unknown(object):
> >+    """marker class for unknown information"""
> >+    def __int__(self):
> >+        return 0
> >+    def __str__(self):
> >+        return 'UNKNOWN'
> 
> What do you think about 'n/a' instead of 'UNKNOWN'? 

For the string?  I don't care

> >+if system in ["Microsoft", "Windows"]:
> >+    info['os'] = 'win'
> 
> I would be in favor to not use any short names for the platform name.

For any reason? Again, as long as a proposal for os names can be agreed on, i don't really care if its 'win', 'windows', 'mac', 'macosx', etc.  We just need to have a convention and stick by it so that we don't have to change code for a namespace change.

> >+        processor = os.environ.get('PROCESSOR_ARCHITECTURE', processor)
> >+        system = os.environ.get("OS", system).replace('_', ' ')
> 
> Oh wait. We handle os and sytem differently? Why?

system is not exported.  It is only used internally.
 
> You should also update system. No-one of our users will know what Darwin means.

Again, system is not exported to users

> Otherwise it would be another point for me to update the system info.
> 
> >+choices = {'os': ['linux', 'win', 'mac', 'unix'],
> 
> Is unix still a valid os? wow!

Yep.  We support solaris, or at least we check for it in mozmill, so 'unix' == 'non linux and non-mac posix'.  I'd prefer dropping it entirely.

> >+    report['system_info'] = {'hostname': mozinfo.hostname,
> >+                             'system': mozinfo.os,
> 
> This will now use the short names for the OS. Can we please keep the old logic
> with the full names? I would really appreciate that.

Not sure what the question is asking.  I suppose interested parties should decide what the strings should be.  I really don't care.
(In reply to comment #11)
> Comment on attachment 501487 [details] [diff] [review]
> initial implementation of mozinfo
> 
> I like the approach.  It doesn't really change any code and it makes it easier
> edit in the future.  I do have a few changes I want to see.
> 
> * Keep the mozinfo.os = ['win'|'mac'|'linux'] idiom you've got going.  That's
> consistent with the internals of our code in other places.

See :whimboo's comment above.  Again, I don't care what the strings are as long as they're canonical.

> * Add in the full information on the OS, for reporting we really want to be
> able to get out the whole Windows NT 6.11 AMD64 string (or be able to construct
> it from its various parts).  So this mozinfo will need a set of attributes to
> use specifically for reporting where we have the full names and versions of the
> operating environments.  Be sure the namespace the reporting stuff properly,
> something like: mozinfo.Report['osfullname'] would return Microsoft Windows or
> whatever.

Can this be in mozinfo.version?  Or do you want a new key, e.g. mozinfo.system.

> * I like being able to request the entire data structure from mozinfo for
> reporting purposes.  So creating some kind of dict of lists or something would
> be useful then you could do a mozinfo.getReportingInfo() and it would hand the
> caller the entire body of reporting information that mozinfo has acquired.

Already available: mozinfo.info:

>>> import mozinfo
>>> mozinfo.info
{'os': 'linux', 'hostname': 'jhammel-THINK', 'version': 'Ubuntu 10.10', 'bits': 32, 'processor': 'x86'}

> * I don't have a problem with hostname being in the reporting information. 
> It's useless outside of the same network ususally, and whether or not it is
> truly "reported" to the world is a presentation layer problem and not a
> "information" layer problem.  That data is always available to any program
> running on the box.  So if Henrik doesn't want to report it, then that is his
> decision.  Whether we report it or not in the default mozmill reporter is
> another question, but either way we should pick up that information in mozinfo.
>  Especially if we want to build a pan-harness information tool that is broader
> than mozmill's needs.
> * While I agree with Henrik that 'n/a' is better for presentation layer
> reporting, but this is not presentation layer reporting.  This is basic "Here
> is the information I can acquire for you" which will be consumed by another
> program.  And when talking between programs it's best to keep it simple.  I'd
> actually prefer using None instead of "UNKNOWN" simply to make it easier for
> consumers to write code around this API, then they could do:
> if mozinfo.GetThatInfo():
>   dosomething
> else
>   # no information got to do something else
> 
> Right now, they'd have to know that "UNKNOWN" is returned when it can't find
> info and write:
> if mozinfo.GetThatInfo() == "UNKNOWN":
>   dosomething
> else:
>   dosomethingelse
> And any time someone has to know a magic value to use an API, the API has
> failed.

Actually, this isn't true.  "UNKNOWN" is just the string representation.  When I upload the unbitrotted patch, the unknown singleton has def __nonzero__(): return False, which is python's very obfuscated way of making the object untrue

> So, it's a great start, still needs some work and a better documented API.  The
> methods and attributes that are exposed should be properly exposed with good
> names and the code needs to be far better documented if we really want to make
> something consumable.  I'd also like to see some tests with it so that it is
> clear how people can use this API.

I'll work on some documentation and tests.  Will upload the unbitrotted patch first
Attached patch unbitrot patch (obsolete) — Splinter Review
Attachment #501487 - Attachment is obsolete: true
(In reply to comment #12)
> I guess the point is that there isnt convergence.  In our public-facing builds,
> we use http://stage.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/
> linux, macosx, and win (ignoring the bits, which mozinfo records separately). 

Please don't use the tinderbox folder as reference for "public facing" builds. That's for developers or heavy testers but not that public like our folders for releases and even nightly builds. Here is what we should support at least:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/

> > >+- version : operating system version string
> > 
> > What does it mean for Windows? Are we still using numbers or names (XP, Vista,
> > ...)?
> 
> That I don't know.  I haven't run it on windows.  That said, we can put
> whatever we want here.  You can run the script on windows and see what it gets
> and if that information is preserved.  On linux it is Ubuntu 10.10 on my
> machine.

So you haven't changed this specific code? If that's the case, we are fine. Otherwise once you make changes you should really test the output on all the major platforms.

> > >+        processor = os.environ.get('PROCESSOR_ARCHITECTURE', processor)
> > >+        system = os.environ.get("OS", system).replace('_', ' ')
> > 
> > Oh wait. We handle os and sytem differently? Why?
> 
> system is not exported.  It is only used internally.

We should export it because that has the information we want. Please export system. See also Clints reply.

> Yep.  We support solaris, or at least we check for it in mozmill, so 'unix' ==
> 'non linux and non-mac posix'.  I'd prefer dropping it entirely.

We can't drop support for it because as already said in another bug, this is a still supported contribute build. And we have people running Mozmill tests.

> > >+    report['system_info'] = {'hostname': mozinfo.hostname,
> > >+                             'system': mozinfo.os,
> > 
> > This will now use the short names for the OS. Can we please keep the old logic
> > with the full names? I would really appreciate that.
> 
> Not sure what the question is asking.  I suppose interested parties should
> decide what the strings should be.  I really don't care.

Exactly but those parties can't decide on it later when you strip important information from the platform name that early and even don't export the platform name anymore. Please don't force it. I agree with Clint here.

> > * I don't have a problem with hostname being in the reporting information. 
> > It's useless outside of the same network ususally, and whether or not it is
> > truly "reported" to the world is a presentation layer problem and not a
> > "information" layer problem.  That data is always available to any program
> > running on the box.  So if Henrik doesn't want to report it, then that is his
> > decision.  Whether we report it or not in the default mozmill reporter is
> > another question, but either way we should pick up that information in mozinfo.

It more or less contains privacy information, which probably should force us to get the acceptance of the user. Have in mind that some routers set the current external IP address as part of the hostname, i.e. xx-xx-xx-xx-dynip.superkabel.de! So if you still want to send this information along with the report we need a privacy agreement from the user.

> > actually prefer using None instead of "UNKNOWN" simply to make it easier for
> > consumers to write code around this API, then they could do:
> > if mozinfo.GetThatInfo():
> >   dosomething
> > else
> >   # no information got to do something else

Does that correlate to undefined in Javascript? I would be totally up for it. Excellent idea. 

> Actually, this isn't true.  "UNKNOWN" is just the string representation.  When
> I upload the unbitrotted patch, the unknown singleton has def __nonzero__():
> return False, which is python's very obfuscated way of making the object untrue

So what happens if there would be a flag which in some cases cannot be retrieved. A setting which could have a false value I wouldn't be able to differentiate with one that isn't available at all?
(In reply to comment #15)

> Please don't use the tinderbox folder as reference for "public facing" builds.
> That's for developers or heavy testers but not that public like our folders for
> releases and even nightly builds. Here is what we should support at least:
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/

Still seeing 'win', 'mac', and 'linux'.
 
> > > >+- version : operating system version string
> > > 
> > > What does it mean for Windows? Are we still using numbers or names (XP, Vista,
> > > ...)?
> > 
> > That I don't know.  I haven't run it on windows.  That said, we can put
> > whatever we want here.  You can run the script on windows and see what it gets
> > and if that information is preserved.  On linux it is Ubuntu 10.10 on my
> > machine.
> 
> So you haven't changed this specific code? If that's the case, we are fine.
> Otherwise once you make changes you should really test the output on all the
> major platforms.

I have tried to port the code as faithfully as possible.  That doesn't necessarily mean I have succeeded.

> > system is not exported.  It is only used internally.
> 
> We should export it because that has the information we want. Please export
> system. See also Clints reply.

So, what is "sytem" other than one of the previous keys?

> > > >+    report['system_info'] = {'hostname': mozinfo.hostname,
> > > >+                             'system': mozinfo.os,
> > > 
> > > This will now use the short names for the OS. Can we please keep the old logic
> > > with the full names? I would really appreciate that.
> > 
> > Not sure what the question is asking.  I suppose interested parties should
> > decide what the strings should be.  I really don't care.
> 
> Exactly but those parties can't decide on it later when you strip important
> information from the platform name that early and even don't export the
> platform name anymore. Please don't force it. I agree with Clint here.

I'm not really sure what is being agreed on or disagreed on.  I will upload a new version with documents, but it sounds like someone other than me should spec this.  What is needed:

 - what keys do we want?  I have os, hostname, version, bits, and processor
 - what do they mean?  Respectively, I have "string identifier for the os", "short name of the host", "full version string of the OS", "number of bits for the processor", "processor family"
 - what choices are there.  I have os="linux, win, mac, and unix", bits=32,64, processor="x86, x86_64, ppc"

I have tried to follow mozilla conventions as closely as I could determine, though A. I am undoubtedly not familiar with all of the conventions; and B. we often use several different conventions for the same thing.
These can of course expand over time, but we should be reticent to change them, since the whole point of having a convention is to avoid changing consumer code.

If someone can spec it, I can probably program it.  However, I will guess this won't be revisited in an expedient manner (read: I doubt anyone will get around to it in 2011).

> > > * I don't have a problem with hostname being in the reporting information. 
> > > It's useless outside of the same network ususally, and whether or not it is
> > > truly "reported" to the world is a presentation layer problem and not a
> > > "information" layer problem.  That data is always available to any program
> > > running on the box.  So if Henrik doesn't want to report it, then that is his
> > > decision.  Whether we report it or not in the default mozmill reporter is
> > > another question, but either way we should pick up that information in mozinfo.
> 
> It more or less contains privacy information, which probably should force us to
> get the acceptance of the user. Have in mind that some routers set the current
> external IP address as part of the hostname, i.e.
> xx-xx-xx-xx-dynip.superkabel.de! So if you still want to send this information
> along with the report we need a privacy agreement from the user.

A. I don't care whether we send the data or not.  Currently we do.  I duplicate this behaviour.  If we want to reconsider this, it is a one line change.

B. This has to do with reporting and has nothing to do with the mozinfo module.  Mozinfo gathers data.  If there is a reason we don't want the hostname to be gathered, that is fine.  But gathering it has nothing to do with sending it.
 
 
> So what happens if there would be a flag which in some cases cannot be
> retrieved. A setting which could have a false value I wouldn't be able to
> differentiate with one that isn't available at all?

All current values are strings, or one integer case for bits.  I advise we reconsider this if/when it becomes a problem.  In any case, since unknown is a singleton, one can do:

if mozinfo.os is unknown:
   ...
Attached patch add a READMESplinter Review
I've added a README briefly detailing the state of things.  However, since we can't agree on spec this is mostly for posterity
Attachment #503360 - Attachment is obsolete: true
(In reply to comment #16)
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/
> 
> Still seeing 'win', 'mac', and 'linux'.

This is absolutely fine for .os but as Clint already told we also have to include .system in the report. For the real values see:

http://mozmill-release.brasstacks.mozilla.com/#/general/reports

>  Mozinfo gathers data.  If there is a reason we don't want the hostname to be
> gathered, that is fine.  But gathering it has nothing to do with sending it.

Agreed on. We could move this out to a separate bug. But I would like to hear from Clint first.

> All current values are strings, or one integer case for bits.  I advise we
> reconsider this if/when it becomes a problem.  In any case, since unknown is a
> singleton, one can do:
> 
> if mozinfo.os is unknown:

But how does it look like in a native JSON object? I wonder how I have to work with it in JS.
Assignee: jhammel → nobody
(In reply to comment #18)
> (In reply to comment #16)
> > > http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/
> > 
> > Still seeing 'win', 'mac', and 'linux'.
> 
> This is absolutely fine for .os but as Clint already told we also have to
> include .system in the report. For the real values see:
> 
> http://mozmill-release.brasstacks.mozilla.com/#/general/reports

Again, waiting for spec.

> > All current values are strings, or one integer case for bits.  I advise we
> > reconsider this if/when it becomes a problem.  In any case, since unknown is a
> > singleton, one can do:
> > 
> > if mozinfo.os is unknown:
> 
> But how does it look like in a native JSON object? I wonder how I have to work
> with it in JS.

You mean, for the reporting module?  That is up to the reporting module.
Here's the spec you wanted.  I believe it solves both your concerns.  Also, your readme refers to "mozmill" in several places you intend it to say "mozinfo".
Assignee: nobody → jhammel
Status: ASSIGNED → NEW
Maybe the thing to do here is to split this bug into two patches:

1. the new MozInfo module
2. the modifications to MozMill (MozRunner, MozProcess, etc) that depend on MozInfo

This should make it easier to review and unbitrot
Whiteboard: [mozmill-2.0?]
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
Assignee: jhammel → nobody
Component: Mozmill → Mozmill Utilities
QA Contact: mozmill → mozmill-utilities
Assignee: nobody → jhammel
Going to (re)start on this bug soon. I'm going to split this into two patches: one for the new package and a second to use it in mozmill.  This way it both avoids bitrot and is easier to use.
Attached patch just the MozInfo package (obsolete) — Splinter Review
separating MozInfo from its usage by mozmill and other utilities in the mozmill repo.  We'll review this first and check it in so that we eliminate the moving target aspect of this bug
For reference this is currently what's in the mozinfo.info on my computer (WIP):

> mozinfo 
os: linux
hostname: system76-pc
osfullname: Linux
version: Ubuntu 10.10
bits: 64
processor: x86_64

I'm going to post some follow-up questions wrt the spec.  Its important to remember that while there is the technical component of the bug (actually programming mozinfo), the substantive part of this bug is deciding what we actually want/need in a (python) Mozilla information package. So, I'm happy to program anything and have some opinions on the architecture of mozinfo.py (structurally, I like how it is now and can explain why if anyone disagrees), I have at most a weak opinion on what information we report or whether e.g. we a key called 'osfullname' vs 'system', etc) so we should make sure what we report is what we want.  We can always add to it layer, but since the purpose of this layer is to unify convention and do all of these sorts of checking in one place, we should avoid changing the variable names as this will always for a complete downstream refactor.
Question:

    ** {'os_name': 'windows', 'isWin': true, 'isLinux': false, 
...
    readonly boolean is_win;

Do we want 'is_win' or 'isWin'? In https://github.com/k0s/mozmill/tree/mozinfo2, I have implemented e.g. isWin. The patch is written traditional OO style, versus python that doesn't really have private attributes (yeah, __foo, but not applicable here). I guess we're getting rid of the short codes (win, mac, linux) entirely outside of the 'is' checks?
Along the lines of comment 26:

    ** {'os_name': 'windows', 'isWin': true, 'isLinux': false, 'arch': 'x86_64'...}
...
    readonly string architecture;

Arch or architecture?
Also (see comment 26) is there any reason we use the "full" name for os_name and only the abbreviation for (e.g.) isWin? I would prefer tighter integration
Why do we have isWin etc when we already have os_name? Seems like duplication.
(In reply to comment #29)
> Why do we have isWin etc when we already have os_name? Seems like duplication.

presumedly its a shortcut so you can do

if mozinfo.isWindows:
  ...

vs.

if mozinfo.os_name == 'Windows':
  ...

That said, I'm a bit confused about what I'm doing now, which is implementing the spec.  If this isn't something we can get buy-in on ... shrug.  I don't know...
(In reply to comment #28)
> Also (see comment 26) is there any reason we use the "full" name for os_name
> and only the abbreviation for (e.g.) isWin? I would prefer tighter integration

Assuming the choices are ['Linux', 'Windows', 'MacOSX', 'Unix'] (previously, ['linux', 'win', 'mac', 'unix']), I'm going to go with isLinux, isWindows, isMacOSX, isUnix for consistency.  I see no reason we should have two different conventions in the same file, unless someone can convince me that its a good idea.
Yeah, I, uh, didn't read the spec. I should do that.
(In reply to comment #32)
> Yeah, I, uh, didn't read the spec. I should do that.

Comments greatly appreciated :) I'm not sure what I think of it, to be honest.  For my purposes (a programmer who wants an API) mostly I need two things:

- a unified way to see if I'm on windows, linux, or mac (or ::shudder:: "other")
- and occassionally a way to know if I'm on a 64 or 32 bit machine

And again, my main motivation here is to do these checks in one place instead of the many different ways its done in e.g. mozmill.  As for the rest of it, its nice to have and I understand putting it here, but to me its just more things to debate at this point.
I'm going to back away from this bug as I'm not really sure what I'm doing here.  Except all of the ways that the spec is vague, there isn't really any technical barrier here.  I just am not sure what problem we're solving anymore. If people are happy and excited about the spec, I am happy to implement (FWIW, I am neither happy nor excited about the spec). However, I don't want to put a lot of time changing APIs and strings around for something that people won't want to use.

Maybe we should take a step back and reconsider the problem.  This is marked for Mozmill 2 but honestly I don't know how I feel about this.  Also, this seems to me one of those bugs that really benefits for a lot of people looking at it and working on it.  Right now I have to ask myself: am I crazy not to like the spec?  Would other people intuitively want os_name vs os? Etc.  So I'm not sold.

My current code is at https://github.com/k0s/mozmill/tree/mozinfo2
Blocks: 628279
(In reply to comment #34)
> I'm going to back away from this bug as I'm not really sure what I'm doing
> here.  Except all of the ways that the spec is vague, there isn't really any
> technical barrier here.  I just am not sure what problem we're solving anymore.
> If people are happy and excited about the spec, I am happy to implement (FWIW,
> I am neither happy nor excited about the spec). However, I don't want to put a
> lot of time changing APIs and strings around for something that people won't
> want to use.

The idea here is that if you have an API that says <foo>.isWindows then you don't have to guess what the os name is: i.e. is it:
* if <foo>.os_name == 'win32' OR
* if <foo>.os_name == 'windows' OR
* if <foo>.os_name == 'win'
* if <foo>.os_name == 'windows64'
If you have just one api that returns a true/false you don't need to do this guess and it makes the api a lot more deterministic.

> Maybe we should take a step back and reconsider the problem.  This is marked
> for Mozmill 2 but honestly I don't know how I feel about this.  Also, this
> seems to me one of those bugs that really benefits for a lot of people looking
> at it and working on it.  Right now I have to ask myself: am I crazy not to
> like the spec?  Would other people intuitively want os_name vs os? Etc.  So I'm
> not sold.
I don't understand why this is a deal breaker on the functionality and is worth throwing up one's hands at.  But I'm all for getting the spec right.  This definitely isn't in the critical path for 2.0, it's just a nice to have.  So, if we take the time to get the spec right then we implement it for 2.1 or whatever, that will be good.

I wrote the spec by looking at all the ways we do this "get system information" currently in our 7 test harnesses and rationalized that down to a sane API.  It could probably use more thought and could certainly use more feedback.
Returning here.  I'm not sure if we can generate consensus on e.g. keynames, but....someone has to! I ported https://bugzilla.mozilla.org/attachment.cgi?id=521269 to its own (hopefully temporary) repository at http://k0s.org/mozilla/hg/mozinfo/ which will be easy to develop on.  While this is scheduled to land on the mozmill repository, there will be other consumers too and it has no dependencies nor will.  

Feel free to play with it.  Ping jhammel in #mozmill or #ateam for push requests or questions.
I have an improved version up at 

http://k0s.org/mozilla/hg/mozinfo/file/2a5b1d3efb38

- I've removed the hostname; we don't need it here and its easy to get
- I've added a command to update the info dict and globals from an arbitrary dict so that detected configuration may be overridden. You can load these from the command line or programmatically
- this is tested on my linux box.  Its a straight port of code in the wild (mozmill.info : https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/info.py), so in theory it should be pretty good.  But any manual testing we could do on other platforms would be great.
We had a discussion on IRC and everybody liked this implementation. I'm importing it into mozilla-central in the patch in bug 664197.
So, I'm trying to figure out what to do here.  My up-front recommendation is that we take what we have and land it in the mozmill repository either as is or with minor changes.  Read further to see why I recommend this.  Please read before you comment.

This has been an outstanding bug for a long time.  I've tried to develop a good straight-forward architecture that is fairly easily extended. I think I've done that.  What is in here is a superset of what is in mozmill.info currently but in a generic and consumable package.  Having this will either avoid bugs like bug 664564 entirely or make fixing it much easier.

Mostly, ABICT, what is contraversial is the name of the strings: "Windows" vs "win" vs "windows", "os" vs "os_name" etc. I had not implemented :ctalbert's spec at the time of comment 38 . If we do want to change anything now, writemozinfo.py will have to change, so we already have an upstream dependency.  

The advantage of this bug is to have a unified way of dealing with system information. In otherwords, checks like

 if sys.platform.startswith('linux') or sys.platform in ('solaris', 'sunos'):

can be reduced to

 if mozinfo.isUnix:

which leads to more declarative code and fixing how to fetch the e.g. operating system (ibid).

The disadvantage of this approach is that once the convention is coded and utilized, e.g. 'win', then all upstream dependencies, which we currently have no way of tracking, must be updated to use the new convention.  This is annoying, time-consuming, and hard.

Personally, I don't particularly care what the key, values are as long as there is some easy way, e.g. `python mozinfo.py --os`, to discover what the values are. I am inclined, since http://mxr.mozilla.org/mozilla-central/source/config/writemozinfo.py already uses the conventions currently in http://k0s.org/mozilla/hg/mozinfo/ :

(mozmill)│python mozinfo.py
version: Ubuntu 11.04
os: linux
bits: 32
processor: x86

On one hand, if we're not happy with these conventions, its much easier to change them now, when we only have one upstream dep (to my knowledge).  But on the other hand, again, I think what we have is fine and I'd rather move forward rather than keep nit-picking here.  Or to put another way, if we are going to go to the trouble of changing how things work, I want a precise way of bringing this discussion to an end.  8+ months is way too long for a turnaround here.
Noting also that while mozinfo is listed here as a mozmill package, that really isn't true. It is a new piece of software that happens to be used by mozmill. It will be used elsewhere too as I will soon outline, though its a bit of a chicken+egg problem wrt it depending on itself (or, more technically, in order to make it worthwhile documenting how all of the pieces [mozinfo, manifestparser, writemozinfo] fit together, mozinfo has to be in some sort of spec-able state)
Attached patch add Mozinfo to mozmill repo (obsolete) — Splinter Review
Attachment #521269 - Attachment is obsolete: true
Attachment #540905 - Flags: review?(ctalbert)
Attachment #540905 - Flags: feedback?(ted.mielczarek)
Attachment #540905 - Flags: feedback?(ted.mielczarek) → feedback+
Attachment #540905 - Attachment is obsolete: true
Attachment #542959 - Flags: review?(ctalbert)
Attachment #540905 - Flags: review?(ctalbert)
Comment on attachment 542959 [details] [diff] [review]
add Mozinfo to mozmill repo

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

r+, just some little nits

::: mozinfo/mozinfo.py
@@ +93,5 @@
> +        service_pack = os.sys.getwindowsversion()[4]
> +        info['service_pack'] = service_pack
> +elif system == "Linux":
> +    (distro, version, codename) = platform.dist()
> +    version = distro + " " + version

this will break if platform.dist ever returns None instead of '', so just use "%s %s" % distro, version style.

@@ +99,5 @@
> +        processor = machine
> +    info['os'] = 'linux'
> +elif system == "Darwin":
> +    (release, versioninfo, machine) = platform.mac_ver()
> +    version = "OS X " + release

%s style

::: mozinfo/setup.py
@@ +6,5 @@
> +    description = file(os.path.join(here, 'README.txt')).read()
> +except IOError:
> +    description = None
> +
> +version = '0.2.1'

This really hasn't ever been released should it have 0.2.1 version?  Why not 0.3?
Attachment #542959 - Flags: review?(ctalbert) → review+
::: mozinfo/setup.py
@@ +6,5 @@
> +    description = file(os.path.join(here, 'README.txt')).read()
> +except IOError:
> +    description = None
> +
> +version = '0.2.1'

> This really hasn't ever been released should it have 0.2.1 version?  Why not 0.3?

Can certainly bump to 0.3 (or whatever) when I land.

Will make the other changes too (as said, this is pretty much a straight port of what is in mozmill.info now, with a few extras)
pushed to master: https://github.com/mozautomation/mozmill/commit/fa0a9acc86176c97433180aae6ea8085d01e01ef

Now, part 2 of this bug, actually make mozmill use it (TODO)
Blocks: 664564
So this is almost ported. I will post a patch after this.

The only unresolved mystery is what to do about what the (default) report calls "system" for windows: https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/info.py#L57 info.py is going away for mozinfo.  This is fine except for this guy.

Originally, I thought this was a mapping from the windows version numbers:
http://www.nirmaltv.com/2009/08/17/windows-os-version-numbers/

However, looking at the brasstacks dashboard, the only instance I see is 'Windows NT'.  Should the report just send "Windows NT" if mozinfo.isWin ?  Or is there a case where this is bad?

I don't really understand what the code above is trying to do.  Does it always send Windows NT?

you can get (conceivably) "Microsoft", "Windows", or os.environ['OS'].replace('_', ' '). Judging by the dashboard, it is *always* the last case, and os.environ['OS'] is *always* "Windows_NT"

Let me know and I can use whatever string we want ;)  Or consumers can create their own report module.
Attached patch utilize mozinfoSplinter Review
use mozinfo throughout all of the packages in the mozmill repository. Since I had to go through all the python code, I fixed some nits while there.  Currently, linux and mac will be sent as "Linux" and "Mac", as appear on the brasstacks dashboard currently.  windows will be sent as "Win", unlike the "Windows NT" currently displayed on the dashboard.  This can be changed or you can tell me what it should be or a subclass of report can be used vs the master class.
Attachment #547831 - Flags: review?(fayearthur+bugs)
Attachment #547831 - Flags: feedback?(ctalbert)
Comment on attachment 547831 [details] [diff] [review]
utilize mozinfo

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

All these things look good.
Attachment #547831 - Flags: review?(fayearthur+bugs) → review+
pushed to master: https://github.com/mozautomation/mozmill/commit/1f95cb13a5546cb1f2647b0066fc27a543a18349
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #547831 - Flags: feedback?(ctalbert) → feedback+
Flags: needinfo?(robaslemaiy)
Flags: needinfo?(k0scist)
Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: