Bug 619437

Summary: Review Request: bm - Rpm package building helper
Product: [Fedora] Fedora Reporter: Helio Chissini de Castro <helio>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: jamatos, martin.gieseking, notting, package-review, rdieter, supercyper1, terjeros
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-05-20 10:39:50 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description Helio Chissini de Castro 2010-07-29 10:27:12 EDT
Spec URL: http://heliocastro.fedorapeople.org/bm.spec
SRPM URL: http://heliocastro.fedorapeople.org/bm-2.2-1.fc13.src.rpm
Description: bm, also known as BuildManager is a handy tool to build inplace rpms using a local rpm structure, without the need to be root or an specific environment setup for user, with ability to create logs of every operation performed.
It comes with a handy extra macros for use --without as a better bypass short-circuit during the development and test. As an example, "bm -l --without build" will short-circuit the package to the install stage, without clean up the build dir, so test installs in large packages will reduce time when something failed in install stage.
Comment 1 Randall "Randy" Berry 2010-07-29 11:34:49 EDT
This is not a full review just a couple of things that stick out in the spec file.

1) The license should be GPLv2+
2) Source: should be Source0: http://including a URL to download original tarball
3) Spec missing %changelog section.
4) Group should be Development/Tools

Is this your first package?
Comment 2 Helio Chissini de Castro 2010-07-29 12:50:56 EDT
> 1) The license should be GPLv2+
Fixed

> 2) Source: should be Source0: http://including a URL to download original
> tarball
Source come direct from Mandriva svn where we developed last years since the author, Gustavo Niemeyer left. So today tarball need be generated from there.

> 3) Spec missing %changelog section.
Fixed. 
Old habits from Mandriva where we have changelogs auto generated by buildsystem

> 4) Group should be Development/Tools
Fixed

> Is this your first package?    
For Fedora in a official way ? Yes
For my internal use ? No
First RPM.. nooooo,  a long noooo :-)
Comment 3 José Matos 2010-07-30 10:20:06 EDT
Hi Helio,
  it is good to see you here (as a kde user your name is quite familiar to me). :-)

Randall questions come from here
http://fedoraproject.org/wiki/PackageMaintainers/Join

So if this is your first package for Fedora the page advises:
http://fedoraproject.org/wiki/PackageMaintainers/Join#Create_Your_Review_Request

"You can add your package to this list by editing your review request bug and adding FE-NEEDSPONSOR in the 'Bug xyz blocks' field (where xyz is the bug number for your review request)."

Also since this is your first package you need to be sponsored in a process described here:

http://fedoraproject.org/wiki/PackageMaintainers/Join#Get_Sponsored

FWIW I am a sponsor and I can drive this process if you want and Randal does not oppose.

Notice that I have reset the assignee role since for reviews this is reserved for the reviewer and not for the submitter (since this field is already set to you).

If you have any questions do not hesitate to ask. :-)
Comment 4 Helio Chissini de Castro 2010-07-30 10:49:52 EDT
Hey, thanks

Rex Dieter already is my sponsor i think :-)

Most of the steps on the page for Join is already done, i have account. etc.. etc..

It's just time until i become adequate to the process.
Comment 5 Randall "Randy" Berry 2010-07-30 12:19:51 EDT
José,
By all means feel free to take the reins. I'm not that familiar with packaging python (yet) so I'll just sit back and observe. My comments were just things I noticed as a passer by.
Comment 6 Terje Røsten 2010-07-30 18:29:32 EDT
Hints for Python packages: 

 https://fedoraproject.org/wiki/Packaging:Python

As a start the --record=INSTALLED_FILES trick is not allowed, *.pyo files are not excluded and defattr use seems strange.

A link to a koji scratch build would be nice.
Comment 7 Helio Chissini de Castro 2010-08-02 06:17:52 EDT
Done.

Modified the package this time following the fedora python package guide.
Here's the koji entry

http://koji.fedoraproject.org/koji/taskinfo?taskID=2373182

Thanks
Comment 8 Randall "Randy" Berry 2010-08-02 06:56:05 EDT
Each time you make a change you have to note the change in the changelog section with version and release, bump the release up one, rebuild the SRPM and post your new spec and new SRPM in the review.

An example:

* Thu Jul 29 2010 Helio Chissini de Castro <helio@kde.org> 2.2-2
- Edit spec per review.
- Rebuild SRPM

* Thu Jul 29 2010 Helio Chissini de Castro <helio@kde.org> 2.2-1
- First release of bm based on mandriva release.
Comment 9 Terje Røsten 2010-08-02 08:36:48 EDT
> Modified the package this time following the fedora python package guide.
> Here's the koji entry
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=2373182

Thanks. Better, however still som issues

 - please post direct links to updates spec and srpm, this make it much simpler
to review your package
 - python_sitearch macro is not used and should be dropped
 - I don't understand why recompiling pyc files are needed, seems to work fine without
 - the recipe for getting the sources must be more explicit, it must be possible
   to redo the source tarball down to the same bits, that means using a revision
   number from svn repo. See 
   https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control
   for more information.
Comment 11 Martin Gieseking 2010-08-09 06:07:03 EDT
Helio, here are some more things to consider:

- If you want to build the package for Fedora only (and not for EPEL), you can remove the BuildRoot tag (and also 'rm -rf %{buildroot}' in %install) [1]. Otherwise, it's recommended to use one of the tags listed at [2].

- you can drop BR: python (it's added automatically as a dependency)

- add %dir %{_sysconfdir}/bash_completion.d/ to %files
  (the package should own this directory, see [3])


[1] http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

[2] http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag

[3] http://fedoraproject.org/wiki/Packaging/Guidelines#Multiple_packages_own_files_in_a_common_directory_but_none_of_them_needs_to_require_the_others.
Comment 12 Rex Dieter 2011-09-02 10:11:41 EDT
helio, I can pick this review up.
Comment 13 Rex Dieter 2012-01-07 10:56:51 EST
OH boy, I suck, forgot to follow up with anything useful here.  will try to do so this weekend.
Comment 14 Rex Dieter 2012-04-19 08:34:06 EDT
I suck, I'll leave this for others at least I can dedicate enough time to do more reviews properly.
Comment 15 Jason Tibbitts 2012-04-24 14:57:14 EDT
Is there a more recent package than the one in comment 10?  Any response from the submitter to comment 11?