Closed Bug 795288 Opened 12 years ago Closed 10 years ago

[mozinstall] is_installer() should not return true for an already installed binary

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Platform:
All
Windows XP
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: whimboo, Assigned: jotes)

References

Details

(Whiteboard: [mentor=whimboo][lang=py])

Attachments

(1 file, 13 obsolete files)

0001-Bug-795288-support-for-PE-headers-in-windows-version.patch
57.33 KB, patch
whimboo
: review+
Details | Diff | Splinter Review 
While working on our automation scripts for Mozmill 2.0 I have seen that when processing the given binary with MozInstall it always returns true when an .exe file has been specified. We should only return true for real installers, so we should check the exe file if it is a real installer.

I will have a look at.
Summary: [mozbase] is_installer() should not return true for an already installed binary → [mozinstall] is_installer() should not return true for an already installed binary
Jeff, so we have two options here:

1. Do a regex check if a file called %app_name%.exe has been specified

2. Read the metadata of the exe file but it will require another pypi package (http://pypi.python.org/pypi/hachoir-core)

With which one of those you would be most comfortable? Version 1 seems to be ok for now given that the binary will always be called after the application. But with this change we would have an API change of is_installer() because the app_name has to be added as parameter.
It helps
Gah, accidental submit.  It helps if I am CCed when asked a question.  So I'm not sure if I understand the problem, exactly.  A test case would help, and it would be nice if mozInstall had tests in general.   I'd rather not do either.  Filenames can change so depending on them seems error-prone.  I'd also rather not depend on another 3rd party library if possible.

Can you explain why this is a practical problem in any more detail?  I think in general if you give mozInstall a bogus .exe, that is user error
(In reply to Jeff Hammel [:jhammel] from comment #3)
> Can you explain why this is a practical problem in any more detail?  I think
> in general if you give mozInstall a bogus .exe, that is user error

On Windows an exe file can mean an installer or the actual application binary. To not require from the consumers to implement those checks again, it would be helpful if mozinstall could return the type of the exe file.

C:\\Firefox 16.0 Setup.exe
C:\\Mozilla Firefox\\firefox.exe

Only the first is a valid installer.
Clint mentioned to me something he wanted to check. He had something else in mind how to check for an installer.
(In reply to Henrik Skupin (:whimboo) from comment #5)
> Clint mentioned to me something he wanted to check. He had something else in
> mind how to check for an installer.

Clint, could you still provide us the details?
Assignee: hskupin → nobody
Flags: needinfo?(ctalbert)
There is really very little way for us to know if it's an application installer or not. The API I was thinking of doesn't actually do anything interesting as I had thought that it did.

I think we rely on the consumers of mozInstall to do that check if they want to do it.

I propose we WONTFIX this bug.
Flags: needinfo?(ctalbert)
Hearing no diving saves to prevent the wontfix, let's go ahead with that. If we find a nice simple way to do this in mozbase, we can reopen, but until then let's treat this as something that a user of mozbase will have to do.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Sorry, I missed to update the bug. I still think it should be part of mozInstall and should not be forced by each consumer of the code.

We have two use cases here which are:

* The consumer specifies the installer
* The consumer specifies the binary of the actual application

Given that mozinstall is going to work for each and every gecko based application, we can always assume that an application.ini file is present in the same folder as the application binary. With that in mind we could do such a check, which will only not work if an installer is actually placed in the same folder as the binary. I don't believe that this will ever happen.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
So in the 2nd use case above (the consumer specifies a path to the actual application eg.firefox.exe), that is considered an invalid workflow and mozInstall will just throw its usual exception as it always has, correct?
- raise Exception('Execution of installer failed.')

And the usage manual seems to confirm this, correct?
- Usage: mozinstall-script.py [options] installer

For the bug work in the valid use case(consumer specifies the installer), I updated a bit of get_binary(), where if an application.ini does not exist within a windows install, we consider a suitable binary has not been found, and will raise the usual exception and then uninstall it.
- raise Exception('[x] does not contain a valid binary.')

Initially I was confused by the title of the bug and thought is_installer() required the changes. But looking at the code I believe it is get_binary() where the check would occur.

If it sounds like I'm on the right track, let me know :) I will assign myself the bug and put up a patch. I've tested the failure case of no application.ini (it throws as expected), and put some prints in during testing to ensure the new code paths are being traversed during an install.
(In reply to Jonathan French (:jfrench) from comment #10)
> So in the 2nd use case above (the consumer specifies a path to the actual
> application eg.firefox.exe), that is considered an invalid workflow and
> mozInstall will just throw its usual exception as it always has, correct?
> - raise Exception('Execution of installer failed.')

We probably should raise a specific exception something like BadInstallerError.

> For the bug work in the valid use case(consumer specifies the installer), I
> updated a bit of get_binary(), where if an application.ini does not exist
> within a windows install, we consider a suitable binary has not been found,
> and will raise the usual exception and then uninstall it.
> - raise Exception('[x] does not contain a valid binary.')

We probably should not totally depend on the application.ini file, at least not until we have checked the situation with xulrunner applications. Those don't necessarily have the application.ini file in the binary folder. We most likely want to check for some very specific Gecko related files like for the JS interpreter, which will never go away. 

> Initially I was confused by the title of the bug and thought is_installer()
> required the changes. But looking at the code I believe it is get_binary()
> where the check would occur.

Why do you think that? AFAIK get_binary() gets called after the installation attempt.

> If it sounds like I'm on the right track, let me know :) I will assign
> myself the bug and put up a patch. I've tested the failure case of no
> application.ini (it throws as expected), and put some prints in during
> testing to ensure the new code paths are being traversed during an install.

I would love if oyu could work on this bug given that it currently blocks our automation scripts from being run against already installed versions of Firefox.
I plan to have a go at this after some other bugs have settled, or in parallel with them; but I will leave the bug unassigned for now in case another contributor wishes to jump on it. No worries either way. I will also touch base with Henrik on IRC w.r.t. any outstanding questions above if I can't figure them out.
I've had a bunch of other bugs on the go. I noticed today on IRC that perhaps jarekps may grab this one, which would be fine.
:jfrench
Yeah, i'm trying to find acceptable solution (i was surprised that Firefox installer doesn't support silent mode).
I'm working on this and #911048 (which are kinda related). I hope to have something to post today/tommorow.
(In reply to Jarek Śmiejczak from comment #14)
> Yeah, i'm trying to find acceptable solution (i was surprised that Firefox
> installer doesn't support silent mode).

Hm? Our installer does indeed support silent mode. That's what currently used to get it installed, and which is part of the mozinstall code.
:hskupin

Sorry, i meant stub installer.
The stub installer is a totally different thing, which we don't support yet. The feature will be added by bug 911048. So don't worry about that yet.
Ok, i'm implementing this like Jonathan suggested (checking if application.ini exists in folder with binary).
But i wouldn't be myself if i didn't explore solution with hachoir ;-)
I've created a simple hello world app, which displays all found available metadata.
I've created results for installer and firefox binary:
https://gist.github.com/jarekps/6529272

I'm not sure if it's good to assume that installer wouldn't have information about build... I'd like to hear Henrik's opinion.
(In reply to Jarek Śmiejczak from comment #18)
> Ok, i'm implementing this like Jonathan suggested (checking if
> application.ini exists in folder with binary).

Please note that application.ini is not necessarily located in the binary folder. That applies AFAIR for non-static libxul builds. Please read my comment 11. We should better check for a library file like the js interpreter, which have to be present.

> I've created results for installer and firefox binary:
> https://gist.github.com/jarekps/6529272

Interesting. So that means the installer doesn't have a title, author, and version set? We might be able to use that information. Do the missing items also apply to the stub installer? And how do xulrunner applications look like in terms of that information? Also how much overhead brings hachoir in terms of dependenices and size?
Assignee: nobody → jot
Status: REOPENED → ASSIGNED
> Do the missing items also apply to the stub installer?
From what i see, yes (i've executed test on latest nightly)

> And how do xulrunner applications look like in terms of that information?
I can check if you could provide me examples which i should check (besides thunderbird ;-).

> Also how much overhead brings hachoir in terms of dependenices and size?
Something about 1,5 MB of packages from pypi, nothing more included (or i'm aware of).

In meantime I've explored different concept - i noticed that setup is just packed via 7z.sfx (but i'm not sure from which version of installer it's applicable, i'll try to research this today). And then i've recalled that windows .exe format (PE) has some data in its headers.
Just for curiosity, i've installed pefile package (around 50 KB, available via pip) and retrieved following info (keys with values):
https://gist.github.com/jarekps/6547949
(In reply to Jarek Śmiejczak from comment #20)
> > And how do xulrunner applications look like in terms of that information?
> I can check if you could provide me examples which i should check (besides
> thunderbird ;-).

Please see the example application attached on bug 775416. You only need the XulRunner SDK.

> In meantime I've explored different concept - i noticed that setup is just
> packed via 7z.sfx (but i'm not sure from which version of installer it's
> applicable, i'll try to research this today). And then i've recalled that
> windows .exe format (PE) has some data in its headers.
> Just for curiosity, i've installed pefile package (around 50 KB, available
> via pip) and retrieved following info (keys with values):
> https://gist.github.com/jarekps/6547949

Sweet! That looks better from the first sight. So how would you differentiate between an installer and a binary now?
> Sweet! That looks better from the first sight. So how would you differentiate between an installer and a binary now?
I'll just add check, if companyName=='Mozilla' and  OriginalFilename == '7zS.sfx.exe'. I think that should be enough for installers.
Looking over those items I would simply check for the buildid. That should be unique to the build itself, while the installer doesn't know anything about it.
Ok, i've added patch in which i'm checking existence of BuildID.
Attachment #804935 - Flags: review?(hskupin)
Comment on attachment 804935 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

That's actually a good start. I gave some comments inline, which we would need to get fixed. But in general it seems to be a nice solution for the installer detection.

::: mozinstall/mozinstall/mozinstall.py
@@ +133,4 @@
>          del trbk
>  
>  
> +def _get_exe_metadata(src):

Personally I would prefer to stay with 'pe' instead of 'exe', because I think it could also cover DLL and other files?

@@ +139,5 @@
> +    Arguments:
> +    src -- path to file
> +
> +    """
> +    import pefile

Please import globally at the beginning of the file.

@@ +142,5 @@
> +    """
> +    import pefile
> +
> +    try:
> +        exe_data = pefile.PE(src)

Can we turn on the fast load modus here? See the fast_load argument.

@@ +143,5 @@
> +    import pefile
> +
> +    try:
> +        exe_data = pefile.PE(src)
> +        for entry in getattr(exe_data, 'FileInfo', []):

Is FileInfo not always present?

@@ +144,5 @@
> +
> +    try:
> +        exe_data = pefile.PE(src)
> +        for entry in getattr(exe_data, 'FileInfo', []):
> +            if entry.name == 'StringFileInfo':

Can we use entry.Key here?

@@ +145,5 @@
> +    try:
> +        exe_data = pefile.PE(src)
> +        for entry in getattr(exe_data, 'FileInfo', []):
> +            if entry.name == 'StringFileInfo':
> +                return entry.StringTable[0].entries

So we can assume that the first array entry of StringTable always contains that information?

@@ +149,5 @@
> +                return entry.StringTable[0].entries
> +        return {}
> +
> +    except:
> +        raise InstallError('Cannot access file\'s metadata: %s' % src)

I don't like when we hide the underlying exception. It makes it really hard to figure out what went wrong.

::: mozinstall/setup.py
@@ +14,5 @@
>  PACKAGE_VERSION = '1.7'
>  
>  deps = ['mozinfo >= 0.4',
> +        'mozfile',
> +        'pefile==1.2.10-123'

Please put spaces around the equal signs so we are in sync with the other line above.
Attachment #804935 - Flags: review?(hskupin) → review-
> Personally I would prefer to stay with 'pe' instead of 'exe', because I think it could also cover DLL and other files?

Changed to 'pe' as you suggested. However, we're thinking only about .exe in our case?

> Can we turn on the fast load modus here? See the fast_load argument.
I've enabled this locallybut faster load means that library doesn't load FileInfo by default. Of course, we can try to port this part of library but i'm not sure if we gain that much performance.

> Is FileInfo not always present?
> Can we use entry.Key here?
> So we can assume that the first array entry of StringTable always contains that information?
After reading internals of Pefile i've changed structure of this function.
Attachment #804935 - Attachment is obsolete: true
Attachment #806264 - Flags: review?(hskupin)
(In reply to Jarek Śmiejczak from comment #26)
> Changed to 'pe' as you suggested. However, we're thinking only about .exe in
> our case?

Right. There are no other file types for the binaries available.

> > Can we turn on the fast load modus here? See the fast_load argument.
> I've enabled this locallybut faster load means that library doesn't load
> FileInfo by default. Of course, we can try to port this part of library but
> i'm not sure if we gain that much performance.

Oh I see. So given that we need that, leave it in.

> > Is FileInfo not always present?
> > Can we use entry.Key here?
> > So we can assume that the first array entry of StringTable always contains that information?
> After reading internals of Pefile i've changed structure of this function.

Lets see when I do the review in a minute.
Comment on attachment 806264 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

Looks way better meanwhile. Just some remaining things to fix.

::: mozinstall/mozinstall/mozinstall.py
@@ +10,4 @@
>  import mozfile
>  from optparse import OptionParser
>  import os
> +import pefile

Please add that import to a new section separated by a blank line below that list. It's not a default module shipped with Python.

@@ +104,5 @@
>      dest = os.path.realpath(dest)
>  
>      if not is_installer(src):
> +        if src.lower().endswith('.exe'):
> +            raise InvalidSource(src + ' is not valid installer file.')

Why have you added this extra check? We would raise the exception anyway in the next line.

@@ +142,5 @@
> +    Arguments:
> +    src -- path to file
> +
> +    """
> +    exe_data = pefile.PE(src)

I thought you changed it to ´pe´?

@@ +148,5 @@
> +
> +    for info in exe_data.FileInfo:
> +        if info.Key == 'StringFileInfo':
> +            for string in info.StringTable:
> +                data.update(string.entries)

Looks much cleaner now. Thanks for diving into that library.

@@ +179,5 @@
> +        if src.lower().endswith('.exe'):
> +            file_metadata = _get_pe_metadata(src)
> +            return 'BuildID' not in file_metadata
> +
> +        else:

nit: please remove the blank line above.
Attachment #806264 - Flags: review?(hskupin) → review-
Ok, i've added next fixes. Please review.
Attachment #806264 - Attachment is obsolete: true
Attachment #810208 - Flags: review?(hskupin)
Sorry, previous patch was a little broken because i didn't spot one thing which was fixed on my desktop machine (and i didn't push this change before travel).
pe_file.FileInfo is optional property, not always set/available (depends on file structure). Anyway - everything now works fine (and tests are passing).
Attachment #810208 - Attachment is obsolete: true
Attachment #810208 - Flags: review?(hskupin)
Attachment #810226 - Flags: review?(hskupin)
Comment on attachment 810226 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

::: mozinstall/mozinstall/mozinstall.py
@@ +142,5 @@
> +    """
> +    pe_data = pefile.PE(src)
> +    data = {}
> +
> +    for info in pe_data.FileInfo:

It looks like you missed to push the right updated patch again? The last two versions are identical and as you said, you fixed this check with your latest addition. But I don't see it yet.
Attachment #810226 - Flags: review?(hskupin) → review-
Yes, i've uploaded wrong patch again. I'm uploading new which passes tests for mozinstall.
Attachment #810226 - Attachment is obsolete: true
Attachment #816328 - Flags: review?(hskupin)
Comment on attachment 816328 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

This looks great, Jarek! I finally had the time to test it in my Windows 7 VM, and all works fine too. We correctly detect if the given file is an installer or just an executable. Good work!

Please get the nit fixed and also ask :jhammel for review.

::: mozinstall/mozinstall/mozinstall.py
@@ +177,5 @@
>      elif mozinfo.isWin:
> +        if src.lower().endswith('.exe'):
> +            file_metadata = _get_pe_metadata(src)
> +            return 'BuildID' not in file_metadata
> +        return zipfile.is_zipfile(src)

nit: please add a single new line above the last return
Attachment #816328 - Flags: review?(hskupin) → review+
:whimboo
I've removed nit as you suggested. I'm uploading new version with :jhammel as a reviewer.
Attachment #816328 - Attachment is obsolete: true
Attachment #823092 - Flags: review?(jhammel)
Comment on attachment 823092 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

I don't really know if I'm the right person to review here.  I am wary of

A. adding a dep
B. adding a dep with a very precise version (1.2.10-123)

I'll give my r+ if this meets with Whimboo's needs; flagging him for feedback
Attachment #823092 - Flags: review?(jhammel)
Attachment #823092 - Flags: review+
Attachment #823092 - Flags: feedback?(hskupin)
Comment on attachment 823092 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

::: mozinstall/setup.py
@@ +14,5 @@
>  PACKAGE_VERSION = '1.7'
>  
>  deps = ['mozinfo >= 0.4',
> +        'mozfile',
> +        'pefile == 1.2.10-123'

Jeff, this is an external module we don't have control over. Reasoning for having a strict dependency is that we do not want to risk any breakage in our tools, only because they got a new version released, which contains API breaking changes. I can see that the version number schema is strange, but that's something on their end.

Jarek, is -xyz an interim release or what does it mean?
Attachment #823092 - Flags: feedback?(hskupin) → feedback+
I don't have a problem with such a specific version IFF it doesn't break and preferably the reason for something this specific is stated.  Usually, e.g. 1.2.10 would be more than sufficient if they have sane versioning.
Indeed. Lets see if there is something better available. Jarek, it would be good if you can have a look or ask.
Flags: needinfo?(jot)
From my quick investigation it looks like author maintains 1.2.10 for pretty long time with -XYZ for smaller fixes. I agree that dep: pefile>=1.2.10. I can ask author about his versioning methodology but it may take some time.
Flags: needinfo?(jot)
*I agree that more general dep: pefile>=1.2.10 would be better.* I've pressed enter too fast ;) Unfortunately i don't see option to edit comments.
As mentioned above I would refrain from doing a more relaxed dependency given that it is an external module we don't really have control over. It can cause our packages to fail whenever a new API incompatible version gets released.

I would say lets ask him if another major/minor release is planed without that suffix. If not then lets get in what we currently have.
Okay, i'll try to contact with author/current maintainer and post update after response.
@whimboo:
Response from author (Ero Carrera):
Hi Jarek,

pefile is currently in a stable state. The PE format hasn't seen any
major updates in years so there's little major functionality to add. I
do fix some bugs every now and then but that's pretty much it.
Regarding the version number, it got "stuck" 1.2.10-xyz a bit
randomly. I never had a target version in mind and, as the changes
grew smaller, I felt there was less justification for a jump to a 1.3
or 2.0.

So far I plan no big changes to pefile and nothing that would make it
backward incompatible. I may one day decide to work on a version for
python 3. But I would like to keep both in sync.

I hope this helps.
--------------------------------

I so i'll change this strict requirement into 1.2.10-XYZ or newer. If you agree (and Jeff of course).
Flags: needinfo?(hskupin)
Personally I would still prefer a strict dependency here given that it is an external module, and what I have expressed twice above. But I would leave this to Jeff as one of the owners of the mozbase repository.
Flags: needinfo?(hskupin) → needinfo?(jhammel)
> I so i'll change this strict requirement into 1.2.10-XYZ or newer.
> If you agree (and Jeff of course).

Yeah, I'd say this is the way to go given the author's response
Flags: needinfo?(jhammel)
I've updated dependency in setup.py. Could you review again?
Attachment #823092 - Attachment is obsolete: true
Attachment #828319 - Flags: review?(hskupin)
Comment on attachment 828319 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

Looks fine, but now please also add at least one test for this new feature if possible.
Attachment #828319 - Flags: review?(hskupin) → review+
Whiteboard: [mentor=whimboo][lang=py]
Hi, i've added small exe file which only returns status 0 and has BuilID property set to '12345689'. Added check in tests for is_installer.
Attachment #828319 - Attachment is obsolete: true
Attachment #831874 - Flags: review?(hskupin)
Comment on attachment 831874 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

Please let us build the exe file on our own similar to the tests under mozprocess. Also we would need two different kind of binaries for a full test.
Attachment #831874 - Flags: review?(hskupin)
Hi, I've added generation of mock file as you suggested.
I'm not sure if understand why do we need second executable? We should test two scenarios:
is_installer for firefox.exe which is already added to repo(doesn't have BuildID property)
is_installer for generated stub which will be compiled per test-run (has BuildID property)

Thanks for your input and please review again.
Attachment #831874 - Attachment is obsolete: true
Attachment #832626 - Flags: review?(hskupin)
(In reply to Jarek Śmiejczak from comment #50)
> is_installer for firefox.exe which is already added to repo(doesn't have
> BuildID property)

Interesting. I haven't seen those yet. Haven't had a look into this test folder for a while. So ok, lets make use of it.

> is_installer for generated stub which will be compiled per test-run (has
> BuildID property)

We might want to move this into a sub folder like `Build_Stub`. And to be mostly identical with the other binaries you can also add the compiled binary to the repository, or at least a script which generates it.
Comment on attachment 832626 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

::: mozinstall/tests/test.py
@@ +17,5 @@
>  # Store file location at load time
>  here = os.path.dirname(os.path.abspath(__file__))
>  
> +
> +def compile_executables(test_dir):

Lets move all the build stuff to an extra file in the appropriate sub folder. So we can re-create the binary later if changes are necessary.

@@ +36,5 @@
> +
> +        # And then we compile next version of browser file which
> +        # contains tested PE Header ('BuidID')
> +        ['gcc', '--output=%s' % browser_exe, stub_example_c,
> +            stub_example_o],

Lets call it firefox.exe.

@@ +63,2 @@
>          cls.dmg = os.path.join(here, 'Installer-Stubs', 'firefox.dmg')
> +        cls.installer_exe = os.path.join(here, 'Installer-Stubs', 'firefox.exe')

I would also change the other lines to `installer_%s`.

@@ +63,3 @@
>          cls.dmg = os.path.join(here, 'Installer-Stubs', 'firefox.dmg')
> +        cls.installer_exe = os.path.join(here, 'Installer-Stubs', 'firefox.exe')
> +        cls.browser_exe = os.path.join(here, 'Installer-Stubs', 'firefox.bin.exe')

Please don't put the real binary in here. It's not related to the installers.

@@ +117,5 @@
>              # test zip installer
>              self.assertTrue(mozinstall.is_installer(self.zipfile))
>              # test exe installer
> +            self.assertTrue(mozinstall.is_installer(self.installer_exe))
> +            # test invalid 

Please add a new method to the test class for the is_installer() checks.

nit: there are some white spaces across the patches you also might want to fix.
Attachment #832626 - Flags: review?(hskupin) → review-
> Lets move all the build stuff to an extra file in the appropriate sub folder. So we can re-create the binary later if changes are necessary.
Do you want to manually build this file or just modify build code to do it only once?

> Lets call it firefox.exe.
Okay, but must be in different dir than rest of exe files to avoid collision with existing firefox.exe which is stub installer.

> Please don't put the real binary in here. It's not related to the installers.
You mean to move this as a inline into testcase instead of keeping it as a property?
Flags: needinfo?(hskupin)
(In reply to Jarek Śmiejczak from comment #53)
> > Lets move all the build stuff to an extra file in the appropriate sub folder. So we can re-create the binary later if changes are necessary.
> Do you want to manually build this file or just modify build code to do it
> only once?

Just build it yourself already and add the exe to the repository. I simply want to have the source code around so that we could always tweak it.

> > Lets call it firefox.exe.
> Okay, but must be in different dir than rest of exe files to avoid collision
> with existing firefox.exe which is stub installer.

That's what I said, yes. Also I wonder if we really need those large installer files in the repository. Mockups should also work here. We should file a follow-up bug for it.

> > Please don't put the real binary in here. It's not related to the installers.
> You mean to move this as a inline into testcase instead of keeping it as a
> property?

No, just use another sub folder for it.
Flags: needinfo?(hskupin)
Got it. :)
Could you review again? Thanks in advance!
Attachment #832626 - Attachment is obsolete: true
Attachment #8338097 - Flags: review?(hskupin)
Comment on attachment 8338097 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

Looks kinda good now! Thank you Jarek. There are only some minor things to update before the patch can be landed.

::: mozinstall/tests/Build_Stub/build.py
@@ +1,1 @@
> +#!/usr/bin/env python

Please keep lower-case letters for directory names.

@@ +6,5 @@
> +
> +import os
> +import subprocess
> +
> +STUB_DIR = os.path.abspath(os.path.split(__file__)[0])

Please call the variable ´here´ and I would use `os.path.dirname()` instead of `os.path.split()`.

@@ +11,5 @@
> +
> +
> +def compile_executables():
> +    '''Function compiles all assets connected to tests'''
> +    browser_exe = os.path.join(STUB_DIR, 'firefox.exe')

I would simply call it executable

@@ +19,5 @@
> +    stub_example_o = os.path.join(STUB_DIR, 'example_stub.o')
> +
> +    if os.path.exists(browser_exe):
> +        print "Removing existing browser stub: %s" % browser_exe
> +        os.unlink(browser_exe)

We don't use unlink across our projects. So please use `remove()`

@@ +29,5 @@
> +
> +        # And then we compile next version of browser file which
> +        # contains tested PE Header ('BuidID')
> +        ['gcc', '--output=%s' % browser_exe, stub_example_c,
> +            stub_example_o],

What is the requirement for gcc? Can't we compile it with the vc compiler?

@@ +35,5 @@
> +
> +    for command in commands:
> +        print "Executing command: %s" % (' '.join(command),)
> +        process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
> +            cwd=STUB_DIR)

I think it would be good to use mozprocess here.

::: mozinstall/tests/Build_Stub/example_stub.c
@@ +1,1 @@
> +/* We need this file to generate mock files for tests. */

Better lets rename the files to firefox.c and firefox.rc so they match the same filename with the executable.

::: mozinstall/tests/Build_Stub/example_stub.rc
@@ +4,5 @@
> +FILETYPE 0x1L
> +FILESUBTYPE 0x0L
> +{
> +    BLOCK "StringFileInfo"
> +    { 

nit: trailing white-space
Attachment #8338097 - Flags: review?(hskupin) → review-
Hi Jarek, when do you think you can do the remaining updates for this patch?
I have it almost done - i just must only figure out some things in Visual Studio to make this work. I hope i'll be able to do this during tomorrow or sunday. Sorry for such delay - as i've described in different bug, i have a little stormy situation. Dayjob has overtaking near all of my time lately. But i want to finish this. I'll try to update you more often.
:whimboo

Sorry again, i'll try to end this task till end this week. If you have someone which could faster than i can hand over this task.
Flags: needinfo?(hskupin)
No, it's fine. Thanks for the information anyway.
Flags: needinfo?(hskupin)
Okay, new version of patch.
Attachment #8338097 - Attachment is obsolete: true
Attachment #8348375 - Flags: review?(hskupin)
Comment on attachment 8348375 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

I think we nearly have it. It looks all fine! A couple of questions and nits remain. Also please attach the pre-build firefox.exe to the patch. The build process should only be run when a change to the pe file is necessary.

::: mozinstall/tests/build_stub/build.py
@@ +20,5 @@
> +    stub_example_obj = os.path.join(here, 'firefox.obj')
> +
> +    if os.path.exists(executable):
> +        print "Removing existing browser stub: %s" % executable
> +        os.remove(executable)

I would make use of mozfile.remove() here.

@@ +28,5 @@
> +        # because is_installer needs to check PE Headers
> +        ['rc.exe', stub_example_rc],
> +
> +        # And then we compile next version of browser file which
> +        # contains tested PE Header ('BuidID')

nit: Spelling mistake. This should be 'BuildID'

@@ +35,5 @@
> +
> +    for command in commands:
> +        print "Executing command: %s" % (' '.join(command),)
> +        process = ProcessHandler(command)
> +        process.run()

I would give it a reasonable timeout.

@@ +36,5 @@
> +    for command in commands:
> +        print "Executing command: %s" % (' '.join(command),)
> +        process = ProcessHandler(command)
> +        process.run()
> +        returncode = process.wait()

In mozrunner we use `self.process_handler.proc.poll()` to get the returncode. Is it not necessary here?

@@ +40,5 @@
> +        returncode = process.wait()
> +        if returncode:
> +            output = '\r\n'.join(process.output)
> +            print "%s: exit %d" % (command, returncode)
> +            print output

Do we have to print the output here? Isn't it enough to push it into the exception for later usage?

@@ +44,5 @@
> +            print output
> +            raise subprocess.CalledProcessError(returncode, command, output)
> +    print "Removing temporary files: %s %s" % (stub_example_res, stub_example_obj)
> +    os.remove(stub_example_res)
> +    os.remove(stub_example_obj)

Same here for mozfile.remove(). And please add an empty line before the print line.

::: mozinstall/tests/build_stub/firefox.c
@@ +1,1 @@
> +/* We need this file to generate mock files for tests. */

Also put a license in?

@@ +1,4 @@
> +/* We need this file to generate mock files for tests. */
> +
> +int main() {
> +	return 0;

nit: 4 chars of indentation should be enough.

::: mozinstall/tests/test.py
@@ +81,5 @@
>              self.assertTrue(mozinstall.is_installer(self.exe))
>  
> +            # test stub browser file
> +            stub_exe = os.path.join(here, 'build_stub', 'firefox.exe')
> +            self.assertFalse(mozinstall.is_installer(stub_exe))

I think the whole test needs refactoring but that should indeed be done later.
Attachment #8348375 - Flags: review?(hskupin) → review-
> In mozrunner we use `self.process_handler.proc.poll()` to get the returncode. Is it not necessary here?
From what i see wait returns return code of ran command. I didn't want to violate Law of Demeter.
New version of patch. Please, review again :)
Attachment #8348375 - Attachment is obsolete: true
Attachment #8351523 - Flags: review?(hskupin)
Comment on attachment 8351523 [details] [diff] [review]
0001-Bug-795288-Extended-checking-inside-is_installed-to-.patch

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

The patch no longer applies to master. So it needs an update before I can do the review and test it.

::: mozinstall/tests/build_stub/build.py
@@ +5,5 @@
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import os
> +import subprocess
> +import mozfile

As usual we should separate non-default packages from system-wide ones.
Attachment #8351523 - Flags: review?(hskupin) → review-
:whimboo

Okay, i've updated patch.
Attachment #8351523 - Attachment is obsolete: true
Attachment #8359451 - Flags: review?(hskupin)
Now that bug 950227 has been fixed, I will have a look at this patch.
Depends on: 950227
Comment on attachment 8359451 [details] [diff] [review]
0001-Bug-795288-support-for-PE-headers-in-windows-version.patch

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

r- because of the missing setup.py file. Otherwise minor updates for the patch. It looks to be ready to get landed with the next update. Thanks Jarek!

::: mozinstall/mozinstall/mozinstall.py
@@ +18,4 @@
>  import mozfile
>  import mozinfo
>  
> +import pefile

I don't see a dependency added in setup.py, so it will fail to import this module. Did you accidentally put this file out of the patch?

::: mozinstall/tests/build_stub/build.py
@@ +36,5 @@
> +    ]
> +
> +    for command in commands:
> +        print "Executing command: %s" % (' '.join(command),)
> +        process = ProcessHandler(command)

In case of the commands cannot be found, shall we handle the OSError and mention that the dev tools have to be installed?

@@ +37,5 @@
> +
> +    for command in commands:
> +        print "Executing command: %s" % (' '.join(command),)
> +        process = ProcessHandler(command)
> +        process.run(timeout=20)

I assume the 20s will be long enough even on slower machines. :)

::: mozinstall/tests/build_stub/firefox.c
@@ +1,1 @@
> +/* 

nit: trailing space
Attachment #8359451 - Flags: review?(hskupin) → review-
:whimboo
Updated :-)
Attachment #8359451 - Attachment is obsolete: true
Attachment #8361964 - Flags: review?(hskupin)
Comment on attachment 8361964 [details] [diff] [review]
0001-Bug-795288-support-for-PE-headers-in-windows-version.patch

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

Looks fine to me. I will go ahead and merge the patch with an updated commit message. Thanks Jarek!

::: mozinstall/setup.py
@@ +15,5 @@
>  
>  deps = ['mozinfo >= 0.7',
> +        'mozfile >= 1.0',
> +        'mozprocess >= 0.15',
> +        'pefile >= 1.2.10'

So you were going to drop the minor version number after the dash? I assume it's not necessary.

::: mozinstall/tests/build_stub/build.py
@@ +46,5 @@
> +            if returncode:
> +                raise subprocess.CalledProcessError(returncode, command, output)
> +
> +        except OSError, e:
> +            if e.errno == errno.ENOENT:

It's good that we found out about bug 961235.
Attachment #8361964 - Flags: review?(hskupin) → review+
Landed on master:
https://github.com/mozilla/mozbase/commit/2b8cb2ad66d4fde27a9583794af0c201021d9c1c

Next thing I will review is bug 911048 so we can also get stub installer support landed. Thanks Jarek for working on this feature!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Depends on: 963176
Blocks: 969276
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: