mozversion should accept binary_path without .exe extension on windows

RESOLVED FIXED in mozilla35

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: wlach, Assigned: wlach)

Tracking

Trunk
mozilla35
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

On e.g. talos (bug 1068989) we want to pass a browser path without the .exe extension. mozversion should be ok with that.
Attachment #8496262 - Flags: review?(dave.hunt) → review+
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?
(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
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
Posted patch Fixed patch with unit test (obsolete) — Splinter Review
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.
Attachment #8496262 - Attachment is obsolete: true
Attachment #8497121 - Flags: review?(dave.hunt)
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?
Attachment #8497121 - Flags: review?(dave.hunt) → review-
Posted patch Take 3Splinter Review
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?
Attachment #8497121 - Attachment is obsolete: true
Attachment #8497571 - Flags: review?(dave.hunt)
Attachment #8497571 - Flags: review?(dave.hunt) → review+
https://hg.mozilla.org/mozilla-central/rev/b212e3c4cbf5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.