Closed Bug 1269513 Opened 8 years ago Closed 8 years ago

Implement PKG_CHECK_MODULES in Python configure

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

MozReview Request: Bug 1269513 - Add a template to execute a depends function only when a given value is present. r=glandium
58 bytes, text/x-review-board-request
glandium
: review+
Details
MozReview Request: Bug 1269513 - Implement PKG_CHECK_MODULES equivalent in Python configure. r=glandium
58 bytes, text/x-review-board-request
glandium
: review+
Details
MozReview Request: Bug 1269513 - Add a helper for check_output in Python configure. r=glandium
58 bytes, text/x-review-board-request
glandium
: review+
Details
MozReview Request: Bug 1269513 - Make queue_debug a no-op if called from within a queue_debug block. r=glandium
58 bytes, text/x-review-board-request
glandium
: review+
Details
MozReview Request: Bug 1269513 - Move --with-system-hunspell to Python configure. r=glandium
58 bytes, text/x-review-board-request
glandium
: review+
Details 
Comment on attachment 8749478 [details]
MozReview Request: Bug 1269513 - Implement PKG_CHECK_MODULES equivalent in Python configure. r=glandium

https://reviewboard.mozilla.org/r/50989/#review48375

::: build/moz.configure/init.configure:771
(Diff revision 1)
>          if build_project != 'js':
>              return value.format(opt.option)
>  
>      add_old_configure_arg(js_option)
>  
> -
> +include('pkg.configure')

It feels to me this could go in checks.configure.

::: build/moz.configure/init.configure:772
(Diff revision 1)
>              return value.format(opt.option)
>  
>      add_old_configure_arg(js_option)
>  
> -
> +include('pkg.configure')
> +add_old_configure_assignment('PKG_CONFIG', pkg_config)

Why not put this in pkg.configure?

::: build/moz.configure/pkg.configure:11
(Diff revision 1)
> +
> +pkg_config = check_prog('PKG_CONFIG', ('pkg-config',), allow_missing=True)
> +
> +@depends_if(pkg_config)
> +@imports('subprocess')
> +def pkg_config_version(pkg_config):

@checking('for pkg-config version') ?

::: build/moz.configure/pkg.configure:12
(Diff revision 1)
> +pkg_config = check_prog('PKG_CONFIG', ('pkg-config',), allow_missing=True)
> +
> +@depends_if(pkg_config)
> +@imports('subprocess')
> +def pkg_config_version(pkg_config):
> +    return Version(subprocess.check_output([pkg_config, '--version']).rstrip())

with a try/except for CalledProcessError

::: build/moz.configure/pkg.configure:38
(Diff revision 1)
> +        min_version = '0.9.0'
> +        if pkg_config is None:
> +            die("*** The pkg-config script could not be found. Make sure it is\n"
> +                "*** in your path, or set the PKG_CONFIG environment variable\n"
> +                "*** to the full path to pkg-config.\n"
> +                "*** Or see http://www.freedesktop.org/software/pkgconfig to get pkg-config.")

That url is 404 :)

::: build/moz.configure/pkg.configure:49
(Diff revision 1)
> +    @depends_when(pkg_config, package_desc, when=condition)
> +    @imports('subprocess')
> +    def package(pkg_config, package_desc):
> +        # package_desc may start as a depends function, so we can't use
> +        # @checking here.
> +        log.info("checking for %s..." % package_desc)

Add a space after the '...' That will trigger the next log.info to be on the same line.

::: build/moz.configure/pkg.configure:50
(Diff revision 1)
> +    @imports('subprocess')
> +    def package(pkg_config, package_desc):
> +        # package_desc may start as a depends function, so we can't use
> +        # @checking here.
> +        log.info("checking for %s..." % package_desc)
> +        try:

you want a with `log.queue_debug():` before the try.

::: build/moz.configure/pkg.configure:53
(Diff revision 1)
> +        # @checking here.
> +        log.info("checking for %s..." % package_desc)
> +        try:
> +            subprocess.check_output([pkg_config, '--errors-to-stdout',
> +                                     '--print-errors', package_desc])
> +            log.info("...found")

