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.

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

I'm not that familiar with mozversion, I'll defer to Dave here.

(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.

(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.

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.

I see. Anyway we will have to keep a fallback mechanism. So both locations will have to work.

With this patch we try to find the ini files in both locations on OS X.

Comment on attachment 8485006 [details] [diff] [review] mac signing changes v1 Review of attachment 8485006 [details] [diff] [review]: ----------------------------------------------------------------- Looks great!

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

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.

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.

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.

Yes, throwing an exception seems like the right approach, as it indicates that something is very wrong and we should not continue.

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.

Additional stability fixes, so we can also ensure to get expected values returned.

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.

I pushed the above patch to try: https://tbpl.mozilla.org/?tree=Try&rev=60e2ded81506

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.

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.

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.

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?

As discussed all above items are fixed, and the other necessary changes have been made. I will trigger a try build in a bit.

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

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?

(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.

(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.

Requested a hopefully last try build: https://tbpl.mozilla.org/?tree=Try&rev=00f439ffe105

Try builds are all green. So waiting for Dave's review.

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

Updated patch with the nit fixed.