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 ReviewAssignee: Ian Weller <ian>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: 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 Flags
build.log of failed build, F12 x86_64 none

Description Nathaniel McCallum 2010-06-13 16:52:25 UTC
Spec URL: http://nathaniel.themccallums.org/rpms/libmodman.spec
SRPM URL: http://nathaniel.themccallums.org/rpms/libmodman-1.0.1-1.fc13.src.rpm

Ian Weller will review.

Comment 1 Ian Weller 2010-06-18 14:29:06 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.

Comment 2 Nathaniel McCallum 2010-06-18 15:15:47 UTC
build.log is not attached.

Comment 3 Ian Weller 2010-06-18 15:27:37 UTC
Created attachment 425156 [details]
build.log of failed build, F12 x86_64

well, that's what I get for being distracted (:

Comment 4 Nathaniel McCallum 2010-06-18 15:37:49 UTC
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.

Comment 5 Ian Weller 2010-06-18 15:54:57 UTC
new URLs?

Comment 7 Ian Weller 2010-06-19 00:53:49 UTC
[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.

Comment 9 Ian Weller 2010-06-19 01:04:39 UTC
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! :)

Comment 10 Chen Lei 2010-06-19 19:15:32 UTC
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.

Comment 11 Nathaniel McCallum 2010-06-19 20:36:00 UTC
New Package CVS Request
=======================
Package Name: libmodman
Short Description: A simple library for managing C++ modules (plug-ins)
Owners: npmccallum
Branches: F-13
InitialCC:

Comment 12 Ian Weller 2010-06-19 21:40:31 UTC
(You can't branch this for F-13, since libproxy is critpath. http://lists.fedoraproject.org/pipermail/devel/2010-June/137665.html)

Comment 13 Nathaniel McCallum 2010-06-19 22:08:59 UTC
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.

Comment 14 Ian Weller 2010-06-19 22:18:45 UTC
but don't the files between the two packages conflict?

Comment 15 Nathaniel McCallum 2010-06-19 23:18:06 UTC
They only conflict in the libproxy 0.4.x series, which will never be in F-13.

Comment 16 Chen Lei 2010-06-20 01:39:35 UTC
(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.

Comment 17 Nathaniel McCallum 2010-06-20 01:44:43 UTC
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.

Comment 18 Chen Lei 2010-06-20 02:24:15 UTC
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.

Comment 19 Nathaniel McCallum 2010-06-20 02:30:47 UTC
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.

Comment 20 Chen Lei 2010-06-20 02:53:56 UTC
(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.

Comment 21 Nathaniel McCallum 2010-06-20 04:22:51 UTC
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

Comment 22 Chen Lei 2010-06-20 04:34:49 UTC
(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.

Comment 23 Kevin Fenzi 2010-06-21 02:23:24 UTC
CVS done (by process-cvs-requests.py).

Comment 24 Nicolas Chauvet (kwizart) 2010-07-02 15:22:04 UTC
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.