Closed Bug 1256568 Opened 8 years ago Closed 8 years ago

Move some MOZ_PATH_PROG to moz.configure

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

      No description provided.
Blocks: pyconfigure
Because some of the existing mozconfigs may be setting some variables,
we need to inject those that are handled by moz.configure now. It likely
doesn't matter for the variables currently in moz.configure, but it will
soon become important when more things are moved to moz.configure.

In fact, it is necessary for GENISOIMAGE and DSYMUTIL that we're going
to move in this bug, set in automation mozconfigs.

The implementation is cumbersome and quite horrible. We could do better
by changing the execution model in mozbuild.configure, which is probably
necessary for other reasons as well, but that requires more work and
testing.

Review commit: https://reviewboard.mozilla.org/r/40017/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40017/
Attachment #8730569 - Flags: review?(cmanchester)
Comment on attachment 8730569 [details]
MozReview Request: Bug 1256568 - Inject some variables set in mozconfig into moz.configure

https://reviewboard.mozilla.org/r/40017/#review36527

And here I thought the new configure wouldn't have so many hacks :P

::: build/moz.configure/init.configure:241
(Diff revision 1)
> +        )
> +    return early_options
> +
> +early_options = early_options()
> +
> +# At the moment, moz.configure doesn't have completely knowledge of all the

Nit: "complete"
Attachment #8730569 - Flags: review+
Comment on attachment 8730570 [details]
MozReview Request: Bug 1256568 - Move awk detection to moz.configure

https://reviewboard.mozilla.org/r/40019/#review36529

Cool. I bet this program checking will be significantly faster than shell on Windows! Should be pretty easy to port them all over in bulk too.
Attachment #8730570 - Flags: review+
Comment on attachment 8730571 [details]
MozReview Request: Bug 1256568 - Move perl detection to moz.configure

https://reviewboard.mozilla.org/r/40021/#review36531

::: js/src/old-configure.in
(Diff revision 1)
>  
>  MOZ_BUILD_ROOT=`pwd -W 2>/dev/null || pwd -P`
>  
>  MOZ_DEFAULT_COMPILER
>  
> -dnl Check for Perl first -- needed for win32 SDK checks

If those referenced checks are in fact still written in Perl, I hope you rewrite them to Python when you port them.

::: moz.configure:100
(Diff revision 1)
> +
> +    @depends(perl)
> +    @checking('for full perl installation')
> +    @advanced
> +    def has_full_perl_installation(perl):
> +        import subprocess

I'm not too keen on all this delayed imports for stdlib modules that will almost certainly always be loaded. But if that's the style we're using for moz.configure, so be it.

::: moz.configure:111
(Diff revision 1)
> +    def require_full_perl_installation(has_full_perl_installation):
> +        if not has_full_perl_installation:
> +            error('Cannot find Config.pm or $Config{archlib}. '
> +                  'A full perl installation is required.')
> +
> +perl_version_check('5.006')

This is so ancient. Over 15 years actually. Also 5.006 was never actually released. There were various releases of 5.005. But never a 5.006. Since we're using >= in the version check, perhaps we should bump this to the version that was actually released.
Attachment #8730571 - Flags: review+
Comment on attachment 8730572 [details]
MozReview Request: Bug 1256568 - Move doxygen, zip, unzip, xargs, rpmbuild, genisoimage and dsymutil detection to moz.configure

https://reviewboard.mozilla.org/r/40023/#review36533

::: moz.configure:118
(Diff revision 1)
> +check_prog('UNZIP', ('unzip',))
> +check_prog('ZIP', ('zip',))
> +check_prog('XARGS', ('xargs',))
> +
> +check_prog('RPMBUILD', ('rpmbuild',), allow_missing=True)
> +check_prog('GENISOIMAGE', ('genisoimage',), allow_missing=True)
> +check_prog('DSYMUTIL', ('dsymutil', 'llvm-dsymutil'), allow_missing=True)

Let's alphabetize these.
Attachment #8730572 - Flags: review+
Blocks: 1256571
(In reply to Mike Hommey [:glandium] from comment #1)
> The implementation is cumbersome and quite horrible. We could do better
> by changing the execution model in mozbuild.configure, which is probably
> necessary for other reasons as well, but that requires more work and
> testing.

Filed bug 1256571
(In reply to Gregory Szorc [:gps] from comment #7)
> This is so ancient. Over 15 years actually. Also 5.006 was never actually
> released. There were various releases of 5.005. But never a 5.006. Since
> we're using >= in the version check, perhaps we should bump this to the
> version that was actually released.

This actually means 5.6. Which is still 16 years old, but actually exists.

http://perldoc.perl.org/perlvar.html#General-Variables
  $]
    The revision, version, and subversion of the Perl interpreter, represented
    as a decimal of the form 5.XXXYYY, where XXX is the version / 1e3 and YYY is
    the subversion / 1e6. For example, Perl v5.10.1 would be "5.010001".
(In reply to Gregory Szorc [:gps] from comment #7)
> I'm not too keen on all this delayed imports for stdlib modules that will
> almost certainly always be loaded. But if that's the style we're using for
> moz.configure, so be it.

I've been thinking about replacing the @advanced decorator with a @import
decorator. Which would have the double advantage of documenting which modules
are required for the function without looking at its AST, and would solve your
concern.
Attachment #8730569 - Flags: review?(cmanchester)
Attachment #8730570 - Flags: review?(cmanchester)
Attachment #8730571 - Flags: review?(cmanchester)
Attachment #8730572 - Flags: review?(cmanchester)
(In reply to Gregory Szorc [:gps] from comment #6)
> Comment on attachment 8730570 [details]
> MozReview Request: Bug 1256568 - Move awk detection to moz.configure
> 
> https://reviewboard.mozilla.org/r/40019/#review36529
> 
> Cool. I bet this program checking will be significantly faster than shell on
> Windows! Should be pretty easy to port them all over in bulk too.

Note that I don't want to do blind porting. I already left it slip in this series that dsymutil, genisoimage and rpmbuild are platform-specific and should thus be in platform-specific sections. Will file a followup.
Depends on: 1256587
Blocks: 1256990
Depends on: 1257413
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.