Bug 518082 - Review Request: rubygem-facade - A module that helps implement the facade pattern
Summary: Review Request: rubygem-facade - A module that helps implement the facade pat...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 518083
TreeView+ depends on / blocked
 
Reported: 2009-08-18 19:55 UTC by Brett Lentz
Modified: 2009-09-04 15:02 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2009-09-04 15:02:37 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Brett Lentz 2009-08-18 19:55:28 UTC
Spec URL: https://wakko.is-a-geek.com/~brett/rubygem-facade.spec
SRPM URL: https://wakko.is-a-geek.com/~brett/rubygem-facade-1.0.4-1.fc11.src.rpm
Description: A module that helps implement the facade pattern

Comment 1 Brett Lentz 2009-08-18 21:08:39 UTC
One more thing I forgot to add: 

This is my first package review request for Fedora, so I'm also going to need a sponsor.

Comment 2 Mamoru TASAKA 2009-08-20 18:13:02 UTC
Some notes:

* Unneeded macros
  - %ruby_sitelib does not seem to be used.

* define -> global
  - Now Fedora recommends to use %global instead of %define for
    some reasons:
    https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define
    https://fedoraproject.org/wiki/Packaging/Ruby#Build_Architecture_and_File_Placement

* License
  - README says facade is under "Artistic 2.0"

* About Source0
  - For gem files, please consider to use
    http://gems.rubyforge.org/gems/%{gemname}-%{version}.gem
    because with this URL you won't have to change "61520".

* Requires/BuildRequires
  - Please refer to:
    https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Packaging_Guidelines
    https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Gems
    ! Some Requires are missing.

  - For BuildRequire'ing gem module, using virtual dependency is preferred
    to using the actual rpm name, ref:
    https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides
    i.e. use "BuildRequires: rubygem(rake)" instead of "BuildRequires: rubygem-rake"

  - "Requires: ruby >= 1.8.2" is not needed (but please check the wiki
    page above)

* %setup, %build
  - Please write %setup, %build macro (even if they are empty)

* Macros usage should be consistent
  - Use macros consistently. If you want to use %mkdir_p, please use
    %rm for consistency

  - As you defines %geminstdir, please use the macro in %files

* Directory ownership issue
  - Some directories are not owned by this package properly, and
    some directories which should not be owned by this package are
    actually owned by this package. 
    Please read:
    https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership
    https://fedoraproject.org/wiki/Packaging:UnownedDirectories

    and make it sure that all directories which are newly created during
    the install of this package are correctly owned by this package.

* Test
  - As this package installs some files into under %geminstdir/test/,
    please create %check stage and execute some test programs in the stage.

Comment 3 Mamoru TASAKA 2009-08-20 18:15:22 UTC
On more thing
* BuildArch
  - Looks like this package should be noarch.

Comment 4 Brett Lentz 2009-08-20 20:13:08 UTC
New package and spec file uploaded to my webserver. Please take another look.

Comment 5 Mamoru TASAKA 2009-08-21 15:35:27 UTC
For -2:

* Requires
  - Still some Requires are missing:
    https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Packaging_Guidelines
     * "Requires: ruby(abi) = 1.8" is missing.
     * Also, for consistency I usually suggest to add 
       "BuildRequires: ruby(abi) = 1.8".

* About the following line:
----------------------------------------------------------
%{__mkdir_p} %{buildroot}%{_libdir}
----------------------------------------------------------
  - Perhaps "%{_mkdir_p} %{buildroot}%{gemdir}" is correct.

* Directory ownership issue
  - Still directory ownership issue is not correctly handled.
  ! If you try # rpm -ivv rubygem-facade-1.0.4-2.fc12.noarch.rpm,
    it will show:
----------------------------------------------------------
D: /usr/lib/ruby/gems/1.8/doc/facade-1.0.4 directory created with perms 0755, no context.
D: /usr/lib/ruby/gems/1.8/gems/facade-1.0.4 directory created with perms 0755, no context.
D: /usr/lib/ruby/gems/1.8/gems/facade-1.0.4/lib directory created with perms 0755, no context.
D: /usr/lib/ruby/gems/1.8/gems/facade-1.0.4/test directory created with perms 0755, no context.
----------------------------------------------------------
    Then for example:
