Move makensis test to python configure

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: glandium, Assigned: Nat)

Tracking

unspecified
mozilla51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

There is a mingw-w64-x86_64-nsis package in msys2 that we can install to fulfil the makensis requirement from configure.

The test in old-configure is broken, though, as it thinks 3.0rc1 is not newer than 3.0b1. Instead of fixing the horrible pile of shell that handles that, we should move the check to python configure and make it do the right thing (it would also be much simpler)

Nat, do you want to take this bug?
I got this.
Assignee: nobody → nhakkakzadeh
Status: NEW → ASSIGNED
Comment on attachment 8775820 [details]
Bug 1290044 - Moved NSIS configure to Python.

https://reviewboard.mozilla.org/r/67876/#review65078

::: build/moz.configure/windows.configure:178
(Diff revision 1)
>          result.append(
>              os.path.join(valid_windows_sdk_dir.path, 'bin', 'x86'))
>      return result
>  
>  
> +nsis = check_prog('MAKENSISU', ('makensis-3.0b3.exe', 'makensis-3.0b1.exe', 'makensis.exe', 'makensis'))

This file is only included on non-artifact builds, and I think we should still allow to build an installer out of an artifact build, so it would be better to move this block in another file. Maybe the top-level moz.configure.

::: build/moz.configure/windows.configure:181
(Diff revision 1)
>  
>  
> +nsis = check_prog('MAKENSISU', ('makensis-3.0b3.exe', 'makensis-3.0b1.exe', 'makensis.exe', 'makensis'))
> +
> +# Make sure the version of makensis is up to date.
> +@template

