If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[mozversion] Update retrieval of application information due to upcoming Mac signing changes

RESOLVED FIXED in mozilla35

Status

Testing
Mozbase
--
major
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Trunk
mozilla35
All
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

3 years ago
For the upcoming signing changes of OS X builds, we have to update mozversion for the location of the application.ini and platform.ini files. Those will no longer be able under Contents/MacOS/ but now under Resources/.

I will work on that tomorrow.
(Assignee)

Updated

3 years ago
Summary: [mozversion] → [mozversion] Update retrieval of application information due to upcoming Mac signing changes
(Assignee)

Comment 1

3 years ago
Andrew, I would like to get your feedback here. I checked the code of mozversion and I think that get_gecko_info() is the best place to make this change, or? Reason is that maybe also other applications are affected.

Stephen, with your changes I would assume that also the following applications will change?
* B2G desktop builds on OS X
* Firefox Mobile (aka Fennec) builds on OS X
Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(ahalberstadt)
I'm not that familiar with mozversion, I'll defer to Dave here.
Flags: needinfo?(ahalberstadt)
Flags: needinfo?(dave.hunt)
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Stephen, with your changes I would assume that also the following
> applications will change?
> * B2G desktop builds on OS X
> * Firefox Mobile (aka Fennec) builds on OS X

I honestly don't know. My focus is on regular OSX builds and we're scrambling to get those done in time. If there's someone with B2G desktop and Fennec experience, it would be great to evaluate the impact that this may have on those builds.
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Andrew, I would like to get your feedback here. I checked the code of
> mozversion and I think that get_gecko_info() is the best place to make this
> change, or? Reason is that maybe also other applications are affected.

I think I'd put it in all Local*Version classes that it applies to. If that's just desktop then it's easy enough to do.
Flags: needinfo?(dave.hunt)
(Assignee)

Updated

3 years ago
Blocks: 980801
(Assignee)

Comment 5

3 years ago
I checked a couple of applications and all have the same folder structure on OS X. So I assume builds like B2G desktop, XulRunner, etc. are also affected. I cannot find any build for Fennec Desktop, which has the class LocalFennecVersion. So I will not update that one for now.
Have you confirmed that those builds are currently being signed, or that they will need to be? If not, there won't be any need to change the folder structure.
(Assignee)

Comment 7

3 years ago
I see. Anyway we will have to keep a fallback mechanism. So both locations will have to work.
(Assignee)

Comment 8

3 years ago
Created attachment 8485006 [details] [diff] [review]
mac signing changes v1

With this patch we try to find the ini files in both locations on OS X.
Attachment #8485006 - Flags: review?(wlachance)
Comment on attachment 8485006 [details] [diff] [review]
mac signing changes v1

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

Looks great!
Attachment #8485006 - Flags: review?(wlachance) → review+
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3368efceb99f
Target Milestone: --- → mozilla35
(Assignee)

Updated

3 years ago
Blocks: 1064193
(Assignee)

Comment 11

3 years ago
I pushed that too early by actually not checking the existent mozversion tests. Given that we make use of hard-coded locations, we are failing.

Backed-out my patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/e11ab3c4dcd9
Target Milestone: mozilla35 → ---
(Assignee)

Comment 12

3 years ago
Actually my patch revealed an underlying problem with the tests, which caused wrong results so far. The ini files, which are getting created by the first two tests, are not removed. So later tests, which expect those files not to exist, are failing.
(Assignee)

Comment 13

3 years ago
Please disregard my last comment here. The problem lays indeed in the newly added code and how we determine if the ini files are present or not. My code always raises a LocalAppNotFound error whereby the old code in those cases just returned an empty dict.
(Assignee)

Comment 14

3 years ago
Just to wrap up here. The former code did a check for the binary if specified. If it doesn't exist, it throw a LocalAppNotFoundError. There is no case when it also checks for an existing application.ini file. It just assumes that such a file might exist in the same folder, given that a path has been specified as binary. This could let some broken calls slip through.

My updated code also checks for the binary specified, and raises an exception in case it doesn't exist. But it also checks for existent ini files in both cases.

So the question is how should this code behave in case of no ini files found. Personally I'm not that behind the idea to only return an empty dict. IMO raising an exception is better, given that if none of those exist the risk is high that the application is simply broken. Each Gecko based application needs to have those files.

Andrew and William, what do you both think? Dave seems to be out today.
Flags: needinfo?(wlachance)
Flags: needinfo?(ahalberstadt)
Yes, throwing an exception seems like the right approach, as it indicates that something is very wrong and we should not continue.
Flags: needinfo?(wlachance)
(Assignee)

Comment 16