----------------------------------------------------------
[tasaka1@localhost ~]$ rpm -qf /usr/lib/ruby/gems/1.8/gems/facade-1.0.4/README 
rubygem-facade-1.0.4-2.fc12.noarch
[tasaka1@localhost ~]$ rpm -qf /usr/lib/ruby/gems/1.8/gems/facade-1.0.4/       
file /usr/lib/ruby/gems/1.8/gems/facade-1.0.4 is not owned by any package
----------------------------------------------------------
    i.e. rubygem-facade binary rpm creates the directory %geminstdir
         to install "README" under there, however %geminstdir itself
         is not correctly owned by rubygem-facade, which is wrong.

    Again please check
    https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership
    https://fedoraproject.org/wiki/Packaging:UnownedDirectories
    especially:
    https://fedoraproject.org/wiki/Packaging:UnownedDirectories#Common_Mistakes
    and modify the %files entry.

    ! Especially:
      For example
------------------------------------------------------------
%files
%{geminstdir}/lib/
------------------------------------------------------------
      contains the directory %geminstdir/lib and all files/directories/etc
      under %{geminstdir}/lib.
    ! When fixing %files entry, please also check the section
      https://fedoraproject.org/wiki/Packaging/Guidelines#Duplicate_Files

By the way I will appreciate it if you post the full URL of your srpm/spec
on this bugzilla when you upload the new ones so that we can find them
easily.

Comment 6 Brett Lentz 2009-08-21 17:29:48 UTC
Ok. I've resolved the issues you mention. Please review one more time. 

https://wakko.is-a-geek.com/~brett/rubygem-facade.spec
https://wakko.is-a-geek.com/~brett/rubygem-facade-1.0.4-3.fc11.src.rpm

Comment 7 Mamoru TASAKA 2009-08-22 18:46:26 UTC
Well, I missed the following, however:

- I guess %{gemdir}/doc/%{gemname}-%{version}/ri should also be marked
  as doc. And the 3 lines:
-----------------------------------------------------
%files
%dir %{gemdir}/doc/%{gemname}-%{version}
%doc %{gemdir}/doc/%{gemname}-%{version}/rdoc
%{gemdir}/doc/%{gemname}-%{version}/ri
------------------------------------------------------
can be simplified as:
------------------------------------------------------
%files
%{gemdir}/doc/%{gemname}-%{version}/
------------------------------------------------------

Comment 9 Mamoru TASAKA 2009-08-26 18:19:52 UTC
Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
After you request for sponsorship a mail will be sent to sponsor 
members automatically (which is invisible for you) which notifies 
that you need a sponsor. After that, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 10/11, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Comment 10 Brett Lentz 2009-08-26 18:24:30 UTC
I have applied to the packagers group.

FAS Account name: wakko666

Comment 11 Mamoru TASAKA 2009-08-26 18:35:43 UTC
Okay, now I am sponsoring you. Please follow "Join" wiki again.

Comment 12 Kevin Fenzi 2009-08-29 03:23:34 UTC
Please add a cvs request template: 
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure
to let us know what you want.

Comment 13 Mamoru TASAKA 2009-08-29 15:08:14 UTC
Once resetting fedora-cvs flag.

Brett, please write the CVS request for new package following the
above URL and set fedora-cvs flag again.

Comment 14 Brett Lentz 2009-08-31 16:42:03 UTC
New Package CVS Request
=======================
Package Name: rubygem-facade
Short Description: A module that helps implement the facade pattern
Owners: wakko666
Branches: F-12
InitialCC: mtasaka

Comment 15 Mamoru TASAKA 2009-08-31 16:54:32 UTC
Well, "Branches F-12" means that you request for early branch?
https://www.redhat.com/archives/fedora-devel-list/2009-August/msg01435.html

Current stable branches are F-11 and F-10. As your srpm has
".fc11" suffix, I guess you want F-11 branch, not F-12.

Comment 16 Brett Lentz 2009-08-31 18:51:46 UTC
Is F-12 not the right thing to request?  I presume the %{?dist} macro will expand to whatever suffix is appropriate.

F-11 or F-12 (or both) is fine by me.

Comment 17 Jason Tibbitts 2009-08-31 18:56:13 UTC
I've already processed the CVS request as provided, although I assume that anyone who requests an F-12 branch at this point understands where their builds will go.

If you really wanted your packages to be available on F-11, you will need to make a new CVS request for an F-11 branch.

CVS done.

Comment 18 Mamoru TASAKA 2009-09-04 15:02:37 UTC
Closing.


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