Closed Bug 884828 Opened 7 years ago Closed 2 months ago

mozinstall.get_binary() should check for executable bit and be less confusing

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: ahal, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

https://tbpl.mozilla.org/php/getParsedLog.php?id=24326891&tree=Cedar

A quick work around would be to set the "application" in the mozharness config to "run-emulator.sh", but I believe mozinstall needs some fixing.

1) there should be an option to not bother checking for a binary (maybe if --app isn't specified?)
2) perhaps change "app == filename" to "app in filename"?
3) "app" is kind of confusing, maybe "binary_hint" would be better
4) https://github.com/mozilla/mozbase/blob/master/mozinstall/mozinstall/mozinstall.py#L78 should run an os.stat to check for the executable bit
Blocks: 872765
This doesn't really block bug 872765, but imo we should still look into it, morphing title to reflect that. I don't think it's a big priority so would make a good first bug.

To work on this bug you'll need to clone mozbase (https://github.com/mozilla/mozbase) and edit mozinstall to use os.stat and touch up the other improvements in comment 0. I think point #2 can be ignored for now as I'm not really sure it's a good idea. If interested on working on this and need more help feel free to ping ahal on irc.mozilla.org.
No longer blocks: 872765
Summary: Mozinstall "does not contain valid binary" for b2g full emulator unittests → mozinstall.get_binary() should check for executable bit and be less confusing
Whiteboard: [good first bug][mentor=ahal]
Depends on: 888756
Hey ahal, I'm interested in working on this. I've already cloned the repo on Github and am taking a look at the code to see how I want to approach this. I'll ping on IRC if I need some help.
Hi Divij, thanks for your interest. I'll needinfo Andrew so that this pops up in his queue.  If you have any questions hop on to irc.mozilla.org and ask in #ateam several people in that channel can likely answer them.
Flags: needinfo?(ahalberstadt)
(In reply to Divij Rajkumar from comment #2)
> Hey ahal, I'm interested in working on this. I've already cloned the repo on
> Github and am taking a look at the code to see how I want to approach this.
> I'll ping on IRC if I need some help.

Hi Divij, thanks for your interest! Let me know if anything isn't clear
Flags: needinfo?(ahalberstadt)
Assignee: nobody → divijrajkumar
Status: NEW → ASSIGNED
Attached patch First pass at fix for #884828 (obsolete) — Splinter Review
Added a first pass at fixing the bug. Here's what changed:

1. Change --app to --binary_hint
2. Skip get_binary if --binary_hint not provided
3. Modify Mac logic in get_binary to check if there is a valid name in Info.plist and it matches the provided binary_hint
4. Modify Windows/Linux logic in get_binary to check executable bit

I wasn't able to test on Linux or Windows since I don't have either of those environments, so if someone could help out there, that would be awesome. I did test every possible case on my Mac and all scenarios have the expected output.
Attachment #775836 - Flags: review?
Comment on attachment 775836 [details] [diff] [review]
First pass at fix for #884828

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

Great start! I have to r-, mostly just for minor nitpicky details (don't worry about getting an r-, that's normal). Thanks for the patch and I look forward to seeing a follow-up :)

::: mozinstall/mozinstall/mozinstall.py
@@ +48,4 @@
>      """Thrown when uninstallation fails. Includes traceback if available."""
>  
>  
> +def get_binary(path, binary_hint_name):

Let's just call this binary_hint

@@ +64,1 @@
>          plist = '%s/Contents/Info.plist' % path

I know this wasn't you, but let's use os.path.join here. Might as well fix it up if you're in the area anyway.

@@ +66,5 @@
>  
> +        # Handle case where Info.plist has bad CFBundleExecutable value
> +        binary_name = readPlist(plist)['CFBundleExecutable']
> +        if not binary_name:
> +            raise InvalidBinary("%s has no valid binary name" % plist)

Hmm, I know I said we should add this in, but I just noticed that there's an 'if not binary' block down below. I think that takes care of the case where a binary isn't found on OSX

