Bug 509445 - Review Request: sblim-cmpi-rpm - CIM access to rpm and other information about installed packages
Review Request: sblim-cmpi-rpm - CIM access to rpm and other information abou...
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Martin Gieseking
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 468400
  Show dependency treegraph
 
Reported: 2009-07-02 15:56 EDT by Praveen K Paladugu
Modified: 2010-09-17 12:18 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-09-17 12:18:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
martin.gieseking: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Praveen K Paladugu 2009-07-02 15:56:54 EDT
Spec URL: http://linux.dell.com/files/fedora/sblim-cmpi-rpm/sblim-cmpi-rpm.spec
SRPM URL: http://linux.dell.com/files/fedora/sblim-cmpi-rpm-1.0.1-1.fc10.src.rpm
Description: <These CIM providers list the software packages installed on GNU/Linux system and provide some detail about them>
Comment 1 Praveen K Paladugu 2009-07-02 16:01:05 EDT
The Link for the source rpm is: 
http://linux.dell.com/files/fedora/sblim-cmpi-rpm/sblim-cmpi-rpm-1.0.1-1.fc10.src.rpm

It was a typo....
Comment 2 Jason Tibbitts 2009-07-02 16:47:42 EDT
Your account, praveenp, is not in the packager group, so I've indicated that you require a sponsor.
Comment 3 Steve Traylen 2009-07-10 11:05:45 EDT
Hi Praveen,

 This is not an official review as am trying to become a package maintainer
 myself.


 Things to change I would say:

1)
$ rpmlint sblim-cmpi-rpm.spec 
sblim-cmpi-rpm.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 2)

2) There should be an initial changelog entry. See
http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

3) The build fails with
checking for CMPI headers... checking standard... checking /usr/include/cmpi... checking /usr/local/include/cmpi... checking /src/Pegasus/Provider/CMPI... checking /opt/tog-pegasus/include/Pegasus/Provider/CMPI... checking /usr/include/Pegasus/Provider/CMPI... checking /usr/include/openwbem... checking /usr/sniacimom/include... configure: error: no. Sorry cannot find CMPI headers files.

so I must be missing a BuildRequires I guess.

  Steve
Comment 4 Praveen K Paladugu 2009-07-13 09:29:23 EDT
Steve, 

 Made the require changes. Please verify now. 


Thank you 
Praveen
Comment 5 Steve Traylen 2009-07-23 14:15:24 EDT
Hi Praveen, 

 Slow reply, sorry.

It looks to be the same, I don't still have the old one lying around
to check if it is.

 checking for CMPI headers... checking standard... checking /usr/include/cmpi... checking /usr/local/include/cmpi... checking /src/Pegasus/Provider/CMPI... checking /opt/tog-pegasus/include/Pegasus/Provider/CMPI... checking /usr/include/Pegasus/Provider/CMPI... checking /usr/include/openwbem... checking /usr/sniacimom/include... configure: error: no. Sorry cannot find CMPI headers files.
error: Bad exit status from /var/tmp/rpm-tmp.r6y1BD (%build)


$ md5sum /tmp/sblim-cmpi-rpm-1.0.1-1.fc10.src.rpm 
cc21850d37fe2ba4d62cd259add38ae6  /tmp/sblim-cmpi-rpm-1.0.1-1.fc10.src.rpm

When you make a new rpm can you increase the Release number at least to 
avoid confusion.

Thanks,

 Steve
Comment 6 Praveen K Paladugu 2009-07-24 10:16:13 EDT
http://linux.dell.com/files/fedora/sblim-cmpi-rpm/sblim-cmpi-rpm-1.0.1-2.fc10.src.rpm

http://linux.dell.com/files/fedora/sblim-cmpi-rpm/sblim-cmpi-rpm.spec


I built the above code on mock, It seems I missed a dependency before.
I changed the version number as you mentioned. 


Thanks
Praveen
Comment 7 Martin Gieseking 2009-08-05 11:52:37 EDT
Praveen,

just a few annotations: :)

- Source0 must be
  http://downloads.sourceforge.net/sblim/%{name}-%{version}.tar.bz2
  (see https://fedoraproject.org/wiki/Packaging/SourceURL)

- remove INSTALL from %doc 

- add a changelog entry for each revision


$ rpmlint sblim-cmpi-rpm-1.0.1-2.fc10.src.rpm

sblim-cmpi-rpm.src: E: description-line-too-long  These CIM providers list the software packages installed on GNU/Linux system and provide some detail about them
sblim-cmpi-rpm.src: E: no-changelogname-tag
sblim-cmpi-rpm.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 2)
1 packages and 0 specfiles checked; 2 errors, 1 warnings.


