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 669554 - Inform user if required system dependencies are missing (systemmodule)
Inform user if required system dependencies are missing (systemmodule)
Status: RESOLVED FIXED
Product: jhbuild
Classification: Infrastructure
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: Jhbuild maintainers
Jhbuild QA
Depends on:
Blocks:
 
 
Reported: 2012-02-07 10:40 UTC by Craig Keogh
Modified: 2012-05-18 11:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Rewrite of get_module_list (19.58 KB, patch)
2012-04-04 12:20 UTC, Craig Keogh
committed Details | Review
tests: Adapt to get_module_list, add sys deps test (7.44 KB, patch)
2012-04-04 12:20 UTC, Craig Keogh
committed Details | Review
3.x: Remove glib's cyclic <suggests/> (2.77 KB, patch)
2012-04-04 12:21 UTC, Craig Keogh
none Details | Review
3.x: Remove glib's cyclic <after/> (807 bytes, patch)
2012-04-04 12:21 UTC, Craig Keogh
none Details | Review
3.x: Remove libproxy's cyclic <after/> (3.31 KB, patch)
2012-04-04 12:22 UTC, Craig Keogh
none Details | Review
3.x: Remove gnome-js-common's cyclic <suggests/> (2.37 KB, patch)
2012-04-04 12:23 UTC, Craig Keogh
none Details | Review
3.x: Remove nautilus's cyclic <after/> (2.14 KB, patch)
2012-04-04 12:23 UTC, Craig Keogh
none Details | Review
Add systemmodule tag & check deps on build (22.60 KB, patch)
2012-04-05 12:31 UTC, Craig Keogh
committed Details | Review
Incorporating Frederic Peters' comments (8.41 KB, patch)
2012-05-16 11:15 UTC, Craig Keogh
committed Details | Review
Handle cyclic dependencies as per Frederic Peters' comment (2.79 KB, patch)
2012-05-16 11:19 UTC, Craig Keogh
committed Details | Review

Description Craig Keogh 2012-02-07 10:40:49 UTC
I want the often out-of-date http://live.gnome.org/JhbuildDependencies to go away. JHBuild should inform the user if they are missing a system dependency. A system dependency is a module that should never be built by JHBuild (for various reasons, for example module needs to be installed system wide, like udisks).