@@ +68,5 @@
> +        binary_name = readPlist(plist)['CFBundleExecutable']
> +        if not binary_name:
> +            raise InvalidBinary("%s has no valid binary name" % plist)
> +        elif not binary_name == binary_hint_name:
> +            raise InvalidBinary("Binary name in %s doesn't match binary_hint" % plist)

I think OSX app bundles explicitly specify the binary with 'CFBundleExecutable' (whereas other platforms it's just a zip file we're dealing with, so we need some kind of hint). So we shouldn't force the user to pass in a binary_hint if we don't need to. So let's just remove this check completely.

@@ +81,5 @@
>  
>          for root, dirs, files in os.walk(path):
>              for filename in files:
>                  # os.access evaluates to False for some reason, so not using it
> +                filepath = root+"/"+filename

Need to use os.path.join here, otherwise it could fail on Windows

@@ +82,5 @@
>          for root, dirs, files in os.walk(path):
>              for filename in files:
>                  # os.access evaluates to False for some reason, so not using it
> +                filepath = root+"/"+filename
> +                if ((filename.lower() == binary_hint_name) and 

nit: extra whitespace

@@ +83,5 @@
>              for filename in files:
>                  # os.access evaluates to False for some reason, so not using it
> +                filepath = root+"/"+filename
> +                if ((filename.lower() == binary_hint_name) and 
> +                    (stat.S_IXUSR & os.stat(filepath)[stat.ST_MODE])):

I think you can use "os.access(fpath, os.X_OK)" here. Slightly more readable.

@@ +291,4 @@
>                        default=os.getcwd(),
>                        help='Directory to install application into. '
>                             '[default: "%default"]')
> +    parser.add_option('--binary_hint', dest='binary_hint',

We use '-' characters to separate command lines, so make this --binary-hint and add a default=None. Please also remove the --app option.

@@ +309,5 @@
> +        if os.path.isdir(src):
> +            binary = get_binary(src, binary_hint_name=options.binary_hint)
> +        else:
> +            install_path = install(src, options.dest)
> +            binary = get_binary(install_path, binary_hint_name=options.binary_hint)

I think this whole chunk could be simplified to:
if not os.path.isdir(src):
    install()
if options.binary_hint is not None:
    get_binary()
Attachment #775836 - Flags: review? → review-
(In reply to Andrew Halberstadt [:ahal] from comment #6)
> Comment on attachment 775836 [details] [diff] [review]
> First pass at fix for #884828
> 
> Review of attachment 775836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great start! I have to r-, mostly just for minor nitpicky details (don't
> worry about getting an r-, that's normal). Thanks for the patch and I look
> forward to seeing a follow-up :)
> 
> ::: mozinstall/mozinstall/mozinstall.py
> @@ +48,4 @@
> >      """Thrown when uninstallation fails. Includes traceback if available."""
> >  
> >  
> > +def get_binary(path, binary_hint_name):
> 
> Let's just call this binary_hint
> 
> @@ +64,1 @@
> >          plist = '%s/Contents/Info.plist' % path
> 
> I know this wasn't you, but let's use os.path.join here. Might as well fix
> it up if you're in the area anyway.
> 
> @@ +66,5 @@
> >  
> > +        # Handle case where Info.plist has bad CFBundleExecutable value
> > +        binary_name = readPlist(plist)['CFBundleExecutable']
> > +        if not binary_name:
> > +            raise InvalidBinary("%s has no valid binary name" % plist)
> 
> Hmm, I know I said we should add this in, but I just noticed that there's an
> 'if not binary' block down below. I think that takes care of the case where
> a binary isn't found on OSX
> 

The reason I added this here is because in the earlier version of this file, binary was being assigned a value:

binary = os.path.join(path, 'Contents/MacOS/', readPlist(plist)['CFBundleExecutable'])

So even if there was no binary found (i.e. CFBundleExecutable was empty), binary would still have "$path/Contents/MacOS/", and would never pass 'if not binary' since the string would never be empty.

> @@ +68,5 @@
> > +        binary_name = readPlist(plist)['CFBundleExecutable']
> > +        if not binary_name:
> > +            raise InvalidBinary("%s has no valid binary name" % plist)
> > +        elif not binary_name == binary_hint_name:
> > +            raise InvalidBinary("Binary name in %s doesn't match binary_hint" % plist)
> 
> I think OSX app bundles explicitly specify the binary with
> 'CFBundleExecutable' (whereas other platforms it's just a zip file we're
> dealing with, so we need some kind of hint). So we shouldn't force the user
> to pass in a binary_hint if we don't need to. So let's just remove this
> check completely.
> 
> @@ +81,5 @@
> >  
> >          for root, dirs, files in os.walk(path):
> >              for filename in files:
> >                  # os.access evaluates to False for some reason, so not using it
> > +                filepath = root+"/"+filename
> 
> Need to use os.path.join here, otherwise it could fail on Windows
> 
> @@ +82,5 @@
> >          for root, dirs, files in os.walk(path):
> >              for filename in files:
> >                  # os.access evaluates to False for some reason, so not using it
> > +                filepath = root+"/"+filename
> > +                if ((filename.lower() == binary_hint_name) and 
> 
> nit: extra whitespace
> 
> @@ +83,5 @@
> >              for filename in files:
> >                  # os.access evaluates to False for some reason, so not using it
> > +                filepath = root+"/"+filename
> > +                if ((filename.lower() == binary_hint_name) and 
> > +                    (stat.S_IXUSR & os.stat(filepath)[stat.ST_MODE])):
> 
> I think you can use "os.access(fpath, os.X_OK)" here. Slightly more readable.
> 
> @@ +291,4 @@
> >                        default=os.getcwd(),
> >                        help='Directory to install application into. '
> >                             '[default: "%default"]')
> > +    parser.add_option('--binary_hint', dest='binary_hint',
> 
> We use '-' characters to separate command lines, so make this --binary-hint
> and add a default=None. Please also remove the --app option.
> 
> @@ +309,5 @@
> > +        if os.path.isdir(src):
> > +            binary = get_binary(src, binary_hint_name=options.binary_hint)
> > +        else:
> > +            install_path = install(src, options.dest)
> > +            binary = get_binary(install_path, binary_hint_name=options.binary_hint)
> 
> I think this whole chunk could be simplified to:
> if not os.path.isdir(src):
>     install()
> if options.binary_hint is not None:
>     get_binary()



Thanks for looking over this! Small comment to add about the checking of binary name on OSX which I've written above. I'll get started on making the rest of the changes till then.
I have the changes ready for this, but I'm still not sure what to do with the `if not binary_name` condition in get_binary(). Can somebody chime in and let me know if having the condition there makes sense or not?

Here's the relevant part of the diff:

> @@ +66,5 @@
> >  
> > +        # Handle case where Info.plist has bad CFBundleExecutable value
> > +        binary_name = readPlist(plist)['CFBundleExecutable']
> > +        if not binary_name:
> > +            raise InvalidBinary("%s has no valid binary name" % plist)
> 
> Hmm, I know I said we should add this in, but I just noticed that there's an
> 'if not binary' block down below. I think that takes care of the case where
> a binary isn't found on OSX
> 

The reason I added this here is because in the earlier version of this file, binary was being assigned a value:

binary = os.path.join(path, 'Contents/MacOS/', readPlist(plist)['CFBundleExecutable'])

So even if there was no binary found (i.e. CFBundleExecutable was empty), binary would still have "$path/Contents/MacOS/", and would never pass 'if not binary' since the string would never be empty.
This flag is mandatory and has to be put into the plist file. Any application which is not doing that should be considered as broken. So I don't see when such a situation should ever happen.

For more information see:
https://developer.apple.com/library/ios/#documentation/General/Reference/InfoPlistKeyReference/Articles/CoreFoundationKeys.html#//apple_ref/doc/uid/20001431-101909
Made changes based on feedback from ahal and whimboo.
Attachment #775836 - Attachment is obsolete: true
Attachment #779543 - Flags: review?
Comment on attachment 779543 [details] [diff] [review]
Second attempt based on feedback

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

For future reference, you should add me (or whoever is doing the review) in the review field. You can put :ahal and it will know who I am. That way it'll show up in my e-mail :)

Anyway, thanks this looks good! I'm giving this a conditional r+ because there is one problem still. If you want you can fix and upload a new patch, otherwise I'll make the fix before I check it in.

::: mozinstall/mozinstall/mozinstall.py
@@ +298,3 @@
>  
> +    if options.binary_hint is not None:
> +        get_binary(src, binary_hint=options.binary_hint)   

nit: extra whitespace

@@ +301,2 @@
>  
> +    print binary

You'll need to add "print get_binary(...)" here and remove the print below it.
Attachment #779543 - Flags: review? → review+
Needinfo'ing myself so this stays on my radar.
Flags: needinfo?(ahalberstadt)
Henrik, I know you use mozinstall in a variety of places. Do you have any objections to the above patch landing (with my proposed fixes in comment 11)? It'll break backwards compatibility, so what modules that you know of would need to be updated?
Flags: needinfo?(hskupin)
Comment on attachment 779543 [details] [diff] [review]
Second attempt based on feedback

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

Andrew, I don't think that any of our consumers would have to be updated. The changes are internally and only improve how to find the binary. Beside that I made a couple of comments on the current code. Those lines are not clear to me and I would like to get some feedback for.

::: mozinstall/mozinstall/mozinstall.py
@@ +75,5 @@
>          for root, dirs, files in os.walk(path):
>              for filename in files:
>                  # os.access evaluates to False for some reason, so not using it
> +                filepath = os.path.join(root, filename)
> +                if os.access(filepath, os.X_OK):

Why don't we no longer check for the binary_hin (app_name) here? There can be multiple executables in the binary folder, e.g. updater.exe and xulrunner.exe. In such a case get_binary() would return updater.exe instead of the real application binary. Or do I miss something?

@@ +284,5 @@
>                        help='Directory to install application into. '
>                             '[default: "%default"]')
> +    parser.add_option('--binary-hint', dest='binary_hint',
> +                      default=None,
> +                      help='Application being installed. [default: "%default"]')

Why have we removed firefox to be the default? If we don't want a default remove the parameter and the help content.

@@ +293,5 @@
>  
>      src = args[0]
>  
> +    if not os.path.isdir(src):
> +        src = install(src, options.dest)

I would propose not to overwrite ´src´ here. The variable's name should better be ´dest´.
Flags: needinfo?(hskupin)
This is still on my radar. The reason I haven't landed it yet is because this will break mozharness (and thus every test job that uses it): https://hg.mozilla.org/build/mozharness/file/008b723bdd3a/mozharness/mozilla/testing/testbase.py#l275

We'll need to coordinate landing this with updating mozharness at the same time. I just haven't had time to do this yet. Divij, I do appreciate the patch, we'll get this landed eventually.
Flags: needinfo?(ahalberstadt)
can we land this?
Flags: needinfo?(ahalberstadt)
Not until we fix mozharness, which means putting in a clever hack, landing this on every branch that runs tests, or moving the mozharness bits that use this in-tree.

None of those options are super easy, and all come with significant risk of burning branches, which is why I've been hesitant to land it.
Flags: needinfo?(ahalberstadt)
Mentor: ahalberstadt
Whiteboard: [good first bug][mentor=ahal] → [good first bug]
if we cannot land this, should this be a good first bug?
Flags: needinfo?(ahalberstadt)
Probably not. Either way there is already a patch, so I'll remove the tag.
Flags: needinfo?(ahalberstadt)
Whiteboard: [good first bug]
reading this it appears we need a mozharness fix, is there a bug for that?  I assume the mozharness pinning which should be rolling out in the coming weeks would allow us to solve this?

Right now this is an assigned bug, if there is no action to take (and hasn't been for 6+ months), we should consider unassigning it and removing the mentor.

Not sure if this is still an issue. I'm going to guess not.

Assignee: divijrajkumar → nobody
Mentor: ahal
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.