3 years ago
Just to mention... in case of any Debian distribution mozversion will not work with the default installation of Firefox, and when specified via '--binary /usr/bin/firefox'. In such a case we should check for a symlink and follow that one appropriately. I will have a follow-up bug for this specific new feature.
(Assignee)

Comment 17

3 years ago
Created attachment 8486072 [details] [diff] [review]
mac signing changes v2

Additional stability fixes, so we can also ensure to get expected values returned.
Attachment #8485006 - Attachment is obsolete: true
Attachment #8486072 - Flags: review?(wlachance)
(Assignee)

Comment 18

3 years ago
Comment on attachment 8486072 [details] [diff] [review]
mac signing changes v2

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

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +61,3 @@
>  
> +        display_name = self._info.get('application_display_name')
> +        if not display_name and self._info.get('application_name'):

I have changed those lines to make sure that entries are only added, when they are really present.

@@ +107,2 @@
>          else:
> +            path = find_location(os.getcwd())

This doesn't change the current behavior and make me less nervous about possible regressions.

::: testing/mozbase/mozversion/tests/test_binary.py
@@ +44,5 @@
> +    @unittest.skipIf(not os.environ.get('BROWSER_PATH'),
> +                 'No binary has been specified.')
> +    def test_real_binary(self):
> +        v = get_version(os.environ.get('BROWSER_PATH'))
> +        self.assertNotEqual(v, {})

I think it's worth having a real test against the binary as built.
(Assignee)

Updated

3 years ago
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 19

3 years ago
I pushed the above patch to try: https://tbpl.mozilla.org/?tree=Try&rev=60e2ded81506
(Assignee)

Comment 20

3 years ago
Comment on attachment 8486072 [details] [diff] [review]
mac signing changes v2

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

Looks like the desktop builds are still not happy given that adb is not installed. The change to the default parameter value might have been wrong. I will revert this single change.
Attachment #8486072 - Flags: review?(wlachance)
(Assignee)

Updated

3 years ago
Blocks: 1064811
(Assignee)

Comment 21

3 years ago
The fallback case is still puzzling myself. I'm not sure how to handle that correctly. Meanwhile I think that any kind of refactoring here should be moved out to another bug. Therefor I have created bug 1064811. I will update the affected tests, so that they will pass for now and make a comment that changes have to be made.

In the above case we always fallback to the remote B2G version class, which then errors out with:

> raise Exception('Unknown device manager type: %s' % dm_type)

given that no device manager has been specified.
(Assignee)

Comment 22

3 years ago
Created attachment 8486402 [details] [diff] [review]
mac signing changes v2.1

Pushed my fixes for this revision to try and all seems to work fine this time:
https://tbpl.mozilla.org/?tree=Try&rev=e0d848e99d1b

Moving review over to Dave given that he is the original author.
Attachment #8486072 - Attachment is obsolete: true
Attachment #8486402 - Flags: review?(dave.hunt)
Comment on attachment 8486402 [details] [diff] [review]
mac signing changes v2.1

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

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +56,5 @@
>          for key in ('BuildID', 'Name', 'CodeName', 'Version',
>                      'SourceRepository', 'SourceStamp'):
>              name = name_map.get(key, key).lower()
> +            if config.has_option(section, key):
> +                self._info['%s_%s' % (type, name)] = config.get(section, key)

This will mean self._info doesn't contain keys when the section/key is missing. I don't think that's much of an issue but it could break downstream code.

@@ +61,3 @@
>  
> +        display_name = self._info.get('application_display_name')
> +        if not display_name and self._info.get('application_name'):

This change is only necessary because of the above change, which is an example of how this could affect downstream consumers. Are these changes needed?

@@ +91,5 @@
> +        def find_location(path):
> +            if os.path.exists(os.path.join(path, 'application.ini')):
> +                return path
> +
> +            if sys.platform == 'darwin':

Should we be using mozinfo here?

@@ +256,4 @@
>      :param dm_type: Device manager type. Must be 'adb' or 'sut' (Firefox OS)
>      :param host: Host address of remote Firefox OS instance (SUT)
>      :param device_serial: Serial identifier of Firefox OS device (ADB)
> +

Nit: Remove the blank line here. See http://legacy.python.org/dev/peps/pep-0257/#multi-line-docstrings

::: testing/mozbase/mozversion/tests/test_binary.py
@@ +41,5 @@
>          os.chdir(self.cwd)
>          mozfile.remove(self.tempdir)
>  
> +    @unittest.skipIf(not os.environ.get('BROWSER_PATH'),
> +                 'No binary has been specified.')

Nit: The indentation doesn't look right here, please check with PEP8.