Frederic Peters' idea from bug 656818 comment 4.
Comment 1 Craig Keogh 2012-02-07 10:51:25 UTC
(In reply to bug 656818 comment #12)
> Craig, could you create another enhancement request for systemmodule (but I
> don't think it should be done that way, as relying on pkg-config won't work for
> the modules that are currently listed in bootstrap)

I want to work in smaller pieces. I want to get pkg-config in first. This will greatly improve the user's experience. Then further enhancements can be made work for bootstrap modules.

Why don't you think it should be done that way?
Comment 2 Craig Keogh 2012-02-17 22:15:36 UTC
Frédéric?
Comment 3 Frederic Peters 2012-02-18 11:20:56 UTC
We already have support for installing system packages meeting pkg-config requirements, and got a new class of problems (ex: libxslt-dev will be installed, but the actual requirement is on xsltproc, packaged seperately, and then you get in a serie of "what did jhbuild skip erroneously").

Adding plain <systemmodule> or <module> (like noted here, or in my comment in bug 656818) won't bring much by itself, it really needs (in my opinion) an additional way of specifying actual package dependencies. (ex: looking at the bootstrap modules mentioned in the other bug report: gettext, m4, autoconf, libtool, automake*, pkg-config at least do not have .pc files).
Comment 4 Olav Vitters 2012-02-18 11:24:50 UTC
Why not add .pc files to as much packages as possible?
Comment 5 Craig Keogh 2012-02-18 11:53:41 UTC
(In reply to comment #3)
> Adding plain <systemmodule> or <module> (like noted here, or in my comment in
> bug 656818) won't bring much by itself, it really needs (in my opinion) an
> additional way of specifying actual package dependencies. (ex: looking at the
> bootstrap modules mentioned in the other bug report: gettext, m4, autoconf,
> libtool, automake*, pkg-config at least do not have .pc files).

I plan to add that. I haven't nutted out the detail yet. Rather than <pkg-config/>, something like <provides-executable>automake-1.11</provides-executable>. I wanted to test the waters first with systemmodule before performing further coding that could get rejected.

Sure, sometimes "what did jhbuild skip erroneously" will happen. But that's better than providing no help at all, as currently implemented.
Comment 6 Craig Keogh 2012-04-04 12:20:21 UTC
Created attachment 211279 [details] [review]
Rewrite of get_module_list


Solving this bug is difficult because get_module_list is a difficult to understand function. I've rewritten it, it discovers the module order using
depth first tree. The function would be very short, but implementing <after>
adds to the functions length.

The rewrite displays warnings about cyclic dependencies. The function used to
raise an exception for hard cyclic dependencies and ignore <suggests> and
<after> cyclic dependencies.

The rewrite displays a warning about missing dependencies. The function used to
ignore them.
Comment 7 Craig Keogh 2012-04-04 12:20:59 UTC
Created attachment 211280 [details] [review]
tests: Adapt to get_module_list, add sys deps test


The original tests pass, with a few tweaks to handle the changed cyclic
detection behaviour.
Comment 8 Craig Keogh 2012-04-04 12:21:27 UTC
Created attachment 211281 [details] [review]
3.x: Remove glib's cyclic <suggests/>
Comment 9 Craig Keogh 2012-04-04 12:21:47 UTC
Created attachment 211282 [details] [review]
3.x: Remove glib's cyclic <after/>
Comment 10 Craig Keogh 2012-04-04 12:22:48 UTC
Created attachment 211283 [details] [review]
3.x: Remove libproxy's cyclic <after/>
Comment 11 Craig Keogh 2012-04-04 12:23:15 UTC
Created attachment 211284 [details] [review]
3.x: Remove gnome-js-common's cyclic <suggests/>
Comment 12 Craig Keogh 2012-04-04 12:23:47 UTC
Created attachment 211285 [details] [review]
3.x: Remove nautilus's cyclic <after/>
Comment 13 Craig Keogh 2012-04-05 12:31:07 UTC
Created attachment 211372 [details] [review]
Add systemmodule tag & check deps on build

Add a systemmodule modtype. systemmodules are modules that must be
provided by your system. 'jhbuild build' will raise an error if the
systemmodule is not provided. systemmodules aim to remove the need for
'sudo yum install libcurl-devel perl-XML-Simple ...' pre-setup.

Use the 'jhbuild sysdeps' command to learn how your system meets the
required dependencies.

To turn off systemmodules, use --nodeps or set in ~/.jhbuildrc:
check_sysdeps = False

I want the often out-of-date http://live.gnome.org/JhbuildDependencies to go away.

Here is an example on how to use. In a .modules file:

<repository type="tarball" name="system" href="None"/>

<systemmodule id="zlib" >
  <pkg-config>zlib.pc</pkg-config>
  <branch repo="system" version="1.2"/>
</systemmodule>

Then to libxml2 package, add:

<dep package="zlib"/>

Documentation needs updating. Documentation updates not included in patch.
Comment 14 Craig Keogh 2012-04-22 23:54:13 UTC
Note, this bug will solve Federico Mena Quintero's point 3 [1]: "Packages fail to build due to missing external dependencies, but you don't get notifid until the package fails to build."

Also see Federico's BuildMeHarder [2].

Sadly, ppp-devel (a dep of Network Manager) doesn't provide a .pc, so this bug won't solve Federico cherry-picked example, but it will solve many others. ppp-devel can be fixed after this bug.

[1] http://mail.gnome.org/archives/desktop-devel-list/2012-April/msg00040.html
[2] https://live.gnome.org/BuildMeHarder
Comment 15 Craig Keogh 2012-05-07 01:27:44 UTC
I'll commit these patches in a few weeks, if no objection. There is a chance the patches may break somethings, but I hope not. I have tested the patches.
Comment 16 Frederic Peters 2012-05-09 14:31:24 UTC
Sorry I didn't respond earlier, could you not divert the tarball repository type (href=None is really ugly) and create a new "system" repository? (that doesn't even have to be declared in the moduleset XML).

I had a quick look at the patch:

+        if config.check_sysdeps and \
+        not self.required_system_dependencies_installed(module_state):

This is not indented properly (this also happens in other places).

+class SystemModule(Package):
+    pass
+
+#    @classmethod
+#    def parse_from_xml(cls, node):#, config, uri, repositories, default_repo):
+#        """Create a new Package instance from a DOM XML node."""
+#        name = node.getAttribute('id')
...

Why those commented lines? (if the code is not useful, it should simply be removed)


+            if not isinstance(module.branch, TarballBranch) and \
+            not isinstance(module, SystemModule):

This check is used in different places, could it become a method of the module object?

What's the purpose of the removal of cyclic dependencies in suggests/after? It looks like we'll lose information here.
Comment 17 Craig Keogh 2012-05-10 01:27:37 UTC
(In reply to comment #16)

Thank you for the review.

> What's the purpose of the removal of cyclic dependencies in suggests/after? It
> looks like we'll lose information here.

I removed the cyclic dependencies because they don't affect JHBuild in anyway. They are effectively 'for your information comments'.

I see 3 options:
1. don't remove cyclic dependencies. JHBuild will warn about them.
2. don't remove cyclic dependencies. Don't warn about them - how JHBuild works currently. The cyclic dependencies should become xml comments <!-- --> to avoid the illusion they do something.
3. don't remove cyclic dependencies. Change JHBuild to build the modules twice. I rasied bug 675787 for this.
Comment 18 Frederic Peters 2012-05-11 06:43:18 UTC
For the time being, could we go with 1) or 2), or an alternate "don't remove cyclic dependencies; don't warn about them because they are not hard deps" (which is the current situation).
Comment 19 Ray Strode [halfline] 2012-05-11 14:01:36 UTC
Hey,
> I want the often out-of-date http://live.gnome.org/JhbuildDependencies to go
> away.
Sounds great.

