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

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: Nat, Assigned: gps)

Tracking

unspecified
mozilla51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

3 years ago
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.
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.
Reporter

Updated

3 years ago
Assignee: nobody → nhakkakzadeh
Status: NEW → ASSIGNED
Reporter

Updated

3 years ago
Assignee: nate.hak → nobody
Status: ASSIGNED → NEW
Assignee

Comment 2

3 years ago
I'm gonna take a stab at this.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

3 years ago
mozreview-review
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).
Attachment #8781318 - Flags: review?(mh+mozilla) → review+

Comment 6

3 years ago
mozreview-review
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.
Attachment #8781319 - Flags: review?(mh+mozilla) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 9

3 years ago
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.
Attachment #8781319 - Flags: review+ → review?(mh+mozilla)

Comment 10

3 years ago
mozreview-review
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.
Attachment #8781319 - Flags: review?(mh+mozilla) → review+
Comment hidden (mozreview-request)

Comment 12

3 years ago
Pushed by gszorc@mozilla.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0253b9b9684a
https://hg.mozilla.org/mozilla-central/rev/c89c51c5414d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.