Implement PKG_CHECK_MODULES in Python configure

RESOLVED FIXED in Firefox 49

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Comment 1

2 years ago
Created 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 commit: https://reviewboard.mozilla.org/r/50987/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50987/
Attachment #8749477 - Flags: review?(mh+mozilla)
Attachment #8749478 - Flags: review?(mh+mozilla)
Attachment #8749479 - Flags: review?(mh+mozilla)
(Assignee)

Comment 2

2 years ago
Created attachment 8749478 [details]
MozReview Request: Bug 1269513 - Implement PKG_CHECK_MODULES equivalent in Python configure. r=glandium

Review commit: https://reviewboard.mozilla.org/r/50989/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50989/
(Assignee)

Comment 3

2 years ago
Created attachment 8749479 [details]
MozReview Request: Bug 1269513 - Move --with-system-hunspell to Python configure.

Review commit: https://reviewboard.mozilla.org/r/50991/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50991/
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
(Assignee)

Comment 7

2 years ago
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.
(Assignee)

Comment 9

2 years ago
Created attachment 8751034 [details]
MozReview Request: Bug 1269513 - Add a helper for check_output in Python configure. r=glandium

Review commit: https://reviewboard.mozilla.org/r/51735/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51735/
Attachment #8751034 - Flags: review?(mh+mozilla)
Attachment #8749478 - Flags: review?(mh+mozilla)
(Assignee)

Comment 10

2 years ago
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/
(Assignee)

Comment 11

2 years ago
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/
(Assignee)

Comment 12

2 years ago
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+
(Assignee)

Comment 15

2 years ago
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)
(Assignee)

Comment 16

2 years ago
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/
(Assignee)

Comment 17

2 years ago
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/
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 19

2 years ago
Created attachment 8752387 [details]
MozReview Request: Bug 1269513 - Make queue_debug a no-op if called from within a queue_debug block. r=glandium

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)
(Assignee)

Comment 20

2 years ago
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/
(Assignee)

Comment 21

2 years ago
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/
(Assignee)

Comment 22

2 years ago
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/
(Assignee)

Comment 23

2 years ago
Created attachment 8753113 [details]
MozReview Request: Bug 1269513 - Move --with-system-hunspell to Python configure. r=glandium

Review commit: https://reviewboard.mozilla.org/r/53012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53012/
(Assignee)

Comment 24

2 years ago
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/
(Assignee)

Comment 25

2 years ago
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/
(Assignee)

Comment 26

2 years ago
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/
(Assignee)

Comment 27

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1295784

Updated

2 years ago
Depends on: 1298740
Blocks: 1307355
You need to log in before you can comment on or make changes to this bug.