> Here is an example on how to use. In a .modules file:
...
> <systemmodule id="zlib" >
>   <pkg-config>zlib.pc</pkg-config>
>   <branch repo="system" version="1.2"/>
> </systemmodule>
So this suffers from the same problem as how sysdeps works now. Namely, necessary packages that don't (and potentially shouldn't) have .pc files won't get pulled in.

Examples: libxml2-python gperf libtasn1-tools

Also, there's things like libjpeg-turbo that should probably have .pc file but don't. Arguably, (as mentioned in comment 4) those cases should get fixed though.

It feels like we need more than just <pkg-config/> but maybe also something for libraries, programs and python files as well. (something akin to AC_CHECK_LIB, AC_PATH_PROG and AC_PYTHON_MODULE)
Comment 20 Craig Keogh 2012-05-12 12:05:28 UTC
(In reply to comment #19)
> It feels like we need more than just <pkg-config/> but maybe also something for
> libraries, programs and python files as well. (something akin to AC_CHECK_LIB,
> AC_PATH_PROG and AC_PYTHON_MODULE)

I agree. This is bug 671042. The bug title does say 'executable files' but more than just executable files is needed. I haven't worked out all the detail yet - your input has helped towards the design. Thank you.
Comment 21 Craig Keogh 2012-05-16 11:15:29 UTC
Created attachment 214175 [details] [review]
Incorporating Frederic Peters' comments


Frederic, I've incorporated your comments. This is patch on top of attachment 211372 [details] [review] to make re-review easier. I can squash patches if you like.

(In reply to comment #16)
> +            if not isinstance(module.branch, TarballBranch) and \
> +            not isinstance(module, SystemModule):
> 
> This check is used in different places, could it become a method of the module
> object?

Do you suggest something like:
module.is_system_module() that returns True or False?
So in the base class (Package) is would be something like:
  def is_system_module(self):
        return False
And in the SystemModule class, overload this method with:
  def is_system_module(self):
        return True

I don't think that makes the code easier to read.
Comment 22 Craig Keogh 2012-05-16 11:19:59 UTC
Created attachment 214176 [details] [review]
Handle cyclic dependencies as per Frederic Peters'  comment

(In reply to comment #18)
> For the time being, could we go with 1) or 2), or an alternate "don't remove
> cyclic dependencies; don't warn about them because they are not hard deps"
> (which is the current situation).

How about we go with 1. The attached patch ensures the cyclic dependencies are handled correctly - the rewrite produces the same order list as JHBuild currently.

This is patch on top of attachment 211279 [details] [review] to make re-review easier. I can squash patches if you like.
Comment 23 Frederic Peters 2012-05-16 12:19:06 UTC
Let's get them in as they are, we'll build on that.

Btw what's your next plan? adding support for package names, to not rely on pkg-config files?
Comment 24 Craig Keogh 2012-05-16 12:40:20 UTC
(In reply to comment #23)
> Btw what's your next plan? adding support for package names, to not rely on
> pkg-config files?

Yes. I haven't come up with a design yet. Please jump over to bug 671042 and suggest how to detect packages.

If there are any major problems with the patch, I'll revert. Any added <systemmodules> will get silently get ignored by JHBuild.
Comment 25 Craig Keogh 2012-05-16 12:48:26 UTC
Comment on attachment 211279 [details] [review]
Rewrite of get_module_list

Committed.
http://git.gnome.org/browse/jhbuild/commit/?id=53cb43149b385e89c4058d279398a61a3331003a
Comment 26 Craig Keogh 2012-05-16 12:48:45 UTC
Comment on attachment 211280 [details] [review]
tests: Adapt to get_module_list, add sys deps test

Committed.
http://git.gnome.org/browse/jhbuild/commit/?id=453b2f18daf751fae60c431e847b874563065f23
Comment 27 Craig Keogh 2012-05-16 12:49:52 UTC
Comment on attachment 211372 [details] [review]
Add systemmodule tag & check deps on build

Committed.
http://git.gnome.org/browse/jhbuild/commit/?id=2bed7087604b89dfbf1b30ffce10ec54d05d1428

Leaving this bug open - documentation to update.
Comment 28 Frederic Peters 2012-05-17 10:09:54 UTC
Using this for a while I wonder if we could change the circular dependencies warnings to be debug messages, not to frighten the user by default; what do you think of this?
Comment 29 Frederic Peters 2012-05-17 11:20:02 UTC
Or the messages could be removed, and displayed only by a new "moduleset check" command? (this would also apply to the "X has a dependency on unknown Y module." messages).
Comment 30 Craig Keogh 2012-05-17 11:50:38 UTC
(In reply to comment #29)
> Or the messages could be removed, and displayed only by a new "moduleset check"
> command? (this would also apply to the "X has a dependency on unknown Y
> module." messages).

I don't like the message either (my original intention was to remove circular dependencies hence no message). I've removed the message [1]. I've raised bug 676224 to handle the 'jhbuild checkmodulesets' part.

[1] http://git.gnome.org/browse/jhbuild/commit/?id=c7ac6186c454d9bea705d5566f546f0b602f05bf
Comment 32 Frederic Peters 2012-05-18 08:35:41 UTC
Thanks Craig.

Sorry I checked that only now, but I get those test suite failures:

======================================================================
FAIL: test_dependency_chain_recursive_after_dependencies (__main__.ModuleOrderingTestCase)
A chain dependency with an <after> module depending on an inversed relation
----------------------------------------------------------------------
Traceback (most recent call last):
  • File "tests.py", line 268 in test_dependency_chain_recursive_after_dependencies
    self.assertRaises(UsageError, self.get_module_list, ['foo', 'bar'])
AssertionError: UsageError not raised

======================================================================
FAIL: test_dependency_cycle (__main__.ModuleOrderingTestCase)
A chain of dependencies with a cycle
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests.py", line 190, in test_dependency_cycle
    self.assertRaises(UsageError, self.get_module_list, ['foo'])
AssertionError: UsageError not raised
Comment 33 Craig Keogh 2012-05-18 11:17:31 UTC
(In reply to comment #32)
> Thanks Craig.
> 
> Sorry I checked that only now, but I get those test suite failures:

Good pick up. I broke that when removing the warning. Fixed now.
http://git.gnome.org/browse/jhbuild/commit/?id=2b2ebe488ea5c0ccb9d9dfc9059a35e00eef0a06