@@ +72,5 @@
>      def test_missing_ini_files(self):
> +        # Bug 1064811 - Non-appropriate fallback causes an exception to be raised
> +        # v = get_version(self.binary)
> +        # self.assertEqual(v, {})
> +        self.assertRaises(Exception, get_version,

We should check the specific exception here. Wouldn't this be a LocalAppNotFoundError? Also, I'm not sure I understand how bug 1064811 applies here.

::: testing/mozbase/mozversion/tests/test_sources.py
@@ +57,4 @@
>          self._check_version(get_version())
>  
>      def test_invalid_sources_path(self):
> +        # Bug 1064811 - Non-appropriate fallback causes an exception to be raised

What exception is raised? Does this mean we're breaking support for B2G?
Attachment #8486402 - Flags: review?(dave.hunt) → review-
(Assignee)

Updated

3 years ago
No longer blocks: 1064811
Depends on: 1064811
(Assignee)

Comment 24

3 years ago
Created attachment 8487162 [details] [diff] [review]
mac signing changes v3

As discussed all above items are fixed, and the other necessary changes have been made. I will trigger a try build in a bit.
Attachment #8486402 - Attachment is obsolete: true
Attachment #8487162 - Flags: review?(dave.hunt)
(Assignee)

Comment 25

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d6f5acb0409c
(Assignee)

Comment 26

3 years ago
Created attachment 8487230 [details] [diff] [review]
mac signing changes v3.1

The last patch caused bustage on tbpl because I missed to add the new errors module to the patch. This patch fixes it.

https://tbpl.mozilla.org/?tree=Try&rev=39223d0c7016
Attachment #8487162 - Attachment is obsolete: true
Attachment #8487162 - Flags: review?(dave.hunt)
Attachment #8487230 - Flags: review?(dave.hunt)
Comment on attachment 8487230 [details] [diff] [review]
mac signing changes v3.1

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

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +96,4 @@
>  
>          if not path:
> +            raise errors.LocalAppNotFoundError('Application not found: %s' % \
> +                                               path)

I would update the LocalAppNotFoundError to accept a path and format the message in the Error class.

@@ +186,4 @@
>              dm = mozdevice.DeviceManagerSUT(host=host)
>          else:
> +            raise errors.RemoteAppNotFoundError('Unknown device manager type: %s' % \
> +                                         dm_type)

Nit: Fix indentation, and backslash is not required between ()

@@ +255,5 @@
>              if version._info.get('application_name') == 'B2G':
>                  version = LocalB2GVersion(binary, sources=sources)
> +    except errors.LocalAppNotFoundError:
> +        version = RemoteB2GVersion(sources=sources, dm_type=dm_type,
> +                                   host=host, device_serial=device_serial)

This could raise a RemoteAppNotFoundError even if the intention was to run against a local app. We could catch the RemoteAppNotFoundError and raise an AppNotFoundError so it's more generic.

::: testing/mozbase/mozversion/tests/test_binary.py
@@ +45,5 @@
> +    @unittest.skipIf(not os.environ.get('BROWSER_PATH'),
> +                     'No binary has been specified.')
> +    def test_real_binary(self):
> +        v = get_version(os.environ.get('BROWSER_PATH'))
> +        self.assertNotEqual(v, {})

I don't think this would detect if v was None, and may pass. Should we perhaps check the v is a dict and not empty?

@@ +75,5 @@
> +        self.assertRaises(errors.AppNotFoundError, get_version,
> +                          self.binary)
> +
> +    def test_without_platform_file(self):
> +        """With a missing sources file no exception should be thrown"""

The comment mentions sources file, but this is about the platform ini file. Should we not raise an exception if the platform.ini file is not present?
Attachment #8487230 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 28

3 years ago
(In reply to Dave Hunt (:davehunt) from comment #27)
> >          if not path:
> > +            raise errors.LocalAppNotFoundError('Application not found: %s' % \
> > +                                               path)
> 
> I would update the LocalAppNotFoundError to accept a path and format the
> message in the Error class.

That's something I already had but reverted to allow the affecting code to specify the real message, which would be hidden otherwise. In this case it's not that obvious but please check below for RemoteAppNotFoundError. Please let me know if we should really change that.

> >              if version._info.get('application_name') == 'B2G':
> >                  version = LocalB2GVersion(binary, sources=sources)
> > +    except errors.LocalAppNotFoundError:
> > +        version = RemoteB2GVersion(sources=sources, dm_type=dm_type,
> > +                                   host=host, device_serial=device_serial)
> 
> This could raise a RemoteAppNotFoundError even if the intention was to run
> against a local app. We could catch the RemoteAppNotFoundError and raise an
> AppNotFoundError so it's more generic.

I actually wanted to do that given that we talked about. But looks like I missed it. Thanks for spotting this Dave.

> ::: testing/mozbase/mozversion/tests/test_binary.py
> @@ +45,5 @@
> > +    @unittest.skipIf(not os.environ.get('BROWSER_PATH'),
> > +                     'No binary has been specified.')
> > +    def test_real_binary(self):
> > +        v = get_version(os.environ.get('BROWSER_PATH'))
> > +        self.assertNotEqual(v, {})
> 
> I don't think this would detect if v was None, and may pass. Should we
> perhaps check the v is a dict and not empty?

get_version should never return None in case no exception is thrown, but indeed we can have this check. Sadly I cannot use assertIsInstance() given that it has been introduced in Python 2.7.

> > +    def test_without_platform_file(self):
> > +        """With a missing sources file no exception should be thrown"""
> 
> The comment mentions sources file, but this is about the platform ini file.
> Should we not raise an exception if the platform.ini file is not present?

This is something we haven't done yet, but it's a good question. I think it's also mandatory beside the application.ini file. Would you mind when we do this change in a new bug, given that it is not really related to this one? I would like to not even pack more underlying behavior changes into this patch. It already scares me. :)
(In reply to Henrik Skupin (:whimboo) from comment #28)
> (In reply to Dave Hunt (:davehunt) from comment #27)
> > >          if not path:
> > > +            raise errors.LocalAppNotFoundError('Application not found: %s' % \
> > > +                                               path)
> > 
> > I would update the LocalAppNotFoundError to accept a path and format the
> > message in the Error class.
> 
> That's something I already had but reverted to allow the affecting code to
> specify the real message, which would be hidden otherwise. In this case it's
> not that obvious but please check below for RemoteAppNotFoundError. Please
> let me know if we should really change that.

RemoteAppNotFoundError is different. I think LocalAppNotFoundError is only likely to need a path and a common message.

> > > +    def test_without_platform_file(self):
> > > +        """With a missing sources file no exception should be thrown"""
> > 
> > The comment mentions sources file, but this is about the platform ini file.
> > Should we not raise an exception if the platform.ini file is not present?
> 
> This is something we haven't done yet, but it's a good question. I think
> it's also mandatory beside the application.ini file. Would you mind when we
> do this change in a new bug, given that it is not really related to this
> one? I would like to not even pack more underlying behavior changes into
> this patch. It already scares me. :)

Yes, please raise a separate bug.
(Assignee)

Comment 30

3 years ago
Created attachment 8487847 [details] [diff] [review]
mac signing changes v4

(In reply to Dave Hunt (:davehunt) from comment #29)
> > That's something I already had but reverted to allow the affecting code to
> > specify the real message, which would be hidden otherwise. In this case it's
> > not that obvious but please check below for RemoteAppNotFoundError. Please
> > let me know if we should really change that.
> 
> RemoteAppNotFoundError is different. I think LocalAppNotFoundError is only
> likely to need a path and a common message.

Makes sense. Updated this part.

> > > Should we not raise an exception if the platform.ini file is not present?
> > 
> > This is something we haven't done yet, but it's a good question. I think
> > it's also mandatory beside the application.ini file. Would you mind when we
> > do this change in a new bug, given that it is not really related to this
> > one? I would like to not even pack more underlying behavior changes into
> > this patch. It already scares me. :)
> 
> Yes, please raise a separate bug.

Filed that as bug 1065988 with you as mentor.

Also all other review comments have been fixed.
Attachment #8487230 - Attachment is obsolete: true
Attachment #8487847 - Flags: review?(dave.hunt)
(Assignee)

Comment 31

3 years ago
Requested a hopefully last try build:
https://tbpl.mozilla.org/?tree=Try&rev=00f439ffe105
(Assignee)

Comment 32

3 years ago
Try builds are all green. So waiting for Dave's review.
(Assignee)

Updated

3 years ago
Blocks: 1066046
Comment on attachment 8487847 [details] [diff] [review]
mac signing changes v4

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

r=me with the PEP8 nit addressed. I discovered an issue with mozdevice which I will raise separately.

::: testing/mozbase/mozversion/mozversion/errors.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +class VersionError(Exception):

Nit: PEP8 E302 expected 2 blank lines, found 1
Attachment #8487847 - Flags: review?(dave.hunt) → review+
(Assignee)

Comment 34

3 years ago
Created attachment 8487955 [details] [diff] [review]
mac signing changes v5

Updated patch with the nit fixed.
Attachment #8487847 - Attachment is obsolete: true
Attachment #8487955 - Flags: review+
(Assignee)

Comment 35

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd83b6fe218
Flags: in-testsuite+
Target Milestone: --- → mozilla35
(Assignee)

Updated

3 years ago
No longer blocks: 980801
https://hg.mozilla.org/mozilla-central/rev/5bd83b6fe218
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1064731
You need to log in before you can comment on or make changes to this bug.