1066605 - [mozversion] Missing properties from ini files should not be added as 'None'

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I already talked with Dave about this yesterday. He wasn't sure if the solution as proposed is ok. So I checked out mozmill-automation source and found a good case why we should change the current behavior.

For example take the application.ini file for a default Firefox build on Ubuntu. It will miss certain properties like the source repository url. So application_repository will be None. To actually use it you will have to do an extra null check:

version_info = mozversion.get_version(self._application)
repo = version_info.get('application_repository')
if not repo:
    repo = 'default'
application.get_mozmill_tests_branch(repo)

With not having it as None present, only three lines of code would be necessary:

version_info = mozversion.get_version(self._application)
repo = version_info.get('application_repository', 'default')
application.get_mozmill_tests_branch(repo)

Now it would use 'default' automatically if the property is not present.

The only thing is that existing code hsa to be updated to use the .get() method instead of directly accessing the property via version_info['application_repository'].

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Yep, I agree this would be a good enhancement.

Comment 2

[Julien Pagès (:parkouss)]

Hi,

Looking at the code I don't see how mozversion can create automatically a key like 'application_repository' if there is not an according option/section. For example:

[App]
Repository =

will create a mozinfo key application_repository with value '' (not None), and:

[App]
OtherKey = Something

will not create the application_repository key at all - since there is no such key.

So, I think that what you really want here is to filter empty values (''), ie when there is an option with no value in the config file (but the option is present).

Is that right ? I will assume this for now and send you a patch. Tell me if there's something I misunderstood or if I took a wrong way. :)

Comment 3

[Julien Pagès (:parkouss)]

Attachment: Missing properties from ini files should not be added

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This isn't about filtering empty values, it's about excluding keys from the dictionary returned instead of having them with a value of None. I'm not sure what Henrik's scenario was, but one way you can replicate this is to run mozversion.get_info against a B2G desktop build:

>>> import pprint
>>> import mozversion
>>> pprint.pprint(mozversion.get_version(binary='/Applications/B2G.app/Contents/MacOS/b2g-bin'))
{'application_buildid': '20141016040204',
 'application_changeset': 'a280a03c9f3c',
 'application_display_name': 'B2G',
 'application_id': '{3c2e2abc-06d4-11e1-ac3b-374f68613e61}',
 'application_name': 'B2G',
 'application_repository': 'https://hg.mozilla.org/mozilla-central',
 'application_vendor': 'Mozilla',
 'application_version': '36.0a1',
 'gaia_changeset': None,
 'gaia_date': '1413459504',
 'platform_buildid': '20141016040204',
 'platform_changeset': 'a280a03c9f3c',
 'platform_repository': 'https://hg.mozilla.org/mozilla-central',
 'platform_version': '36.0a1'}

Note that 'gaia_changeset' has a value of None. What we want is to exclude 'gaia_changeset' from the dictionary entirely, so that the consumer can return a suitable default when the key does not exist.

Comment 5

[Julien Pagès (:parkouss)]

Ok Dave, thanks for the info.

But I'm not sure Henrik and you ask for the same thing.

The None thing you are talking (for 'gaia_changeset') can be found in the code because of this line: http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozversion/mozversion/mozversion.py#146.

However, I don't see how 'application_repository' could be done in the current code (see comment 0). For me it could just be an empty string, but evaluated as False by python in if statements, like Henrik shows in comment 0.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Julien Pagès from comment #5)
> The None thing you are talking (for 'gaia_changeset') can be found in the
> code because of this line:
> http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozversion/
> mozversion/mozversion.py#146.

That's correct.

> However, I don't see how 'application_repository' could be done in the
> current code (see comment 0). For me it could just be an empty string, but
> evaluated as False by python in if statements, like Henrik shows in comment
> 0.

We have the 'or None' also on this line: http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozversion/mozversion/mozversion.py#52 which would add the key with a value of None. Instead we want to avoid adding the key.

