Joel Maher ( :jmaher ) (UTC -8) Reporter
	Description • 10 years ago

I ran into errors with the new mozversion while testing locally.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 1 • 10 years ago

a pretty simple bug

	William Lachance (:wlach)
	Comment 2 • 10 years ago

Comment on attachment 8518146 [details] [diff] [review]
try/except around missing data (1.0)

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

This patch is really dangerous as it could allow us to submit invalid results to the graphserver (remember the NULL issue on MacOS X?).

I can't reproduce this problem using the latest released firefox and this command line:

talos -e ~/tmp/firefox/firefox --tppagecycles 1 --tpcycles 1 --cycles 1 -a tart --develop

It seems like every released version of Firefox includes the necessary information so I'm not sure what problem you ran into?

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 3 • 10 years ago

hmm, this was an official distribution inside my ubuntu package, found with `which firefox`.  I can see how this could be dangerous, maybe allow this if we are in --develop mode?

	William Lachance (:wlach)
	Comment 4 • 10 years ago

(In reply to Joel Maher (:jmaher) from comment #3)
> hmm, this was an official distribution inside my ubuntu package, found with
> `which firefox`.  I can see how this could be dangerous, maybe allow this if
> we are in --develop mode?

Ah, that would make sense.

Instead of hacking up Talos, I think it might be better to special case binary handling in mozversion to look for the .ini files in special locations if the path matches a default installation path. On Ubuntu the .ini files we need are in /usr/lib/firefox/. I'll bet something similar is the case on Win32 and MacOS X.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 5 • 10 years ago

to work on this bug you will need to be setup to develop in mozilla-central:
https://developer.mozilla.org/en-US/docs/mozilla-central

the mozversion code lives here in the tree:
http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozversion

a first step of this bug should be to support /usr/lib/firefox for application.ini if we cannot find it by default.

	William Lachance (:wlach)
	Comment 6 • 10 years ago

(In reply to Joel Maher (:jmaher) from comment #5)
> a first step of this bug should be to support /usr/lib/firefox for
> application.ini if we cannot find it by default.

I think we should only do this if the binary path is specified as /usr/bin/firefox. If specified as something like /home/wlach/tmp/firefox/firefox, we should probably error out as now (rather than using a system copy of firefox's ini version information which does not correspond).

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 7 • 10 years ago

Joel, what type of build is /usr/bin/firefox? Are you on Ubuntu? In such a case it will be a symlink, pointing to /usr/lib/firefox/firefox. Please check the application.ini file in that folder. I assume it does not contain the respository and sourcestamp information. So in the example code as attached, you should use version_info.get('application_repository') instead. 

This is a recent change, which was part of my patch on bug 1088060. In the cases above you cannot expect that the value exists.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 8 • 10 years ago

jmaher@jmaher-ThinkPad-X230:~$ echo `which firefox`
/usr/bin/firefox
jmaher@jmaher-ThinkPad-X230:~$ ls -la /usr/bin/firefox
lrwxrwxrwx 1 root root 25 Oct 13 17:03 /usr/bin/firefox -> ../lib/firefox/firefox.sh
jmaher@jmaher-ThinkPad-X230:~$ 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
jmaher@jmaher-ThinkPad-X230:~$

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 9 • 10 years ago

As you see there is not such information available. So change the code in talos if you want to use it with an official Firefox package on Ubuntu. This is not a bug in mozversion.

	William Lachance (:wlach)
	Comment 10 • 10 years ago

I wonder if we could solve this problem just by having mozversion follow symbolic links of the binary.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 11 • 10 years ago

:wlach, back to --develop mode to not require this?

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 12 • 10 years ago

(In reply to William Lachance (:wlach) from comment #10)
> I wonder if we could solve this problem just by having mozversion follow
> symbolic links of the binary.

mozversion is already doing this. I implemented this not that long ago in bug 1061809.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 13 • 10 years ago

Oh, and if you don't want to run such kind of builds with Talos you might wanna do it as you did in the patch. In our case for Mozmill tests we simply assume the latest release. Not the best, but so far it works.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 14 • 10 years ago

here is how to get setup with talos:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

use the patch on this bug and if we get an exception and we are not in --develop mode, then we need to quit the test run with an error.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 15 • 10 years ago

wlach, any further thoughts on a final direction here?  I keep running into this locally.  I don't mind hacking this out for --develop.

	William Lachance (:wlach)
	Comment 16 • 10 years ago

If it's not too hacky, yeah, I think we should ignore missing revision information in --develop mode.

I'm not sure if this is really a good first bug; I think I'd rather someone have a bit of experience with talos before tackling it. I might make it a "good next bug".

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 18 • 9 years ago

Comment on attachment 8562192 [details] [diff] [review]
Patch to check for sourcestamp/repository on a release build locally.

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

just one nit and an idea for making this a little better!

::: talos/run_tests.py
@@ +189,5 @@
> +        browser_config['repository'] = version_info['application_repository']
> +        browser_config['sourcestamp'] = version_info['application_changeset']
> +    except:
> +        if not browser_config['develop']:
> +            print "unable to find changeset or repository: %s" % version_info 

nit: please remove the blank space at the end of the line.

@@ +190,5 @@
> +        browser_config['sourcestamp'] = version_info['application_changeset']
> +    except:
> +        if not browser_config['develop']:
> +            print "unable to find changeset or repository: %s" % version_info 
> +            sys.exit()

I would like to hack these values as we might need them, how about adding:
else:
    browser_config['repository'] = 'develop'
    browser_config['sourcestamp'] = 'develop'

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 20 • 9 years ago

Comment on attachment 8562220 [details] [diff] [review]
Added else statements and removed blankspace

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

thanks!

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 21 • 9 years ago

https://hg.mozilla.org/build/talos/rev/9e89d4094c48