1280220 - find_program should append an exe extension to absolute paths on Windows

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

This works in autoconf but not moz.configure.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: bug 1280220 - find_program should append an exe extension to absolute paths on Windows.

Review commit: https://reviewboard.mozilla.org/r/59352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59352/

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8762897 [details]
bug 1280220 - find_program should append an exe extension to absolute paths on Windows.

https://reviewboard.mozilla.org/r/59352/#review56370

::: python/mozbuild/mozbuild/test/configure/test_checks_configure.py:28
(Diff revision 1)
>  from common import ConfigureTestSandbox
>  
>  
> +def ensure_exe_extension(path):
> +    if sys.platform.startswith('win') and \
> +       os.path.splitext(path)[1].lower() != '.exe':

You could remove this part of the test.

::: python/mozbuild/mozbuild/test/configure/test_checks_configure.py:229
(Diff revision 1)
> +        self.assertEqual(config, {'FOO': self.KNOWN_A})
> +        self.assertEqual(out, 'checking for foo... %s\n' % self.KNOWN_A)
> +
> +        config, out, status = self.get_result(
> +            'check_prog("FOO", ("unknown", "known-b", "known c"))',
> +            ['FOO=%s' % os.path.splitext(self.KNOWN_A)[0]])

It would be better to have another test with FOO=self.KNOWN_A (the full path *with*  .exe extension)

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8762897 [details]
bug 1280220 - find_program should append an exe extension to absolute paths on Windows.

https://reviewboard.mozilla.org/r/59352/#review56374

::: build/moz.configure/util.configure:75
(Diff revision 1)
>  @imports('itertools')
> +@imports('sys')
>  @imports(_from='os', _import='pathsep')
>  def find_program(file, paths=None):
>      if is_absolute_or_relative(file):
> +        if sys.platform.startswith('win') and os.path.splitext(file)[1].lower() != '.exe':

note that which actually does more than this:

https://dxr.mozilla.org/mozilla-central/source/python/which/which.py#169

Maybe we could simply replace the absolute path case with which.which(os.path.basename(path), os.path.dirname(path))

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8762897 [details]
bug 1280220 - find_program should append an exe extension to absolute paths on Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59352/diff/1-2/

Comment 5

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/59352/#review56374

> note that which actually does more than this:
> 
> https://dxr.mozilla.org/mozilla-central/source/python/which/which.py#169
> 
> Maybe we could simply replace the absolute path case with which.which(os.path.basename(path), os.path.dirname(path))

I don't know if the variable-extension support is actually useful, but just re-using which there is a great idea!

Comment 6

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/59352/#review56370

> It would be better to have another test with FOO=self.KNOWN_A (the full path *with*  .exe extension)

That's already covered in `test_check_prog_with_args` below. I just wanted to ensure we covered the different cases: just a filename passed, with an exe extension, and a full path passed without an exe extension (which is the bug I actually fixed).

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8762897 [details]
bug 1280220 - find_program should append an exe extension to absolute paths on Windows.

https://reviewboard.mozilla.org/r/59352/#review56538

::: build/moz.configure/util.configure:75
(Diff revision 2)
>  def find_program(file, paths=None):
> -    if is_absolute_or_relative(file):
> -        return os.path.abspath(file) if os.path.isfile(file) else None
>      try:
> +        if is_absolute_or_relative(file):
> +            return normsep(which(os.path.basename(file),

If the file doesn't exit, this is going to throw, isn't it?

Comment 8

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/59352/#review56538

> If the file doesn't exit, this is going to throw, isn't it?

If the file doesn't exist which raises a `WhichError`, which we're already catching, so the function will return `None`.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8762897 [details]
bug 1280220 - find_program should append an exe extension to absolute paths on Windows.

https://reviewboard.mozilla.org/r/59352/#review56548

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8762897 [details]
bug 1280220 - find_program should append an exe extension to absolute paths on Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59352/diff/2-3/

Comment 11

[(not currently active) Ted Mielczarek]

I was having trouble getting all the configure tests to pass. I wound up only having to make two simple changes to `ConfigureTestSandbox.which`--making it call `mozpath.abspath` so that the paths it returns are Windows-style on Windows, and having it check the filename with .exe to match the real `which` implementation.

Comment 12

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1fcf3bae37b9b9da2338fc0f97a91a7f990e3a
bug 1280220 - find_program should append an exe extension to absolute paths on Windows. r=glandium

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b1fcf3bae37