Perhaps needinfo Henrik if you have further questions regarding his use case, but he suggests that a default Firefox build on Ubuntu would be missing a number of these expected keys from the INI files.

Comment 7

[Julien Pagès (:parkouss)]

(In reply to Dave Hunt (:davehunt) from comment #6)
> (In reply to Julien Pagès from comment #5)

> > However, I don't see how 'application_repository' could be done in the
> > current code (see comment 0). For me it could just be an empty string, but
> > evaluated as False by python in if statements, like Henrik shows in comment
> > 0.
> 
> We have the 'or None' also on this line:
> http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozversion/
> mozversion/mozversion.py#52 which would add the key with a value of None.
> Instead we want to avoid adding the key.

We are in a loop "for key, value in config.items(section):" here.
I don't think that config.items could return a key with a None value, so I think that this 'or None' just replace empty string values with None - and that is what we don't want if I understand well.

To me, it is about filtering empty values here (in response of comment 4).

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Flagging Henrik for input.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes, so on Ubuntu we don't have the application and platform repository information. Instead of having those set to None, they should not be part of the generated dict.

Comment 10

[Julien Pagès (:parkouss)]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Yes, so on Ubuntu we don't have the application and platform repository
> information. Instead of having those set to None, they should not be part of
> the generated dict.

Can you look at your .ini files ? I think that application and platform repository information are defined in the .ini files, but empty - and so the problem here is to eventually filter empty information in .ini files.

Could you check that please ?

Thanks

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As said those keys do not exist! So here the listing of the App section of application.ini:

[App]
Vendor=Mozilla
Name=Firefox
Version=33.0
BuildID=20141013200257
ID={ec8030f7-c20a-464f-9b0e-13a3a9e97384}

Comment 12

[Julien Pagès (:parkouss)]

Ok, sorry for the disturbance.

Well I don't see how I can help here - I can not reproduce it. I'm on ubuntu too, my application.ini file is like this:

cat /usr/lib/firefox/application.ini
; This file is not used. If you modify it and want the application to use
; your modifications, move it under the browser/ subdirectory and start with
; the "-app /path/to/browser/application.ini" argument.
[App]
Vendor=Mozilla
Name=Firefox
Version=33.0
BuildID=20141013200257
ID={ec8030f7-c20a-464f-9b0e-13a3a9e97384}

[Gecko]
MinVersion=33.0
MaxVersion=33.0

[XRE]
EnableProfileMigrator=1
EnableExtensionManager=1

[Crash Reporter]
Enabled=1
ServerURL=https://crash-reports.mozilla.com/submit?id={ec8030f7-c20a-464f-9b0e-13a3a9e97384}&version=33.0&buildid=20141013200257

And if I run a script like this:

from mozversion import get_version
print get_version(binary='/usr/lib/firefox/firefox')

I got this output (there is NO None values...):

{'application_id': '{ec8030f7-c20a-464f-9b0e-13a3a9e97384}', 'application_buildid': '20141013200257', 'application_display_name': 'Firefox', 'platform_version': '33.0', 'application_name': 'Firefox', 'application_vendor': 'Mozilla', 'platform_buildid': '20141013200257', 'application_version': '33.0'}

But now, if I add in the application.ini file:

[App]
repository =

I got the None value now ->

{'platform_buildid': '20141013200257', 'application_id': '{ec8030f7-c20a-464f-9b0e-13a3a9e97384}', 'application_buildid': '20141013200257', 'application_display_name': 'Firefox', 'platform_version': '33.0', 'application_name': 'Firefox', 'application_vendor': 'Mozilla', 'application_repository': None, 'application_version': '33.0'}

I don't know what I can add to this. Well, maybe it is better if I don't work on this particular bug.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Interesting. Looks like I fixed that accidentally with my patch on bug 1088060! No wonder why we cannot replicate it anymore. :) Thank you for pointing this out Julien.

So this patch is already alive in the latest mozversion release on pypi!