Closed Bug 554709 Opened 14 years ago Closed 14 years ago

Mozrunner should look for Firefox/Thunderbird on Windows systems in "C:\Program Files (x86)"

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: avarma, Assigned: k0scist)

Details

(Whiteboard: [jetpack-p1][mozmill-1.4.2+])

Attachments

(1 file, 1 obsolete file)

checks windows registry for path to Firefox/Thunderbird
2.24 KB, patch
ahal
: review+
Details | Diff | Splinter Review 
Myk Melez had problems with Mozrunner finding the proper location of Firefox in bug 554241 comment 11. He's on 64-bit Windows, so Firefox is located in a different Program Files folder, called "Program Files (x86)". This should be added to the list of default paths that Mozrunner busts through when it tries to locate Firefox/Thunderbird.
According to the Wikipedia docs on the ProgramFiles environment variable on 64-bit Windows [1]:

"""
This variable points to Program Files directory, which stores all the installed program of Windows and others. The default on English-language systems is C:\Program Files. In 64-bit editions of Windows (XP, 2003, Vista), there are also %ProgramFiles(x86)% which defaults to C:\Program Files (x86) and %ProgramW6432% which defaults to C:\Program Files. The %ProgramFiles% itself depends on whether the process requesting the environment variable is itself 32-bit or 64-bit (this is caused by Windows-on-Windows 64-bit redirection).
"""

So it looks like Myk's not just running on a 64-bit version of Windows, but he's using 64-bit Python; if he were using 32-bit Python, then presumably %ProgramFiles% would actually point to the right folder.

It looks like a successful patch would modify mozrunner to run through all possible variations of the program files folder, in order from most preferred/important to least, stopping when it found a match.

