GNOME Bugzilla – Bug 669554
Inform user if required system dependencies are missing (systemmodule)
Last modified: 2012-05-18 11:17:31 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.
(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?
Frédéric?
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).
Why not add .pc files to as much packages as possible?
(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.
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.
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.
Created attachment 211281 [details] [review] 3.x: Remove glib's cyclic <suggests/>
Created attachment 211282 [details] [review] 3.x: Remove glib's cyclic <after/>
Created attachment 211283 [details] [review] 3.x: Remove libproxy's cyclic <after/>
Created attachment 211284 [details] [review] 3.x: Remove gnome-js-common's cyclic <suggests/>
Created attachment 211285 [details] [review] 3.x: Remove nautilus's cyclic <after/>
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.
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
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.
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.
(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.
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).
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)
(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.
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.
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.
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?
(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 on attachment 211279 [details] [review] Rewrite of get_module_list Committed. http://git.gnome.org/browse/jhbuild/commit/?id=53cb43149b385e89c4058d279398a61a3331003a
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 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.
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?
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).
(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
Updated the documentation http://git.gnome.org/browse/jhbuild/commit/?id=fa895c89d73241c329e50b12b9134ae12dcadb0f Updated moduleset.dtd http://git.gnome.org/browse/jhbuild/commit/?id=84983960bc0ff17e37315ebab5d6059b69486933
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):
+ Trace 230240
self.assertRaises(UsageError, self.get_module_list, ['foo', 'bar'])
====================================================================== 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
(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