1073697 - mozversion should accept binary_path without .exe extension on windows

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

On e.g. talos (bug 1068989) we want to pass a browser path without the .exe extension. mozversion should be ok with that.

Comment 1

[William Lachance (:wlach)]

Attachment: Quick patch to allow not specifying .exe extension

Comment 2

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

Comment on attachment 8496262 [details] [diff] [review]
Quick patch to allow not specifying .exe extension

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

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +95,5 @@
>              if not os.path.exists(binary):
> +                # on windows we should also try extending the binary path with
> +                # .exe and see if that's valid
> +                if sys.platform != 'win32' or not binary.endswith('.exe') \
> +                   or not os.path.exists(binary + '.exe')):

nit: For visualization this line should better have an indentation by 8 chars. Here we make even use of 3, which is totally unusual but still allowed?

Comment 3

[William Lachance (:wlach)]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 8496262 [details] [diff] [review]
> Quick patch to allow not specifying .exe extension
> 
> Review of attachment 8496262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mozbase/mozversion/mozversion/mozversion.py
> @@ +95,5 @@
> >              if not os.path.exists(binary):
> > +                # on windows we should also try extending the binary path with
> > +                # .exe and see if that's valid
> > +                if sys.platform != 'win32' or not binary.endswith('.exe') \
> > +                   or not os.path.exists(binary + '.exe')):
> 
> nit: For visualization this line should better have an indentation by 8
> chars. Here we make even use of 3, which is totally unusual but still
> allowed?

I think it is allowed, at least my auto-indenter and python interpreter thinks so. :)

But according to pep8, it's better to wrap the line in parantheses here, so I'll change the patch to do that: http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length

Comment 4

[William Lachance (:wlach)]

Pushed. I realized just after my first push that there was a logic error which could cause problems (basically we would unconditionally accept it if not under win32), so I did a quick followup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6abcab74130a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8be8d3dd116b

Note that mozversion is not currently used in the build so there should be build breakage caused by either patch.

I also updated the mozversion version to 0.8 and did a new release on pypi:

https://pypi.python.org/packages/source/m/mozversion/mozversion-0.8.tar.gz#md5=6f581d90b4eab74bf420c7aee9dccf2f

Comment 5

[William Lachance (:wlach)]

Attachment: Fixed patch with unit test

So I had a bad time landing this. The patch that got r+'ed had an error in it. I noticed the problem then sent a patch to fix things up, but I pressed CTRL-C because I had second thoughts at the last minute which meant it never appeared in the pushlog. 

Anyway, they're both backed out now and I think I'm going to ask for for an r? just for sober second thought before I try this again. I added a new unit test and verified that the old one passes.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8497121 [details] [diff] [review]
Fixed patch with unit test

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

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +95,5 @@
> +            # if not specified, also try extending the binary path with .exe
> +            # and see if that's valid
> +            if (not os.path.exists(binary) and
> +                (not binary.endswith('.exe') and
> +                 not os.path.exists(binary + '.exe'))):

Sorry for not catching the previous issue. Is the endswith check necessary here? We could just test binary + '.exe', which should do no harm if the binary already ends with .exe, but would make this condition a little easier to read.

::: testing/mozbase/mozversion/tests/test_binary.py
@@ +94,5 @@
> +        exe_name_unprefixed = self.binary + '1'
> +        exe_name = exe_name_unprefixed + '.exe'
> +        with open(exe_name, 'w') as f:
> +            f.write('foobar')
> +        self._check_version(get_version(exe_name_unprefixed))

Could we add another check_version using exe_name to check when a full binary path is provided?

Comment 7

[William Lachance (:wlach)]

Attachment: Take 3

Agreed on simplifying the clause, it doesn't really matter if we allow firefox.exe.exe. :)

I didn't add an extra test like you mention because that would basically be just duplicating other unit tests. Thoughts?

Comment 8

[William Lachance (:wlach)]

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b212e3c4cbf5

Comment 9

[Ed Morley [:emorley]]

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