874476 - [mozrunner] Tilde character not expanded for application arguments

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Johannes Vogel (:nebelhom)]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130512194445

Steps to reproduce:

Used ~ character instead of an absolute path

e.g. ~/my/path/to/application


Actual results:

The path was not recognised.


Expected results:

It should be recognised ;)

--------------------------------------------
This is more a point of discussion than an actual bug. While fixing a different bug in mozbase, I stumbled over the above. Apparently, this is due to os.path.expanduser (http://docs.python.org/2/library/os.path.html?highlight=expanduser#os.path.expanduser) not being utilised.

Do you feel that this should be included? Is there a good reason why it isn't?

Comment 1

[Jeff Hammel]

I'm not sure what piece of mozbase software this is for.  Could you elaborate?

In general, while `foo bar=~/fleem` will not expand `foo bar ~/fleem` will.  I always use the latter so never notice these sort of things.

In general, if we're going to do this with path options, I'd prefer unifying on a way to do so.  mozoptions was proposed long ago and it'd be nice to leverage that instead of having extra code all over the place IMHO

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

In that case it was for specifying the application.ini file as --app-arg for mozrunner, so that you can start a XULRunner application like:

mozrunner --app-arg="~/test-app/application.ini" -b ~/test-app/xulrunner

Jeff, I hope it's ok when I set you as mentor for that.

Comment 3

[Jeff Hammel]

I'm not really sure what the scope of this bug so its hard for me to know what to mentor on.  So no, I don't feel qualified. Are we looking for step by step fixes that are basically copy+paste code? Are we looking for a singular instance of such a fix?  If so, what is the rationale of fixing on place vs the other potential paths? Are we looking to have centralized library code that will e.g. play with optparse or argparse that will expanduser for CLI paths?  Are we looking for a function in mozfile to do this?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Fredrik would like to work on this bug.

Comment 5

[Fredrik Broman [:bromaniac]]

I'm not sure how to reproduce this. When I try running

mozrunner --app-arg="~/test-app/application.ini" -b ~/test-app/xulrunner

I get:

OSError: Binary path does not exist: /home/vagrant/test-app/xulrunner

which makes sense since I don't have xulrunner on this vm BUT the point is that the ~ is expanded to the correct path to $HOME. I don't get "The path was not recognised." neither on OSX 10.9 nor Ubuntu 12.04.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

For this specific test you should use a real xulrunner build and the application as I will attach in a bit. Running this build via the command given should fail because the application.ini file is not found.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: xulapp.zip

Comment 8

[Fredrik Broman [:bromaniac]]

Attachment: bug874476.patch

I still can't reproduce the error but here's a patch anyway :)

mozrunner -b ~/test-app/XULTest/myapp.app/ --app-arg="~/test-app/application.ini"

before patch:

Starting: /Users/fredrikb/test-app/XULTest/myapp.app/Contents/MacOS/xulrunner ~/test-app/application.ini -foreground -profile /var/folders/j7/kzb2j2d91r93zyc5nyjx9n240000gn/T/tmpEDW3FC.mozrunner

after patch:

Starting: /Users/fredrikb/test-app/XULTest/myapp.app/Contents/MacOS/xulrunner /Users/fredrikb/test-app/application.ini -foreground -profile /var/folders/j7/kzb2j2d91r93zyc5nyjx9n240000gn/T/tmpw5wjGd.mozrunner

This is on OSX 10.9.2 with Python 2.7.5

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8392476 [details] [diff] [review]
bug874476.patch

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

I have just tested the patch and it works fantastic Fredrik! Great work. There is one small thing I would like to see. With that done you have my r+, but also please ask for feedback from :ahal once updated.

::: testing/mozbase/mozrunner/mozrunner/local.py
@@ +305,5 @@
>      ### methods for running
>  
>      def command_args(self):
>          """additional arguments for the mozilla application"""
> +        self.options.appArgs = map(os.path.expanduser, self.options.appArgs)

I would not modify self.options.appArgs but create a new variable here. That way we keep the original values.

Comment 10

[Fredrik Broman [:bromaniac]]

Attachment: bug874476.patch

+ Made the change suggested in the latest review
+ Updated docstring with comment about the code

Ran test.py before and after patch without problems. Reran the tests I mentioned in the last patch commit with the same result.

Also tested with an argument that isn't a path:
$ mozrunner -b ~/test-app/XULTest/myapp.app/ --app-arg="--test-ett foo"
Starting: /Users/fredrikb/test-app/XULTest/myapp.app/Contents/MacOS/xulrunner --test-ett foo -foreground -profile /var/folders/j7/kzb2j2d91r93zyc5nyjx9n240000gn/T/tmp0imzL5.mozrunner

No problem there either.

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8394291 [details] [diff] [review]
bug874476.patch

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

Functionally this looks good, but please fix the minor style comments I made below.

::: testing/mozbase/mozrunner/mozrunner/local.py
@@ +306,5 @@
>  
>      def command_args(self):
> +        """additional arguments for the mozilla application
> +
> +           Also expands ~ into users home directory path

I don't really think this comment is necessary.

@@ +307,5 @@
>      def command_args(self):
> +        """additional arguments for the mozilla application
> +
> +           Also expands ~ into users home directory path
> +           

nit: please remove this whitepsace

@@ +310,5 @@
> +           Also expands ~ into users home directory path
> +           
> +        """
> +        expanded_paths = map(os.path.expanduser, self.options.appArgs)
> +        return expanded_paths

nit: no need to store this in an expanded_paths variable, just return the result directly.

Comment 12

[Fredrik Broman [:bromaniac]]

Attachment: bug874476.patch

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 8394338 [details] [diff] [review]
bug874476.patch

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

Thanks!

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like we missed to land the patch here. I cannot do it right now because inbound is closed due to a bustage. Fredrik, I will land the patch early tomorrow.

Thanks a lot for working on it. If you want to work on something else please let me know. Also we have an automation training day tomorrow again, feel free to join us in #automation all day long.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de90dbe2204a

There is nothing we can test right now, but I will keep the in-testsuite flag so we can look for possibilities in the future.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de90dbe2204a