Bug 603514 (libmodman)
Summary: | Review Request: libmodman - A simple library for managing C++ modules (plug-ins) | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Nathaniel McCallum <nathaniel> | ||||
Component: | Package Review | Assignee: | Ian Weller <ian> | ||||
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | low | ||||||
Version: | rawhide | CC: | fedora-package-review, ian, kwizart, nathaniel, notting, supercyper1 | ||||
Target Milestone: | --- | Flags: | ian:
fedora-review+
kevin: fedora-cvs+ |
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2010-07-03 16:00:57 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 603517 | ||||||
Attachments: |
|
Description
Nathaniel McCallum
2010-06-13 16:52:25 UTC
[ OK ] specfiles match: 950b547a9132a37e853b8db64cae1d39 libmodman.spec 950b547a9132a37e853b8db64cae1d39 libmodman.spec.1 [ OK ] source files match upstream: 05213b381858c9c0fdf71309ada81ea3 libmodman-1.0.1.tar.gz 05213b381858c9c0fdf71309ada81ea3 libmodman-1.0.1.tar.gz.1 [ OK ] package meets naming and versioning guidelines. [ OK ] spec is properly named, cleanly written, and uses macros consistently. [ OK ] dist tag is present. [ OK ] build root is correct. [ OK ] license field matches the actual license. [ OK ] license is open source-compatible. [ OK ] license text included in package. [ OK ] latest version is being packaged. [FAILED] BuildRequires are proper. build.log attached [ OK ] compiler flags are appropriate. [ OK ] %clean is present. [FAILED] package builds in mock. build.log attached [ OK ] no duplicates in %files. [ OK ] file permissions are appropriate. [ OK ] scriptlets match those on ScriptletSnippets page. [ OK ] code, not content. [ OK ] documentation is small, so no -docs subpackage is necessary. [ OK ] %docs are not necessary for the proper functioning of the package. [ N/A ] desktop files valid and installed properly. Items that will be checked after package builds in mock: [ ] package installs properly. [ ] debuginfo package looks complete. [ ] rpmlint is silent. [ ] final provides and requires are sane [ ] %check is present and all tests pass [ ] no shared libraries are added to the regular linker search paths. [ ] owns the directories it creates. [ ] doesn't own any directories it shouldn't. [ ] no headers. [ ] no pkgconfig files. [ ] no libtool .la droppings. build.log is not attached. Created attachment 425156 [details]
build.log of failed build, F12 x86_64
well, that's what I get for being distracted (:
Ok its fixed. The error was that we link against libz during one of the tests to see if our symbol resolution code works. libz-devel is now in buildrequires, but is not needed for the final binary. new URLs? http://nathaniel.themccallums.org/rpms/libmodman.spec http://nathaniel.themccallums.org/rpms/libmodman-1.0.1-2.fc13.src.rpm [FAILED] license text included in package. I didn't catch this in my earlier iteration, sorry. -devel needs the %docs. [ OK ] BuildRequires are proper. [ OK ] package builds in mock. [ OK ] package installs properly. [ OK ] debuginfo package looks complete. [FAILED] rpmlint is silent. libmodman-devel.x86_64: W: no-documentation [ OK ] final provides and requires are sane [ OK ] %check is present and all tests pass [ OK ] no shared libraries are added to the linker search paths w/o ldconfig [ OK ] owns the directories it creates. [FAILED] doesn't own any directories it shouldn't. -devel needs to own %{_includedir}/%{name} just list the directory (without %dir) and it'll pull everything under it in [ OK ] no headers outside -devel [ OK ] no pkgconfig files outside -devel [ OK ] no libtool .la droppings. Fixed. http://nathaniel.themccallums.org/rpms/libmodman.spec http://nathaniel.themccallums.org/rpms/libmodman-1.0.1-3.fc13.src.rpm Outstanding issues fixed. -------------------------------------- This package (libmodman) is APPROVED -------------------------------------- Remember the caveat mentioned on devel.org with regards to compatibility with libproxy. Also, you're sponsored now! :) Some suggestions: %{_datadir}/cmake/Modules is not owned by this package, you may need to remove Findlibmodman.cmake if compiling libproxy 0.4.4 don't need this files or requires cmake. Requires: pkgconfig is also not needed now, rpmbuild will take it automatically. (In reply to comment #7) > [FAILED] license text included in package. > I didn't catch this in my earlier iteration, sorry. -devel needs the %docs. This is probably not needed, %file don't need to not include the same files twice. Most libraries(glibc zlib qt etc.) in fedora don't include the same doc with -devel subpackage, rpmlint warnings can be ignored. New Package CVS Request ======================= Package Name: libmodman Short Description: A simple library for managing C++ modules (plug-ins) Owners: npmccallum Branches: F-13 InitialCC: (You can't branch this for F-13, since libproxy is critpath. http://lists.fedoraproject.org/pipermail/devel/2010-June/137665.html) It is libproxy >=0.4.0 that cannot go into F-13. libmodman should be able to go in just fine. Additionally, since libproxy won't be upgraded for F-13, there is no conflict between libmodman and libproxy in F-13. The conflict is temporary and in rawhide/F-14 only. In short, libmodman can go into F-13 just fine. The only challenge is F-14/rawhide. but don't the files between the two packages conflict? They only conflict in the libproxy 0.4.x series, which will never be in F-13. (In reply to comment #14) > but don't the files between the two packages conflict? Agree, the -devel subpackage of libmodman is conflict with libproxy 0.3.x. Hi Noathaiel, You still need to address the issue I mentioned, especially for the unowned directory %{_datadir}/cmake/Modules. Chen Lei, I can just make -devel Require cmake, right? That should resolve the directory ownership... However, I cannot see any way in which libmodman-devel is in conflict with libproxy 0.3.x. If you believe it is, please demonstrate how. I'm sorry, libproxy 0.3.x itself is not conflict with libmodman, but since you add Conflicts field to libmodman, those two packages will be conflicts and can't coexist on F13. Also, conflicts is also needed for libmodman-devel, it conflicts with libproxy-devel 0.4.0. However, I'd rather suggest you to remove all conflicts field in spec, thus libmodman and libproxy can coexist in F13. Yum works fine without those conflicts field, libproxy 0.4.0 will upgrade to libmodman and libproxy 0.4.4 sanely. My goal with libmodman and F-13 is to get libmodman working in F-14 with libproxy >= 0.4.4, then I will backport to F-13 and remove the conflicts. That is why I applied for a libmodman F-13 branch. So the only outstanding issue I can see is the libmodman-devel directory ownership issue. I believe this can be resolved by having libmodman-devel require cmake (which owns the directory). If this is not appropriate, please suggest an alternative option. (In reply to comment #19) > My goal with libmodman and F-13 is to get libmodman working in F-14 with > libproxy >= 0.4.4, then I will backport to F-13 and remove the conflicts. That > is why I applied for a libmodman F-13 branch. > It make sense, I can't find any better solution right now :) > So the only outstanding issue I can see is the libmodman-devel directory > ownership issue. I believe this can be resolved by having libmodman-devel > require cmake (which owns the directory). If this is not appropriate, please > suggest an alternative option. Requires cmake if building libproxy 0.4.4 need this file or simply remove this file after make install. For pkgconfig, see http://fedoraproject.org/wiki/PackagingGuidelines#Pkgconfig_Files, but requires pkgconfig is also Okay, it's the same. Incluing documentions in -devel is not mandatory, some rpmlint warnings can be ignored, e.g. W: spelling-error, W: no-documentation. You don't need to add LICENSE file to all subpackages, -devel subpackage should only contain docs which are related to development e.g. apidocs. Ok, this should be fixed. If it is approved, do I need to reapply for the new package cvs request? http://nathaniel.themccallums.org/rpms/libmodman.spec http://nathaniel.themccallums.org/rpms/libmodman-1.0.1-4.fc13.src.rpm (In reply to comment #21) > Ok, this should be fixed. If it is approved, do I need to reapply for the new > package cvs request? > > http://nathaniel.themccallums.org/rpms/libmodman.spec > http://nathaniel.themccallums.org/rpms/libmodman-1.0.1-4.fc13.src.rpm Not needed, this package is already approved by Ian, his formal review is quite well :), all of my comments are just suggestions. Unblocking FE-NEEDSPONSOR for all of your packages. CVS done (by process-cvs-requests.py). libmodman need to be imported in devel in order to have libproxy updated. (and rebuilt for the webkitgtk ABI bump) BTW, I see the conflicts with libproxy < 0.4.4 as totally pointless, since we don't provides multiple version of the same package in rawhide. |