Martin
Comment 9 Martin Gieseking 2009-08-07 05:33:41 EDT
Hello Praveen,

here is my review of your package. :)


rpmlint output:

sblim-cmpi-rpm.i586: E: devel-dependency rpm-devel
sblim-cmpi-rpm.i586: W: incoherent-version-in-changelog .0.1-3 ['1.0.1-3.fc11', '1.0.1-3']
sblim-cmpi-rpm.i586: E: library-without-ldconfig-postin /usr/lib/libcimrpm.so.0.0.0
sblim-cmpi-rpm.i586: E: library-without-ldconfig-postun /usr/lib/libcimrpm.so.0.0.0
sblim-cmpi-rpm.i586: E: library-without-ldconfig-postin /usr/lib/libcimrpmv4.so.0.0.0
sblim-cmpi-rpm.i586: E: library-without-ldconfig-postun /usr/lib/libcimrpmv4.so.0.0.0
sblim-cmpi-rpm-devel.i586: W: no-dependency-on sblim-cmpi-rpm/sblim-cmpi-rpm-libs/libsblim-cmpi-rpm
sblim-cmpi-rpm-devel.i586: W: summary-not-capitalized devel files for sblim-cmpi-rpm
4 packages and 0 specfiles checked; 5 errors, 3 warnings.


- Add a changelog entry for *every* revision so that the revision history can be reproduced (revision number is 3 now, so there must be 3 changelog entries, newest first)

- It wasn't necessary to shorten the description. The lines were just longer than 80 characters. Split long descriptions to several lines.


---------------------------------
keys used in following checklist:

[+] OK
[#] OK, not applicable
[-] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}
[-] MUST: The package must meet the Packaging Guidelines.
    - see rpmlint output 

[+] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: license file added to %doc 
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[-] MUST: The sources used to build the package must match the upstream source, as provided in the spec URL.
    - md5 hashes are different
    - please use the latest orignal tarball from upstream
   
[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[+] MUST: All build dependencies must be listed in BuildRequires
[#] MUST: The spec file MUST handle locales properly.
    - no locales

[-] MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
    - add %post and %postun scriptlets
    - see https://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries

[#] MUST: If the package is designed to be relocatable,...
    - package not relocatable
 
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in the spec file's %files listings.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[#] MUST: Large documentation files must go in a -doc subpackage.
    - no large documentation

[+] MUST: If a package includes something as %doc, it must not affect the runtime of the application.
[+] MUST: Header files must be in a -devel package.
[-] MUST: Static libraries must be in a -static package.
    - run configure with --disable-static to disable build of static libraries
    - see https://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries

[#] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
    - no pkgconfig files

[+] MUST: .so (without suffix) must go in a -devel package.
[-] MUST: devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
    - add Requires: %{name} = %{version}-%{release} to the -devel package

[-] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
    - remove the .la files in the %install section and remove them from %files devel

[#] MUST: Packages containing GUI applications must include a %{name}.desktop file
    - no GUI

[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: All filenames in rpm packages must be valid UTF-8.


[+] SHOULD: The reviewer should test that the package builds in mock.
    - builds in mock

[-] SHOULD: If scriptlets are used, those scriptlets must be sane.
    - no scriptlets
    - add %post and %postun (see above)
Comment 10 Praveen K Paladugu 2009-08-10 18:23:53 EDT
With the latest sources, had to add a patch to add libd.a among the libraries while linking. Without this there were some build failures because of the dlopen other dlxxxx function definitions missing.


http://linux.us.dell.com/files/fedora/sblim-cmpi-rpm/sblim-cmpi-rpm-1.0.1-3.fc10.src.rpm

http://linux.us.dell.com/files/fedora/sblim-cmpi-rpm/sblim-cmpi-rpm.spec


I added all the above changes suggested. Please verify.

Thank you
Praveen
Comment 11 Martin Gieseking 2009-08-11 05:38:01 EDT
Of course, you may apply patches to the sources but you must not change the tarball from upstream. Otherwise it's hard to check its validity.
Let rpmbuild do the work for you. Place the patch file into the SOURCES folder of your rpmbuild directory and add a Patch0 and a %patch0 entry to your spec file (this already seems to be done). rpmbuild will add the patch file to the SRPM and will apply it when creating the binary RPM.
So again, don't change the tarball (or other upstream files) directly.


Some minor things you should fix:

- Add a short comment above Patch0 on what the patch does

- change the two lines 
  rm -rf $RPM_BUILD_ROOT/%{_libdir}/cmpi/*.la
  rm -rf $RPM_BUILD_ROOT/%{_libdir}/*.la 
 
  into these:

  rm -f $RPM_BUILD_ROOT/%{_libdir}/cmpi/*.la
  rm -f $RPM_BUILD_ROOT/%{_libdir}/*.la 

  (the -r option is not necessary because you want to delete files here)


- The description text is a bit unclear. At least, I didn't understand what the package contains before reading the info on the upstream website. Please use the complete text from upstream: 

  These providers list the software packages (currently for RPM) installed on   
  GNU/Linux systems and provide some detail information about them. 

Since the text is longer than 80 characters, split it to several lines.

- in the changelog, add a blank between ">" and "-": 
  Mon Jul 13 2009 Praveen K Paladugu <praveen_paladugu@dell.com> - 1.0.1-2
  Thu Jul 02 2009 Praveen K Paladugu <praveen_paladugu@dell.com> - 1.0.1-1

  (see http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs)
Comment 12 Praveen K Paladugu 2009-08-11 10:04:54 EDT
Hey Martin,

 The upstream source files are intact. I never touched the sources. I added a patch file by the name "sblim_cmpi_rpm_ldl_library.patch" and patched the sources.

 The md5sum of the tar.bz2 file matches that of the upstream sources at :
http://downloads.sourceforge.net/project/sblim/sblim-cmpi-rpm/1.0.1/sblim-cmpi-rpm-1.0.1.tar.bz2


I also made the other suggested changes:

http://linux.us.dell.com/files/fedora/sblim-cmpi-rpm/sblim-cmpi-rpm-1.0.1-4.fc10.src.rpm

http://linux.us.dell.com/files/fedora/sblim-cmpi-rpm/sblim-cmpi-rpm.spec


Thank you
Praveen
Comment 13 Martin Gieseking 2009-08-11 10:45:47 EDT
Hello Praveen,

thanks for the fixes. Now, the md5sums match (5a6ebb04abe884b51416054e97777d64).
The tarball in your previous SRPM sblim-cmpi-rpm-1.0.1-3.fc10.src.rpm had a different hash value, and there was no patch file included with the SRPM:

$ rpm -qlp sblim-cmpi-rpm-1.0.1-3.fc10.src.rpm 
sblim-cmpi-rpm-1.0.1.tar.bz2
sblim-cmpi-rpm.spec

$ md5sum sblim-cmpi-rpm-1.0.1.tar.bz2
e1130e6a170f40e84e1a87d2cf41af8a  sblim-cmpi-rpm-1.0.1.tar.bz2

But now it's fixed. :)

Everything seems to be clean now. You should check whether the build also works with make %{?_smp_mflags}
If not, you can leave it as it is, and add a comment before the make statement that the smp flags break the builds. Besides of that, I think we can finish here.


The package is APPROVED.
Comment 14 Praveen K Paladugu 2009-08-11 10:57:29 EDT
Hey Martin, 
 I verified that "make %{?_smp_mflags}" doesn't work. 
 So, I will leave it as is.


 Thanks for your time.

Praveen
Comment 15 Praveen K Paladugu 2009-08-11 11:00:20 EDT
New Package CVS Request
=======================
Package Name: sblim-cmpi-rpm
Short Description: CIM access to installed software packages
Owners: praveenp
Branches: F-10 F-11 EL-4 EL-5
InitialCC: praveenp mdomsch
Comment 16 Kevin Fenzi 2009-08-12 19:58:46 EDT
cvs done.
Comment 17 Bill Nottingham 2010-01-19 23:51:40 EST
Was someone going to build this?
Comment 18 Praveen K Paladugu 2010-01-20 16:15:33 EST
I tried to build it once and there seem to be some function definitions missing.
I checked today and there are no updated sources available. So, the build still fails.



If you want to build this package, please go ahead.

Praveen
Comment 19 Bill Nottingham 2010-01-20 16:47:22 EST
You're the maintainer, I don't care one way or another whether it's built. I just thought it was odd that the package has never been built.
Comment 20 Praveen K Paladugu 2010-09-17 12:18:31 EDT
The packages are on git and the builds are fine. So closing this issue.

Note You need to log in before you can comment on or make changes to this bug.