log.info("found")

::: build/moz.configure/pkg.configure:56
(Diff revision 1)
> +            subprocess.check_output([pkg_config, '--errors-to-stdout',
> +                                     '--print-errors', package_desc])
> +            log.info("...found")
> +            return True
> +        except subprocess.CalledProcessError as e:
> +            die(e.output)

While not entirely obvious, PKG_CHECK_MODULES will not error out if you pass a 4th argument and the package description isn't fulfilled.

::: build/moz.configure/pkg.configure:62
(Diff revision 1)
> +
> +    @depends_when(pkg_config, package_desc, when=condition)
> +    @checking('%s_CFLAGS' % var, callback=lambda t: ' '.join(t))
> +    @imports('subprocess')
> +    def pkg_cflags(pkg_config, package_desc):
> +        flags = subprocess.check_output([pkg_config, '--cflags', package_desc])

try: except for CalledProcessError... I'm starting to think we need a helper around subprocess.check_output.

::: build/moz.configure/pkg.configure:69
(Diff revision 1)
> +
> +    @depends_when(pkg_config, package_desc, when=condition)
> +    @checking('%s_LIBS' % var, callback=lambda t: ' '.join(t))
> +    @imports('subprocess')
> +    def pkg_libs(pkg_config, package_desc):
> +        libs = subprocess.check_output([pkg_config, '--libs', package_desc])

Likewise

::: build/moz.configure/pkg.configure:76
(Diff revision 1)
> +        return tuple(libs.replace('-Wl,--export-dynamic', '').split())
> +
> +    set_config('%s_CFLAGS' % var, pkg_cflags)
> +    set_config('%s_LIBS' % var, pkg_libs)
> +
> +    return package

I think at some point we'll have to have a @depends that returns a namespace containing the cflags and libs and have the template return that.
Attachment #8749478 - Flags: review?(mh+mozilla)
Attachment #8749477 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8749477 [details]
MozReview Request: Bug 1269513 - Add a template to execute a depends function only when a given value is present. r=glandium

https://reviewboard.mozilla.org/r/50987/#review48379
Attachment #8749479 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8749479 [details]
MozReview Request: Bug 1269513 - Move --with-system-hunspell to Python configure.

https://reviewboard.mozilla.org/r/50991/#review48381
https://reviewboard.mozilla.org/r/50989/#review48375

> It feels to me this could go in checks.configure.

This requires updating several tests to expect PKG_CONFIG in resulting configs...

> Why not put this in pkg.configure?

Including enough to get add_old_configure in unit tests makes the unit tests messier, because we end up with things the test doesn't care about in the config.
(In reply to Chris Manchester (:chmanchester) from comment #7)
> https://reviewboard.mozilla.org/r/50989/#review48375
> 
> > It feels to me this could go in checks.configure.
> 
> This requires updating several tests to expect PKG_CONFIG in resulting
> configs...
> 
> > Why not put this in pkg.configure?
> 
> Including enough to get add_old_configure in unit tests makes the unit tests
> messier, because we end up with things the test doesn't care about in the
> config.

I guess it makes sense. Eventually, though, we should convert those tests to doing the same kind of thing as toolchain.configure tests, which allows to cherry pick what is tested.
Comment on attachment 8749477 [details]
MozReview Request: Bug 1269513 - Add a template to execute a depends function only when a given value is present. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50987/diff/1-2/
Comment on attachment 8749478 [details]
MozReview Request: Bug 1269513 - Implement PKG_CHECK_MODULES equivalent in Python configure. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50989/diff/1-2/
Comment on attachment 8749479 [details]
MozReview Request: Bug 1269513 - Move --with-system-hunspell to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50991/diff/1-2/
Comment on attachment 8751034 [details]
MozReview Request: Bug 1269513 - Add a helper for check_output in Python configure. r=glandium

https://reviewboard.mozilla.org/r/51735/#review48665

::: build/moz.configure/toolchain.configure:13
(Diff revision 1)
>  # ==============================================================
>  yasm = check_prog('YASM', ['yasm'], allow_missing=True)
>  
>  @depends_if(yasm)
>  @checking('yasm version')
>  @imports('subprocess')

You can remove the @imports.

::: build/moz.configure/util.configure:32
(Diff revision 1)
> +    try:
> +        return subprocess.check_output(args, **kwargs)
> +    except subprocess.CalledProcessError as e:
> +        if onerror:
> +            return onerror(e)
> +        die(e.output)

It would be better to stream e.output to log.debug and then call onerror/die. (see try_preprocess in toolchain.configure ; in fact, maybe just copy what that function does here and make it call this new function instead)

::: moz.configure:119
(Diff revision 1)
>  
>  @template
>  def perl_version_check(min_version):
>      @depends(perl)
>      @checking('for minimum required perl version >= %s' % min_version)
>      @imports('subprocess')

You can remove the @imports.
Attachment #8751034 - Flags: review?(mh+mozilla)
Comment on attachment 8749478 [details]
MozReview Request: Bug 1269513 - Implement PKG_CHECK_MODULES equivalent in Python configure. r=glandium

https://reviewboard.mozilla.org/r/50989/#review48667

::: build/moz.configure/pkg.configure:45
(Diff revisions 1 - 2)
>              die("*** The pkg-config script could not be found. Make sure it is\n"
>                  "*** in your path, or set the PKG_CONFIG environment variable\n"
> -                "*** to the full path to pkg-config.\n"
> +                "*** to the full path to pkg-config.")
> -                "*** Or see http://www.freedesktop.org/software/pkgconfig to get pkg-config.")
>          if version < min_version:
> -            die("*** Your version of pkg-config is too old. You need version %s or newer.\n"
> +            die("*** Your version of pkg-config is too old. You need version %s or newer." %

You can use , instead of % (printf-style)

::: build/moz.configure/pkg.configure:58
(Diff revisions 1 - 2)
> -        log.info("checking for %s..." % package_desc)
> +        log.info("checking for %s... " % package_desc)
> +        with log.queue_debug():
> -        try:
> +            try:
> -            subprocess.check_output([pkg_config, '--errors-to-stdout',
> +                subprocess.check_output([pkg_config, '--errors-to-stdout',
> -                                     '--print-errors', package_desc])
> +                                         '--print-errors', package_desc])
> -            log.info("...found")
> +                log.info("found")

"yes" would be more consistent with what we currently have.

::: build/moz.configure/pkg.configure:62
(Diff revisions 1 - 2)
> -                                     '--print-errors', package_desc])
> +                                         '--print-errors', package_desc])
> -            log.info("...found")
> +                log.info("found")
> -            return True
> +                return True
> -        except subprocess.CalledProcessError as e:
> +            except subprocess.CalledProcessError as e:
> +                if allow_missing:
> +                    return e.output

I think it would be better to do something like:
    log.info("no")
    log_writer = log.warning if allow_missing else log.error
    with LineIO(lambda l: log_writer(l)) as o:
        o.write(e.output)
    if not allow_missing:
        sys.exit(1)
    return None

As a bonus, you don't need have_package anymore.
Attachment #8749478 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8749477 [details]
MozReview Request: Bug 1269513 - Add a template to execute a depends function only when a given value is present. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50987/diff/2-3/
Attachment #8751034 - Flags: review?(mh+mozilla)
Comment on attachment 8751034 [details]
MozReview Request: Bug 1269513 - Add a helper for check_output in Python configure. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51735/diff/1-2/
Comment on attachment 8749478 [details]
MozReview Request: Bug 1269513 - Implement PKG_CHECK_MODULES equivalent in Python configure. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50989/diff/2-3/
Attachment #8749479 - Attachment is obsolete: true
Comment on attachment 8751034 [details]
MozReview Request: Bug 1269513 - Add a helper for check_output in Python configure. r=glandium

https://reviewboard.mozilla.org/r/51735/#review49349