I'm not convinced it's worth using a template. I don't think we've made the effort to do so in other similar version testing cases (I'm thinking about yasm, for example)

::: build/moz.configure/windows.configure:191
(Diff revision 1)
> +    @imports('subprocess')
> +    def valid_nsis(nsis):
> +        try:
> +            release_order = ['', 'a', 'b', 'rc']
> +
> +            nsis_version_template = r'v(?P<major>[0-9]+)\.(?P<minor>[0-9]+)(?P<release>(a|b|rc)*)(?P<patch>[0-9]*)'

Like in many cases, there is a much simpler way to handle all this:
- first, you don't need to capture each part of the version in separate groups.
- Put the "v" in a positive lookbehind assertion (check out https://docs.python.org/2/library/re.html)
- once you have your version string extracted, you can get a LooseVersion-derived object by calling Version().
- once you have the Version instance, you can just compare it to '3.0b1' (like, literally, version >= '3.0b1'), and it will do the right thing.
Attachment #8775820 - Flags: review?(mh+mozilla)
Duplicate of this bug: 1275919
Comment on attachment 8775820 [details]
Bug 1290044 - Moved NSIS configure to Python.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67876/diff/1-2/
Attachment #8775820 - Flags: review?(mh+mozilla)
Comment on attachment 8775820 [details]
Bug 1290044 - Moved NSIS configure to Python.

https://reviewboard.mozilla.org/r/67876/#review65208

::: moz.configure:232
(Diff revision 2)
>                                      check_for_hunspell)
>  
>  set_config('MOZ_SYSTEM_HUNSPELL', system_hunspell)
>  
> +
> +nsis = check_prog('MAKENSISU', ('makensis-3.0b3.exe', 'makensis-3.0b1.exe', 'makensis.exe', 'makensis'))

Oh, we shouldn't do this check on non-windows, so please wrap the list of executables in a @depends function.

Also, toolchain.configure, and thus windows.configure, is not included on artifact builds, so you can't rely on depends_win below, so you'll want something like:

@depends(target)
def makensis_progs(target):
    if target.kernel == 'WINNT':
        return ('makensis-3.0b3.exe', ...)

nsis = check_prog('MAKENSISU', makensis_progs)

@depends(nsis)
@checking(...)
@imports(...)
def valid_nsis(nsis):
    ...

::: moz.configure:236
(Diff revision 2)
> +
> +nsis = check_prog('MAKENSISU', ('makensis-3.0b3.exe', 'makensis-3.0b1.exe', 'makensis.exe', 'makensis'))
> +
> +# Make sure the version of makensis is up to date.
> +@depends_win(nsis)
> +@checking('for NSIS version >= %s' % '3.0b1')

you can inline '3.0b1' in the string, it won't hurt.

::: moz.configure:242
(Diff revision 2)
> +@imports('re')
> +@imports('subprocess')
> +def valid_nsis(nsis):
> +    nsis_min_version = '3.0b1'
> +    try:
> +        out = subprocess.check_output([nsis, '-version'])

Sorry, I should have caught this in the previous round: we have a helper function in python configure to use instead of subprocess.check_output. Check out what yasm_version does in toolchain.configure.

::: moz.configure:243
(Diff revision 2)
> +@imports('subprocess')
> +def valid_nsis(nsis):
> +    nsis_min_version = '3.0b1'
> +    try:
> +        out = subprocess.check_output([nsis, '-version'])
> +        m = re.search(r'(?<=v)[0-9]+\.[0-9]+(a|b|rc)*[0-9]*', out)

((a|b|rc)[0-9]+)?

would be better (and you can use "(?:" instead of "(", too, to avoid a useless capture)

::: moz.configure:245
(Diff revision 2)
> +    nsis_min_version = '3.0b1'
> +    try:
> +        out = subprocess.check_output([nsis, '-version'])
> +        m = re.search(r'(?<=v)[0-9]+\.[0-9]+(a|b|rc)*[0-9]*', out)
> +
> +        out_of_date_msg = 'To build the installer you must have NSIS version %s or greater in your path' % nsis_min_version

You should just move this string in the FatalCheckError instantiation.

::: moz.configure:248
(Diff revision 2)
> +        m = re.search(r'(?<=v)[0-9]+\.[0-9]+(a|b|rc)*[0-9]*', out)
> +
> +        out_of_date_msg = 'To build the installer you must have NSIS version %s or greater in your path' % nsis_min_version
> +        if not m:
> +            raise FatalCheckError('Unknown version of makensis')
> +        ver = Version(m.string.lstrip('v'))

You should use m.group(0) (which won't contain the v)
Attachment #8775820 - Flags: review?(mh+mozilla)
Comment on attachment 8775820 [details]
Bug 1290044 - Moved NSIS configure to Python.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67876/diff/2-3/
Attachment #8775820 - Flags: review?(mh+mozilla)
Attachment #8775820 - Flags: review?(mh+mozilla)
Comment on attachment 8775820 [details]
Bug 1290044 - Moved NSIS configure to Python.

https://reviewboard.mozilla.org/r/67876/#review65518

::: moz.configure:234
(Diff revision 3)
>  set_config('MOZ_SYSTEM_HUNSPELL', system_hunspell)
>  
> +
> +@depends(target)
> +def makensis_progs(target):
> +    if target.kernel == 'WININT':

typo on WINNT.

::: moz.configure:240
(Diff revision 3)
> +        return ('makensis-3.0b3.exe', 'makensis-3.0b1.exe', 'makensis.exe', 'makensis')
> +
> +nsis = check_prog('MAKENSISU', makensis_progs)
> +
> +# Make sure the version of makensis is up to date.
> +@depends(nsis)

You'll want a @depends_if here, otherwise this will also run on non-windows platforms, and fail because nsis is None.

::: moz.configure:245
(Diff revision 3)
> +@depends(nsis)
> +@checking('for NSIS version >= 3.0b1')
> +@imports('re')
> +def valid_nsis(nsis):
> +    nsis_min_version = '3.0b1'
> +    out = check_cmd_output(nsis, '-version', onerror=lambda: die('Failed to get nsis version.'))

Not sure in my browser, but this looks like this might be over 80 characters. If it is, please line-wrap.

::: moz.configure:253
(Diff revision 3)
> +    if not m:
> +        raise FatalCheckError('Unknown version of makensis')
> +    ver = Version(m.group(0))
> +
> +    if ver < nsis_min_version:
> +        raise FatalCheckError('To build the installer you must have NSIS version %s or greater in your path'

This looks over 80-characters too.

::: moz.configure:256
(Diff revision 3)
> +
> +    if ver < nsis_min_version:
> +        raise FatalCheckError('To build the installer you must have NSIS version %s or greater in your path'
> +                              % nsis_min_version)
> +
> +    return nsis

Trying the patch, I figure this would be better if the function was called "nsis_version" and returned the version, which, in the configure output, then looks like:

"checking for NSIS version >= 3.0b1... 3.0rc1"

But then it might actually be better to remove ">= 3.0b1" from the @checking, so that it looks like

"checking for NSIS version... 3.0rc1"

If the version is not enough, it will look like

"checking for NSIS version... 2.0"
"ERROR: To build the installer you must have NSIS version 3.0b1 or greater in your path"

::: old-configure.in
(Diff revision 3)
> -if test "$OS_ARCH" = "WINNT"; then
> -    MIN_NSIS_MAJOR_VER=3
> -    MIN_NSIS_MINOR_VER=0
> -    MIN_NSIS_PRERELEASE_TYPE=b
> -    MIN_NSIS_PRERELEASE_VER=1
> -    MOZ_PATH_PROGS(MAKENSISU, $MAKENSISU makensis-3.0b3.exe makensis-3.0b1.exe makensis)

There is a AC_SUBST(MAKENSISU) further down in the file that needs to be removed, otherwise configure fails with "Cannot add 'MAKENSISU' to configuration: Key already exists".
Comment on attachment 8775820 [details]
Bug 1290044 - Moved NSIS configure to Python.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67876/diff/3-4/
Attachment #8775820 - Flags: review?(mh+mozilla)
Attachment #8775820 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8775820 [details]
Bug 1290044 - Moved NSIS configure to Python.

https://reviewboard.mozilla.org/r/67876/#review65592

::: moz.configure:258
(Diff revision 4)
> +    if ver < nsis_min_version:
> +        raise FatalCheckError('To build the installer you must have NSIS'
> +                              ' version %s or greater in your path'
> +                              % nsis_min_version)
> +
> +    return m.group(0)

Returning the Version object would be nicer and would work similarly.
Comment on attachment 8775820 [details]
Bug 1290044 - Moved NSIS configure to Python.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67876/diff/4-5/
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/7ab959c316fc
Moved NSIS configure to Python. r=glandium
https://hg.mozilla.org/mozilla-central/rev/7ab959c316fc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(In reply to Mike Hommey [:glandium] from comment #3)
> ::: build/moz.configure/windows.configure:178
[...]
> This file is only included on non-artifact builds, and I think we should
> still allow to build an installer out of an artifact build, so it would be
> better to move this block in another file. Maybe the top-level moz.configure.

Unfortunately, moving this to the top-level moz.configure means that all builds
on windows now have a NSIS build dependency -- even things like standalone
spidermonkey (which servo uses).  Before this, I believe --disable-installer would
also skip the NSIS dep, which it doesn't now.  Should I file a new bug or is there
a quick workaround?
File a new bug.
Depends on: 1311871
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.