[1] http://en.wikipedia.org/wiki/Environment_variable
Can you please check what's the output of "os.environ['ProgramFiles']" is on a x86_64 system? Shouldn't it point to the folder you mentioned above?
OS: Mac OS X → Windows 7
Hardware: x86 → x86_64
(In reply to comment #2)
> Can you please check what's the output of "os.environ['ProgramFiles']" is on a
> x86_64 system? Shouldn't it point to the folder you mentioned above?

>>> import os
>>> os.environ['ProgramFiles']
'C:\\Program Files'

Note that there are both "Program Files" and "Program Files (x86)" directories on the machine.  According to Internet sources, this is Microsoft's way of distinguishing between 32-bit and 64-bit binaries.  It puts the 32-bit ones in "Program Files (x86)" and the 64-bit ones in "Program Files".
Myk, could you then please attach the contents of os.environ to that bug? I do not have a 64bit Windows so it's kinda hard for me to start thinking.
Microsoft Windows [Version 6.1.7600]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

C:\Users\myk>python
Python 2.6.4 (r264:75708, Oct 26 2009, 07:36:50) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.environ
{
  'TMP': 'C: \\Users\\myk\\AppData\\Local\\Temp',
  'COMPUTERNAME': 'WIN-4SUQ9RPFJTK',
  'USERDOMAIN': 'WIN-4SUQ9RPFJTK',
  'PSMODULEPATH': 'C: \\Windows\\system32\\WindowsPowerShell\\v1.0\\Modules\\',
  'VS90COMNTOOLS': 'c: \\Program Files (x86)\\Microsoft Visual Studio 9.0\\Common7\\Tools\\',
  'COMMONPROGRAMFILES': 'C: \\Program Files\\Common Files',
  'PROCESSOR_IDENTIFIER': 'Intel64 Family 6 Model 23 Stepping 6,
  GenuineIntel',
  'PROGRAMFILES': 'C: \\Program Files',
  'PROCESSOR_REVISION': '1706',
  'SYSTEMROOT': 'C: \\Windows',
  'PATH': 'C: \\Python26\\;C: \\Program Files (x86)\\ActiveState Komodo IDE 5\\;C: \\Windows\\system32;C: \\Windows;C: \\Windows\\System32\\Wbem;C: \\Windows\\System32\\WindowsPowerShell\\v1.0\\;c: \\Program Files(x86)\\Microsoft SQL Server\\90\\Tools\\binn\\',
  'PROGRAMFILES(X86)': 'C: \\Program Files (x86)',
  'COMSPEC': 'C: \\Windows\\system32\\cmd.exe',
  'TEMP': 'C: \\Users\\myk\\AppData\\Local\\Temp',
  'COMMONPROGRAMFILES(X86)': 'C: \\Program Files (x86)\\Common Files',
  'PROCESSOR_ARCHITECTURE': 'AMD64',
  'ALLUSERSPROFILE': 'C: \\ProgramData',
  'LOCALAPPDATA': 'C: \\Users\\myk\\AppData\\Local',
  'HOMEPATH': '\\Users\\myk',
  'PROGRAMW6432': 'C: \\Program Files',
  'USERNAME': 'myk',
  'LOGONSERVER': '\\\\WIN-4SUQ9RPFJTK',
  'PROMPT': '$P$G',
  'SESSIONNAME': 'Console',
  'PROGRAMDATA': 'C: \\ProgramData',
  'PATHEXT': '.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC',
  'FP_NO_HOST_CHECK': 'NO',
  'WINDIR': 'C: \\Windows',
  'APPDATA': 'C: \\Users\\myk\\AppData\\Roaming',
  'HOMEDRIVE': 'C: ',
  'SYSTEMDRIVE': 'C: ',
  'NUMBER_OF_PROCESSORS': '1',
  'MOZ_NO_RESET_PATH': '1',
  'PROCESSOR_LEVEL': '6',
  'COMMONPROGRAMW6432': 'C: \\Program Files\\Common Files',
  'OS': 'Windows_NT',
  'PUBLIC': 'C: \\Users\\Public',
  'USERPROFILE': 'C: \\Users\\myk'
}
Ok, while thinking about that I have to ask myself if we shouldn't better use the registry to get the default installation of Firefox. See, when we will offer 32bit and 64bit builds we still will not know in which folder Firefox is installed.

But having the registry those two links will give us all the information:

HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\Mozilla Firefox - CurrentVersion
HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\Mozilla Firefox\3.6.2 (en-US)\Main - PathToExe

Clint or Mikeal, what do you think?
Well, you didn't ask me, but I think it's a grand idea. :) Since Python comes with the _winreg module this should be easy to implement without adding any weird dependencies or digging into ctypes, too.
Whiteboard: [jetpack-p1]
I like the idea too, for the record.  Sorry for not responding earlier.
Whiteboard: [jetpack-p1] → [jetpack-p1][mozmill-1.4.2+]
(In reply to comment #6)
> Ok, while thinking about that I have to ask myself if we shouldn't better use
> the registry to get the default installation of Firefox. See, when we will
> offer 32bit and 64bit builds we still will not know in which folder Firefox is
> installed.
> 
> But having the registry those two links will give us all the information:
> 
> HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\Mozilla Firefox - CurrentVersion
> HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\Mozilla Firefox\3.6.2 (en-US)\Main -
> PathToExe
> 
> Clint or Mikeal, what do you think?

See also: http://kaply.com/weblog/2007/04/23/firefox-and-the-windows-registry/
Assignee: nobody → jhammel
* look in the windows registry for the default app location; bug 554709

* remove unused function: NaN

* remove unused import: pwd
Attachment #458007 - Flags: review?(ahalberstadt)
Comment on attachment 458007 [details] [diff] [review]
checks windows registry for path to Firefox/Thunderbird

Looks good and really straightforward.  Just a few things:

- I assume the two deletions aren't related to the bug?
- For the exception, I think you can just catch 'WindowsError'.
- I realize this doesn't do anything functionally, but maybe add some sort of default 'app_name' to the base Runner class so future implementors know they should set it.

Anyway, I couldn't find anything wrong, r+.  Let me know if you have questions.
Attachment #458007 - Flags: review?(ahalberstadt) → review+
update of attachment 458007 [details] [diff] [review] to overcome bitrot
Attachment #458007 - Attachment is obsolete: true
Attachment #458378 - Flags: review?(ahalberstadt)
Comment on attachment 458378 [details] [diff] [review]
checks windows registry for path to Firefox/Thunderbird

Still looks good, same comments as before.

r=ahalberstadt
Attachment #458378 - Flags: review?(ahalberstadt) → review+
(In reply to comment #11)
> Comment on attachment 458007 [details] [diff] [review]
> checks windows registry for path to Firefox/Thunderbird
> 
> Looks good and really straightforward.  Just a few things:
> 
> - I assume the two deletions aren't related to the bug?

Now one, but yes, just general cleanup.

> - For the exception, I think you can just catch 'WindowsError'.

I'm not sure if this is sufficient.  There is a possible ValueError and ImportError if the platform information is wrong.  Not sure what error is given if I try to access a key that doesn't existed, but its not WindowsError.  Probably the thing to do to debug this is remove the try except entirely and then add exceptions in that occur in the wild and log them.  But its probably also not super-important at this point.

> - I realize this doesn't do anything functionally, but maybe add some sort of
> default 'app_name' to the base Runner class so future implementors know they
> should set it.

I left this out intentionally as I want a hard failure if a subclass doesn't implement it (vs just using some wrong default).  A comment might be nice, though.
 
> Anyway, I couldn't find anything wrong, r+.  Let me know if you have questions.
Atul or Myk, can you both please check the latest dev build of Mozmill? Does it fix your reported issue?
Verified by installing Firefox to a non default location. Mozmill grabs the right bits out from the registry and starts it.
Status: RESOLVED → VERIFIED
Note that this doesn't work with Minefield.  Since we look at r"Software\Mozilla\Mozilla %s" % self.app_name and Minefield is at Software\Mozilla\Minefield , we don't get the correct version.
That code is already too smart, and has never worked with Minefield, ever.  That's one of the reasons I'd advocate completely removing it from 2.0 entirely (except that real users find it useful).  I find this code completely annoying.  We're not going to fix it to work with Minefield.  Working with branded products is more than enough in my opinion.
FWIW, Jetpack devs test against Minefield all the time.
I'm not seeing this fixed on the tip of the Jetpack SDK, but it's not clear that the tip is running a version of Mozrunner from after the fix (and if it is, perhaps the "ERROR: The system was unable to find the specified registry key or value." message I see upon activating the SDK is to blame).  Atul: can you confirm one way or another?
Myk, can you please run "mozrunner --info" which will give you more information about the actual version of Mozrunner.
Resolved, until we have a "WFM" from the Jetpack guys.
Status: VERIFIED → RESOLVED
Closed: 14 years ago → 14 years ago
I tried running |mozrunner --info|, but it threw an exception because I don't have pkg_resources.

However, I was able to confirm that the version of mozrunner in the SDK does have the change by perusing its code.

Nevertheless, I still have the same problem, so it doesn't look like the change fixed my problem.

To confirm, I'm running Windows 7 Professional 64bit, and I have the 32bit version of Firefox 3.6 (specifically, 3.6.8, the latest stable version) installed in the default location, which is "C:\Program Files (x86)\Mozilla Firefox\firefox.exe".  That version is set as my default browser.

Also note bug 590666, which was just filed about the same problem.  The bug includes a patch, attachment 469155 [details] [diff] [review], which does resolve my problem and, as a bonus, it also supports Minefield in a straightforward way.

Might it be possible to land that patch upstream?
(In reply to comment #24)
> I tried running |mozrunner --info|, but it threw an exception because I don't
> have pkg_resources.
> 

Yes, this will only work if you have mozrunner installed via setuptools

> However, I was able to confirm that the version of mozrunner in the SDK does
> have the change by perusing its code.
> 
> Nevertheless, I still have the same problem, so it doesn't look like the change
> fixed my problem.
> 
> To confirm, I'm running Windows 7 Professional 64bit, and I have the 32bit
> version of Firefox 3.6 (specifically, 3.6.8, the latest stable version)
> installed in the default location, which is "C:\Program Files (x86)\Mozilla
> Firefox\firefox.exe".  That version is set as my default browser.
> 
> Also note bug 590666, which was just filed about the same problem.  The bug
> includes a patch, attachment 469155 [details] [diff] [review], which does resolve my problem and, as a
> bonus, it also supports Minefield in a straightforward way.
> 
> Might it be possible to land that patch upstream?

I would not be opposed to landing the part of the patch that deals with firefox.exe if it solves problems (don't have time to investigate right now, sorry).  I don't think we should special-case minefield.
(In reply to comment #25)
> I would not be opposed to landing the part of the patch that deals with
> firefox.exe if it solves problems (don't have time to investigate right now,
> sorry).  I don't think we should special-case minefield.

Why not?  Minefield is a special case.  It's not just yet another name one can call a Mozilla-based browser (which of course we can't support in the general case).

And for Jetpack's use of mozrunner, in any case, Minefield is an important target, because we are tracking the latest nightly build of Firefox 4 in addition to Firefox 3.6 (and in fact the latest nightly is even more important than Firefox 3.6, since Firefox 4 is our primary target platform).
We already have the fix from bug 554709 in jetpack, but it doesn't work, because I don't have any Mozilla stuff in the registry.
May be because I never answer to the dialog "set firefox as your default browser" ? ... don't really know ...

Don't hesitate to ask me for another patch if you really want to split Firefox from Minefield.
(In reply to comment #27)
> We already have the fix from bug 554709 in jetpack, but it doesn't work,
> because I don't have any Mozilla stuff in the registry.
> May be because I never answer to the dialog "set firefox as your default
> browser" ? ... don't really know ...

I wondered about that too, so I made sure Firefox was set as my default browser before the last round of testing, but it still didn't work.
Mozrunner doesn't use any registry entries.  It only looks in certain directory locations because by default Firefox doesn't make any registry entires until you click the "use default browser" bit which is not usually set on testing profiles.

I'm taking a couple of changes for a mozrunner bump, and I'll take one to add Minefield to the list of directories it looks for if that will help out you jetpack folk, you can see the bug we're tracking this on in bug 590666.  I recommend we move further discussion there.
(In reply to comment #29)
> I'm taking a couple of changes for a mozrunner bump, and I'll take one to add
> Minefield to the list of directories it looks for if that will help out you
> jetpack folk, you can see the bug we're tracking this on in bug 590666.  I
> recommend we move further discussion there.

Thanks Clint, will do!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: