*** Bug 245689 has been marked as a duplicate of this bug. ***

Tim: I don't see you in the account system.  Is this your first package for Fedora?

Hi Jason.  Yes this is my first Fedora package.  I actually work with RHEL more
than Fedora so there might be a few mixups in that area as well.

I was trying to follow the PackageReviewProcess page in the Fedora Project Wiki
which seemed to imply that a Fedora account isn't required at this stage.  I'm
happy to be wrong about this though.

Please let me know what I need to do!


Thanks,

Tim.

I'm new, so I won't officially review this package, but let me start by giving
you a few pointers as best I can.

rpmlint returns:
[review@dao noarch]$ rpmlint python-pywbem-0.5-1.fc8.noarch.rpm 
python-pywbem.noarch: W: invalid-license LGPL

It needs a license version.  Refer to http://fedoraproject.org/wiki/Licensing

Out of the following musts (abridged list for relevancy):
      - MUST: rpmlint must be run on every package. The output should be posted
in the review.
Done
      - MUST: The package must be named according to the Package Naming Guidelines.
Good
      - MUST: The spec file name must match the base package %{name}, in the
format %{name}.spec unless your package has an exemption on Package Naming
Guidelines.
Good
      - MUST: The package must meet the Packaging Guidelines.
Good so far
      - MUST: The package must be licensed with a Fedora approved license and
meet the Licensing Guidelines.
I'm assuming you want the LGPLv2 for now, which passes.  Please be more explicit.
      - MUST: The License field in the package spec file must match the actual
license.
The actual license according to the README is LGPL.  According to one of the
files, it's LGPLv2+

      - MUST: If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package must be included in %doc.
Check
      - MUST: The spec file must be written in American English.
For what definition of American English?  Check ;)
      - MUST: The spec file for the package MUST be legible. If the reviewer is
unable to read the spec file, it will be impossible to perform a review. Fedora
is not the place for entries into the Obfuscated Code Contest
(http://www.ioccc.org/).
Legible
      - MUST: The sources used to build the package must match the upstream
source, as provided in the spec URL. Reviewers should use md5sum for this task.
If no upstream URL can be specified for this package, please see the Source URL
Guidelines for how to deal with this.
Good
      - MUST: The package must successfully compile and build into binary rpms
on at least one supported architecture.
Compiles on i386
      - MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in ExcludeArch. 
Not tested.  I should figure out how to do this.
      - MUST: All build dependencies must be listed in BuildRequires, except for
any that are listed in the exceptions section of Packaging Guidelines; inclusion
of those as BuildRequires is optional. Apply common sense.
I did not see any non standard python imports.  Check

      - MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory. Refer to the Guidelines for examples.
Check
      - MUST: A package must not contain any duplicate files in the %files listing.
Check (It lists a directory.  I recommend that it be changed to a file list if
you can.)
      - MUST: Permissions on files must be set properly. Executables should be
set with executable permissions, for example. Every %files section must include
a %defattr(...) line.
Check.
      - MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
CHeck
      - MUST: Each package must consistently use macros, as described in the
macros section of Packaging Guidelines.
Check
      - MUST: The package must contain code, or permissable content. This is
described in detail in the code vs. content section of Packaging Guidelines.
Check
      - MUST: If a package includes something as %doc, it must not affect the
runtime of the application. To summarize: If it is in %doc, the program must run
properly if it is not present.
Check
      - MUST: Packages must not own files or directories already owned by other
packages. 
AFAIC, Check
      - MUST: At the beginning of %install, each package MUST run rm -rf
%{buildroot} (or $RPM_BUILD_ROOT). See Prepping BuildRoot For %install for details.
Check
      - MUST: All filenames in rpm packages must be valid UTF-8.
AFAIC, Check

The following shoulds:
- SHOULD: If the source package does not include license text(s) as a separate
file from upstream, the packager SHOULD query upstream to include it.
Fail.  Please beg upstream for this.
- SHOULD: The description and summary sections in the package spec file should
contain translations for supported Non-English languages, if available.
Missing.  (My NB question, how is this accomplished?)
- SHOULD: The reviewer should test that the package builds in mock. See
MockTricks for details on how to do this.
- SHOULD: The package should compile and build into binary rpms on all supported
architectures.
Not yet tested.
- SHOULD: The reviewer should test that the package functions as described. A
package should not segfault instead of running, for example.
Not yet tested


Two final notes for now, Upstream has a new version out.  I checked, and it's
very similar.  I'm willing to continue this review on a new version.

The LICENSE file is missing, and this might make RMS angry.  Please contact
upstream to make sure his fine literature is disseminated in this package :).