::: build/moz.configure/util.configure:32
(Diff revisions 1 - 2)
> +@imports(_from='mozbuild.configure.util', _import='LineIO')
> +@imports(_from='mozbuild.shellutil', _import='quote')
>  def check_cmd_output(*args, **kwargs):
>      onerror = kwargs.pop('onerror', None)
> -    try:
> -        return subprocess.check_output(args, **kwargs)
> +
> +    with log.queue_debug():

Mmmmm it might be better to leave the queue_debug out of this function. Or to change queue_debug to a no-op when we're already queuing debug messages (which we are, when this is called in a @checking function). The latter might be better.

::: build/moz.configure/util.configure:50
(Diff revisions 1 - 2)
> +                log.debug('Its %s was:', desc)
> +                with LineIO(lambda l: log.debug('| %s', l)) as o:
> +                    o.write(out)
>          if onerror:
> -            return onerror(e)
> -        die(e.output)
> +            return onerror()
> +        die('Command failed.')

The backlog on error is not guaranteed to display the command executed, so it would be better to add it here too.
Attachment #8751034 - Flags: review?(mh+mozilla) → review+
Review commit: https://reviewboard.mozilla.org/r/52331/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52331/
Attachment #8749477 - Attachment description: MozReview Request: Bug 1269513 - Add a template to execute a depends function only when a given value is present. → MozReview Request: Bug 1269513 - Add a template to execute a depends function only when a given value is present. r=glandium
Attachment #8751034 - Attachment description: MozReview Request: Bug 1269513 - Add a helper for check_output in Python configure. → MozReview Request: Bug 1269513 - Add a helper for check_output in Python configure. r=glandium
Attachment #8749478 - Attachment description: MozReview Request: Bug 1269513 - Implement PKG_CHECK_MODULES equivalent in Python configure. → MozReview Request: Bug 1269513 - Implement PKG_CHECK_MODULES equivalent in Python configure. r=glandium
Attachment #8752387 - Flags: review?(mh+mozilla)
Comment on attachment 8749477 [details]
MozReview Request: Bug 1269513 - Add a template to execute a depends function only when a given value is present. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50987/diff/3-4/
Comment on attachment 8751034 [details]
MozReview Request: Bug 1269513 - Add a helper for check_output in Python configure. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51735/diff/2-3/
Comment on attachment 8749478 [details]
MozReview Request: Bug 1269513 - Implement PKG_CHECK_MODULES equivalent in Python configure. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50989/diff/3-4/
Comment on attachment 8749477 [details]
MozReview Request: Bug 1269513 - Add a template to execute a depends function only when a given value is present. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50987/diff/4-5/
Comment on attachment 8752387 [details]
MozReview Request: Bug 1269513 - Make queue_debug a no-op if called from within a queue_debug block. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52331/diff/1-2/
Comment on attachment 8751034 [details]
MozReview Request: Bug 1269513 - Add a helper for check_output in Python configure. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51735/diff/3-4/
Comment on attachment 8749478 [details]
MozReview Request: Bug 1269513 - Implement PKG_CHECK_MODULES equivalent in Python configure. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50989/diff/4-5/
Comment on attachment 8752387 [details]
MozReview Request: Bug 1269513 - Make queue_debug a no-op if called from within a queue_debug block. r=glandium

https://reviewboard.mozilla.org/r/52331/#review49904

::: python/mozbuild/mozbuild/test/configure/test_util.py:264
(Diff revision 2)
> +        handler =  ConfigureOutputHandler(out, out, maxlen=10)
> +        handler.setFormatter(logging.Formatter('%(levelname)s| %(message)s'))
> +        logger.addHandler(handler)
> +
> +        try:
> +            self.assertFalse(handler._queue_is_active)

I'm not convinced it's worth asserting implementation details. What matters here is the result.
Attachment #8752387 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8753113 [details]
MozReview Request: Bug 1269513 - Move --with-system-hunspell to Python configure. r=glandium

https://reviewboard.mozilla.org/r/53012/#review49918
Attachment #8753113 - Flags: review+
Depends on: 1279151
Depends on: 1298740
Blocks: 1307355
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: