After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 724993 - gjs now depends on lcov due to --enable-coverage
gjs now depends on lcov due to --enable-coverage
Status: RESOLVED FIXED
Product: jhbuild
Classification: Infrastructure
Component: module sets
unspecified
Other All
: Normal normal
: ---
Assigned To: Jhbuild maintainers
Jhbuild QA
Depends on:
Blocks:
 
 
Reported: 2014-02-23 04:41 UTC by Michael Catanzaro
Modified: 2014-03-06 14:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
sysdeps-3.12: add lcov (916 bytes, patch)
2014-02-23 04:41 UTC, Michael Catanzaro
committed Details | Review
core-deps-3.12: gjs depends on lcov (834 bytes, patch)
2014-02-23 04:42 UTC, Michael Catanzaro
reviewed Details | Review
gjs: use a conditional to support coverage builds (1.70 KB, patch)
2014-03-01 16:39 UTC, Michael Catanzaro
none Details | Review
gjs: use a conditional to support coverage builds (1.22 KB, patch)
2014-03-01 19:00 UTC, Michael Catanzaro
none Details | Review
gjs: use a conditional to support coverage builds (1.22 KB, patch)
2014-03-01 19:01 UTC, Michael Catanzaro
committed Details | Review

Description Michael Catanzaro 2014-02-23 04:41:36 UTC
checking for lcov... no
checking for genhtml... no
configure: error: lcov and genhtml are required for --enable-coverage

genhtml is part of the lcov package. I don't think it needs to be added separately? And it's not autotools, so does that mean it goes in sysdeps rather than core-deps?
Comment 1 Michael Catanzaro 2014-02-23 04:41:39 UTC
Created attachment 270033 [details] [review]
sysdeps-3.12: add lcov
Comment 2 Michael Catanzaro 2014-02-23 04:42:13 UTC
Created attachment 270034 [details] [review]
core-deps-3.12: gjs depends on lcov

Needed for --enable-coverage
Comment 3 Frederic Peters 2014-02-23 07:42:43 UTC
Comment on attachment 270034 [details] [review]
core-deps-3.12: gjs depends on lcov

I would rather have it pass --disable-coverage by default.
Comment 4 Michael Catanzaro 2014-02-23 14:40:00 UTC
Jasper?
Comment 5 Jasper St. Pierre (not reading bugmail) 2014-02-23 14:46:51 UTC
I explicitly added --enable-coverage to test it on my system, and then pushed it thinking it wouldn't have many difficulties. I don't particular care what we end up doing, but I know that other apps like gnome-weather want to use the coverage system as well, so...
Comment 6 Michael Catanzaro 2014-02-26 23:39:25 UTC
Frederic, thoughts?
Comment 7 Frederic Peters 2014-02-27 08:10:26 UTC
I stand by my comment, in its default behaviour I'd rather not have that dependency in jhbuild. Also, it would probably a case where the conditions support would be appropriate (cf bug 720839), so people that wants would pass --conditions=+coverage to jhbuild and get coverage support in all modules.

Michael, I'll let the decision to you, would you create the minimal patch adding --disable-coverage to gjs? or do you want do go a step further and try to add the condition stuff?
Comment 8 Michael Catanzaro 2014-03-01 16:39:20 UTC
Created attachment 270629 [details] [review]
gjs: use a conditional to support coverage builds

Here's what I came up with, but I hope there's a better way to do this....
Comment 9 Frederic Peters 2014-03-01 17:48:54 UTC
I didn't try, but something like this should be possible:

    <autotools id="gjs" autogenargs="--enable-installed-tests --disable-coverage">
      <if condition-set="coverage">
          <autogenargs>--enable-coverage</autogenargs>
      </if>
      <branch/>
      <dependencies>
        <dep package="dbus-glib"/>
        <dep package="gobject-introspection"/>
        <dep package="js24"/>
        <if condition-set="coverage">
          <dep package="lcov"/>
        </if>
      </dependencies>
    </autotools>
Comment 10 Michael Catanzaro 2014-03-01 18:10:40 UTC
I don't think autogenargs is a valid tag:

[mcatanzaro@victory-road ~]$ jhbuild buildone -naf gjs
jhbuild buildone: <%s/> tag must contain value=''

(If that did work, then I think we'd have to specify --enabled-installed-tests twice?)
Comment 11 Frederic Peters 2014-03-01 18:36:29 UTC
Well, I said I didn't try it.  Going back to bug 720839, you see that the parameter should be specified as the value attribute, not the note content, e.g. <autogenargs value="--enable-coverage"/>.
Comment 12 Michael Catanzaro 2014-03-01 19:00:32 UTC
Created attachment 270640 [details] [review]
gjs: use a conditional to support coverage builds

Coverage will be disabled by default.
Comment 13 Michael Catanzaro 2014-03-01 19:01:54 UTC
Created attachment 270641 [details] [review]
gjs: use a conditional to support coverage builds

Coverage will be disabled by default.
Comment 14 Michael Catanzaro 2014-03-06 01:12:35 UTC
Frederic, are these OK to push?  They work fine for me when testing with local modulesets enabled (both with and without the "coverage" condition).

One possible future downside is if the user wants to build some apps with coverage and some without, then it might be better to use a more specific condition name. On the other hand, that would make it harder to turn on coverage for all apps that might support it conditionally.
Comment 15 Jasper St. Pierre (not reading bugmail) 2014-03-06 01:35:03 UTC
Is there a problem with depending on lcov unconditionally?
Comment 16 Frederic Peters 2014-03-06 09:45:13 UTC
Michael, fine as is, I thought --enable-coverage was the default but it is not.

Jasper, my idea is to limit the number of dependencies by default, to reduce the chances of failure for new contributors.
Comment 17 Michael Catanzaro 2014-03-06 14:39:20 UTC
FYI: to get coverage, just add the following line to your jhbuildrc:

conditions.add("coverage")
Comment 18 Michael Catanzaro 2014-03-06 14:40:33 UTC
Attachment 270033 [details] pushed as 55a82b1 - sysdeps-3.12: add lcov
Attachment 270641 [details] pushed as 341cea1 - gjs: use a conditional to support coverage builds