Tim, any response?  This has been set to NEEDINFO for several weeks now; it will
be closed soon if there's no update.

Hi Jason and Yaakov.  I'll address the review points and update the package.


Thanks,

Tim.

I've updated the spec file for the new upstream release, as well as adding a
LICENSE.txt file which is implemented as a patch.

Spec URL: http://samba.org/~tpot/python-pywbem/python-pywbem.spec
SRPM URL: http://samba.org/~tpot/python-pywbem/python-pywbem-0.6-1.src.rpm


Regards,

Tim.

Well, now Tim has responded, but this ticket is assigned to Doug Chapman who has
yet to comment on this ticket.

Doug, did you intend to review this?

(In reply to comment #10)
> Well, now Tim has responded, but this ticket is assigned to Doug Chapman who has
> yet to comment on this ticket.
> 
> Doug, did you intend to review this?

I had grabbed this to help Tim try to push this through.  I actually don't have
the authority to do reviews (as far as I know).  So please by all means feel
free to take this back if you are ready to review it.

thanks,

- Doug

I am removing ASSIGNED status to New and removing Doug from Assigned To, so that other folks could view this as a new request and come to remove.

Thanks,

*typo, it was (last line)
...come to review :)

Recent changes to the infrastructure permit us to sponsor folks with much less overhead, so I thought I'd take a look at our oldest pending needsponsor ticket.  Honestly I think that anyone who still wants to get a package in after all this time deserves it.  Unfortunately, though, the package in comment 9 fails to build for me, in both rawhide and F-9:

RPM build errors:
error: Installed (but unpackaged) file(s) found:
   /usr/lib/python2.5/site-packages/pywbem-0.6-py2.5.egg-info
    Installed (but unpackaged) file(s) found:
   /usr/lib/python2.5/site-packages/pywbem-0.6-py2.5.egg-info

You'll need to package up the egg-info file that python generates as well.  The simplest way to do this that will also still work with F-8 (which doesn't generate the egg-info files) is to replace
  %{python_sitelib}/pywbem
with
  %{python_sitelib}/*
in your %files list.

Hi Jason.  Thanks for remembering me.  (-:  I've been a bit slack as well.

I'll take a look at the build errors and update the bugzilla.

Any updates?

I'm installing a F9 kvm as I type this!

OK that was easy.  I've updated the %files list for the egg info and uploaded some new versions:

http://samba.org/~tpot/python-pywbem/python-pywbem-0.7-1.fc9.src.rpm
http://samba.org/~tpot/python-pywbem/python-pywbem.spec

(This isn't the actual 0.7 version.  I've uploaded the latest subversion trunk for review purposes.  A new release should happen in a few days).

This builds fine; rpmlint says:

  python-pywbem.src: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 7)
Not a big deal; fix it if you like.

  python-pywbem.noarch: E: non-executable-script 
   /usr/lib/python2.5/site-packages/pywbem/mof_compiler.py 0644
  python-pywbem.noarch: E: non-executable-script 
   /usr/lib/python2.5/site-packages/pywbem/wbemcli.py 0644
These look like scripts (start with #!/whatever) but are not executable.  This happens often with python code; for some reason python developers tend to make files look like scripts even though nothing is ever supposed to run them.  If they are intended to be executed, though, they should be executable.  Otherwise I wouldn't worry about it, although you can remove the #! lines if you like.

  python-pywbem.noarch: W: incoherent-version-in-changelog 0.6-1
   ['0.7-1.fc10', '0.7-1']
Your last changelog entry is for version 0.6-1 but the package has version 0.7-1.  Each version/release bump should have a changelog entry.

So just one really problematic rpmlint complaint.

Obviously I can't check against the upstream tarball since it doesn't exist yet.

We don't generally provide license files when upstream doesn't include them.  All we do is bug upstream to include them when they're absent.  Since upstream seems to be you, I guess you can consider yourself bugged.  Maybe it can get into that final 0.7 tarball.

Also, things are a bit confusing, because README says LGPLv2 but the actual source files say LGPLv2+.  Can you clarify that?

There should be no need for the manual python dependency; rpm will generate an appropriate one for you.

? Can't check source against upstream since it hasn't been released yet.
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
? Actual license is not completely clear.
* license is open source-compatible.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
X rpmlint has valid complaints.
X manual python dependency is unnecessary:
   python-pywbem = 0.7-1.fc10
  =
X  python >= 2.3
   python(abi) = 2.5

* %check is not present; not test suite upstream.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

Thanks for the review.  I've updated the packaging as follows:

python-pywbem.src: Happy to fix but I don't know which file the error message is referring to!

mof_compiler.py: This file can actually be run as a standalone executable, or included as a module.  Personally I've used it more as an import so might just leave it in the site-packages directory and add permissions.  I'll check with the other PyWBEM author.

wbemcli.py: This is very handy.  I added it as a script to setup.py and it ended up being installed in /usr/bin which would be great.  Are there any policy problems with a python package installing stuff into /usr?

changelog: I added an entry for 0.7-1 after I uploaded the spec file so the error should be fixed now.

license: Hmm - the intent was LGPLv2 only.  I'll try and sort this out upstream.  The adding of the LICENSE.txt file is a bit of a hack.  Also need to coordinate this with upstream.

Thanks again!


Tim.

FYI, the spaces-and-tabs thing is referring to the .spec file. 

If you have scripts that you expect people to actually run, they should be in /usr/bin and not buried in some python directory.  It's perfectly acceptable for a python package to install executables in /usr/bin.

Any response?  If you've fixed any of the above issues, please give a link to the updated package.

Hi Jason.  I've made most of the fixes but haven't uploaded a new version just yet.  I'm pretty busy for the rest of the week but will sort things out as soon as I can.
(Also waiting for the official 0.7 release which hasn't happened yet).

Nuts - I was too slow to add LICENSE.txt to the 0.7.0 release, but it will be present in future releases.

Here's an updated version which fixes various rpmlint and other problems:

http://samba.org/~tpot/python-pywbem/python-pywbem.spec
http://samba.org/~tpot/python-pywbem/python-pywbem-0.7.0-1.fc9.src.rpm

Currently open issues:

  - LICENSE.txt patch still present, but can be removed for the next major or minor upstream release

  - rpmlint says "symlink-should-be-relative" for /usr/bin/mofcomp and /usr/bin/pywbemcli symlinks.  Is it official Fedora policy to have relative symlinks between different directories like this?

Getting closer...  (-:

I don't see why you need the LICENSE.txt patch.  Why not just remove it?  There is no requirement to ship the license text if the upstream does not do so.  When upstream gets around to including the license text, this package can then include it.  All of the program source files seem to have the correct license blocks so there is no confusion as to the actual license in use.

Yes, the symlinks need to be made relative.  Think about what happens when you install into a chroot.

Not sure why I thought I needed the license patch - probably just copied from the SUSE spec file.  It's gone now anyway.

Relative symlinks have been fixed and new files uploaded.

For what it's worth, I just used this package on F10 for an ESXi WBEM monitoring script for nagios, found here:
http://communities.vmware.com/docs/DOC-7170

Works great. Having pywbem will be a great addition to Fedora, thanks!

I think this looks fine now.  I will sponsor you.  It doesn't look like you've gotten as far as completing the CLA at this point, so you'll need to do that (which may involve HP legal; I don't know) and then apply for membership in the packager group.  I will sponsor you, although if it takes some time for you to get to that point then please ping me in this ticket as I might not notice when you apply.

APPROVED

Thanks Jason.  I've sent a request to HP legal and hopefully I can complete the CLA soon.

It's been nearly half a year since this was approved; has anything happened that would allow this to move forward?

Well, someone else has submitted a pywbem package and is willing to work on it, while this ticket is still sitting here.  I'm going to close this and work on the new one.  If you work out the legal issues (as apparently at least one other HP employee has been able to do) and still wish to be involved with pywbem maintenance then feel free to try to get sponsorship and apply to co-maintain the other package once it's in the distribution.

*** This bug has been marked as a duplicate of bug 508188 ***