GNOME Bugzilla – Bug 724993
gjs now depends on lcov due to --enable-coverage
Last modified: 2014-03-06 14:40:40 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?
Created attachment 270033 [details] [review] sysdeps-3.12: add lcov
Created attachment 270034 [details] [review] core-deps-3.12: gjs depends on lcov Needed for --enable-coverage
Comment on attachment 270034 [details] [review] core-deps-3.12: gjs depends on lcov I would rather have it pass --disable-coverage by default.
Jasper?
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...
Frederic, thoughts?
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?
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....
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>
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?)
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"/>.
Created attachment 270640 [details] [review] gjs: use a conditional to support coverage builds Coverage will be disabled by default.
Created attachment 270641 [details] [review] gjs: use a conditional to support coverage builds Coverage will be disabled by default.
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.
Is there a problem with depending on lcov unconditionally?
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.
FYI: to get coverage, just add the following line to your jhbuildrc: conditions.add("coverage")
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