Closed
Bug 1269513
Opened 8 years ago
Closed 8 years ago
Implement PKG_CHECK_MODULES in Python configure
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox49 fixed)
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: chmanchester, Assigned: chmanchester)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
The existing implementation is in https://dxr.mozilla.org/mozilla-central/source/build/autoconf/pkg.m4
Assignee | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50989/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50989/
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50991/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50991/
Comment 4•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8749477 -
Flags: review?(mh+mozilla) → review+
Comment 5•8 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 https://reviewboard.mozilla.org/r/50987/#review48379
Updated•8 years ago
|
Attachment #8749479 -
Flags: review?(mh+mozilla) → review+
Comment 6•8 years ago
|
||
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•8 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.
Comment 8•8 years ago
|
||
(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•8 years ago
|
||
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•8 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•8 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•8 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 13•8 years ago
|
||
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 14•8 years ago
|
||
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•8 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•8 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•8 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•8 years ago
|
Attachment #8749479 -
Attachment is obsolete: true
Comment 18•8 years ago
|
||
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•8 years ago
|
||
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•8 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•8 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•8 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•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53012/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53012/
Assignee | ||
Comment 24•8 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•8 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•8 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•8 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 28•8 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 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 29•8 years ago
|
||
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+
Comment 30•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/59d454cec87f https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa9b5719a80 https://hg.mozilla.org/integration/mozilla-inbound/rev/810bc87c256d https://hg.mozilla.org/integration/mozilla-inbound/rev/b465c1ff97c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/54f2a1fe535f
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/59d454cec87f https://hg.mozilla.org/mozilla-central/rev/7aa9b5719a80 https://hg.mozilla.org/mozilla-central/rev/810bc87c256d https://hg.mozilla.org/mozilla-central/rev/b465c1ff97c4 https://hg.mozilla.org/mozilla-central/rev/54f2a1fe535f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•