1291944 - Configure chooses 64-bit makensis instead of the 32-bit makensis

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nathan Hakkakzadeh [:Nat]]

On Windows, configure may choose a 64-bit version of makensis if it is in the PATH. This is problematic since an NSIS plugin we rely on is 32-bit making compiling the installer for Firefox impossible.

We should ensure that configure does NOT choose a 64-bit version of makensis.

Comment 1

[Mike Hommey [:glandium]]

I think there are two parts here:
- one is that configure should try to find a 32-bits version of makensis if it can find one on its own (by checking in /ming32/bin even if it's not in PATH, for example ; there's a complication, though: /mingw32/bin is a msys path, configure don't support those)
- and configure should check that once it found an makensis, it is 32-bits.

Comment 2

[Gregory Szorc [:gps]]

I'm gonna take a stab at this.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1291944 - Search for nsis in msys environment;

Previously, configure found nsis on PATH, likely from MozillaBuild.
In our msys2 environment, nsis is installed under /mingw32. We
supplement moz.configure to look for nsis in this location.

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

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1291944 - Verify makensis binary is 32-bits;

This required implementing a utility function to resolve the binary
type. I used GetBinaryTypeW via ctypes because this seems the fastest.
I arbitrarily limited the function to testing 32-bit and 64-bit Windows
executables because hopefully those are the only executables we'll
ever encounter. We can expand the binary detection later, if needed.
This includes support for running on non-Windows platforms.

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

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8781318 [details]
Bug 1291944 - Search for nsis in msys environment;

https://reviewboard.mozilla.org/r/71728/#review69272

::: moz.configure:273
(Diff revision 1)
> +    # over a 64-bit exe that may be in PATH.
> +    if 'MSYSTEM_PREFIX' in os.environ:
> +        prefix = os.path.dirname(os.environ['MSYSTEM_PREFIX'])
> +        candidates.insert(0, os.path.join(prefix, 'mingw32', 'bin', 'makensis.exe'))
> +
> +    return candidates

not that it makes a lot of difference, but please return a tuple here (immutable results are better).

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8781319 [details]
Bug 1291944 - Verify makensis binary is 32-bits;

https://reviewboard.mozilla.org/r/71730/#review69274

::: build/moz.configure/util.configure:82
(Diff revision 1)
> +        return '32-bit'
> +    elif bin_type.value == 6:
> +        return '64-bit'

It might be desirable to return a mozbuild.util.EnumString subclass instance.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8781318 [details]
Bug 1291944 - Search for nsis in msys environment;

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

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8781319 [details]
Bug 1291944 - Verify makensis binary is 32-bits;

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

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8781319 [details]
Bug 1291944 - Verify makensis binary is 32-bits;

Please take another look at this since I want to be sure you agree with the EnumString usage.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8781319 [details]
Bug 1291944 - Verify makensis binary is 32-bits;

https://reviewboard.mozilla.org/r/71730/#review69286

::: moz.configure:304
(Diff revision 2)
> +@depends_if(nsis)
> +@checking('for 32-bit NSIS')
> +@imports(_from='mozbuild.configure.constants', _import='BinaryType')
> +def nsis_binary_type(nsis):
> +    bin_type = windows_binary_type(nsis)
> +    if bin_type != BinaryType('win32'):

You only need to compare with 'win32' (so you don't need to import BinaryType here either).

::: python/mozbuild/mozbuild/configure/constants.py:66
(Diff revision 2)
>  Endianness = EnumString.subclass(
>      'big',
>      'little',
>  )
>  
> +BinaryType = EnumString.subclass(

Let's make that a WindowsBinaryType for now.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8781319 [details]
Bug 1291944 - Verify makensis binary is 32-bits;

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0253b9b9684a
Search for nsis in msys environment; r=glandium
https://hg.mozilla.org/integration/autoland/rev/c89c51c5414d
Verify makensis binary is 32-bits; r=glandium

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0253b9b9684a
https://hg.mozilla.org/mozilla-central/rev/c89c51c5414d