Bug 551838 - Review Request: opendchub - A hub software for Direct Connect
Summary: Review Request: opendchub - A hub software for Direct Connect
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-01-02 14:10 UTC by Roshan Singh
Modified: 2010-02-06 00:08 UTC (History)
4 users (show)

Fixed In Version: 0.8.1-5.fc11.1
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-01-21 17:23:08 UTC
mtasaka: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Roshan Singh 2010-01-02 14:10:02 UTC
Spec URL: http://roshansingh.fedorapeople.org/opendchub/opendchub.spec
SRPM URL: http://roshansingh.fedorapeople.org/opendchub/opendchub-0.8.1-1.fc12.src.rpm
Description: Opendchub is the hub software for Direct Connect P2P network. It runs as a daemon.

Comment 1 Mohamed El Morabity 2010-01-02 15:46:52 UTC
Hi,

here is an informal review of your package, while waiting to be sponsored ^^.
Just a few remarks about BR, the general look of the .spec seems pretty good ^^.

* since glibc-devel is a dependancy of gcc,
and since gcc is part of the minimal build system in Fedora (see
https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2), there is no
need to set glibc-devel as a BuildRequires.

* However, after checking the configure.in, I think you'll need to set perl-devel as a BuildRequires.

* By the way, according to the file configure.in, the BR libcap-devel is only required if the option « --enable-switch_user » is explicitely called. Otherwise it is useless.
If you think that this option is useful, it may be a good thing to enable it, to offer as many enabled features as possible in your binary.

You should check that you have defined all the required BuildRequires with mock:
   http://fedoraproject.org/wiki/Extras/MockTricks

Comment 2 Roshan Singh 2010-01-03 14:28:27 UTC
Hi,
Thanks for reviewing my package.

I have removed glibc-devel.

> * However, after checking the configure.in, I think you'll need to set
> perl-devel as a BuildRequires.

perl-ExtUtils-Embed needs perl-devel as its dependency, so i have not included it in BuildRequires.

> * By the way, according to the file configure.in, the BR libcap-devel is only
> required if the option « --enable-switch_user » is explicitely called.
> Otherwise it is useless.
> If you think that this option is useful, it may be a good thing to enable it,
> to offer as many enabled features as possible in your binary.

Thank you for noticing this. I have included it: 
%configure --enable-switch_user

I have checked it with mock, it is running fine. However it was first time that I was using mock. I would like you to take a look :-).

Link to updated spec and srpm:
SPEC: http://roshansingh.fedorapeople.org/opendchub/opendchub.spec
SRPM: http://roshansingh.fedorapeople.org/opendchub/opendchub-0.8.1-2.fc12.src.rpm

Comment 3 Mohamed El Morabity 2010-01-03 15:25:20 UTC
> perl-ExtUtils-Embed needs perl-devel as its dependency, so i have not included
> it in BuildRequires.
You're right, excellent :)

mock runs find indeed, and rpmlint is silent on your SRPM, your binary RPM and the debuginfo one.
I don't see any problem, I think your RPM is OK :-)

Just a little comment about the Group tag: isn't it more appropriate to set it to "
Applications/Internet"?
Just my personal opinion about your Description also, but I find it a little bit "laconic" ^^. Maybe you could use the description provided on the opendchub home page to make it more precise ^^.

Comment 4 Roshan Singh 2010-01-03 16:11:17 UTC
(In reply to comment #3)

> Applications/Internet"?
> Just my personal opinion about your Description also, but I find it a little
> bit "laconic" ^^. Maybe you could use the description provided on the opendchub
> home page to make it more precise ^^.  

I am the maintainer of the project as well. Thanks for the suggestion, I was also not sure in which group I should put it to.

I think this should fix all the issues. New spec and srpm:
SPEC: http://roshansingh.fedorapeople.org/opendchub/opendchub.spec
SRPM: http://roshansingh.fedorapeople.org/opendchub/opendchub-0.8.1-3.fc12.src.rpm

Comment 5 Mamoru TASAKA 2010-01-06 15:34:36 UTC
Well,

* perl module dependency
  https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides
  - When writing perl module rpm dependency as (Build)Requires,
    use the virtual provides on the package, not the rpm name itself
    (i.e. use "BuildRequires: perl(ExtUtils::Embed)"

* creating directory under home directory
  - configure.in reads:
-----------------------------------------------------------
    60  dnl Check if config directory exists.
    61  if test ! -d $HOME/.opendchub; then
    62     echo "creating config directory: $HOME/.opendchub"
    63     mkdir $HOME/.opendchub
    64     chmod 700 $HOME/.opendchub;
    65  fi
    66  
    75  if test "$ENABLE_PERL" = "yes"; then
    76     AC_CHECK_PROG(ENABLE_PERL, perl, yes, no)
    77     if test "$ENABLE_PERL" = "no"; then
    78        AC_MSG_WARN(Perl wasn't found. Scripting will be disabled.)
    79     else
    85         dnl Check if script directory exists.
    86         dnl Creates it and copies sample scripts to it if it doesn't.
    87         if test ! -d $HOME/.opendchub/scripts; then
    88            echo "creating script directory: $HOME/.opendchub/scripts"
    89            mkdir $HOME/.opendchub/scripts
    90            chmod 700 $HOME/.opendchub/scripts;
    91            for i in Samplescripts/*; do
    92                cp $i $HOME/.opendchub/scripts;
    93            done
    94         fi       
    95     fi
    96  else
    97      echo "Perl script support is disabled."
    98  fi
-----------------------------------------------------------
    and build.log says:
-----------------------------------------------------------
   129  Switch user support is enabled.
   130  creating config directory: /builddir/.opendchub
   131  checking for perl... yes
   132  creating script directory: /builddir/.opendchub/scripts
-----------------------------------------------------------
    Well,
    - build process should not create any directories under home
      directory. If these directories are needed, they should be
      created when the program is actually executed.
      Also Fedora forbids to create directories under home directory
      during build process:
      https://fedoraproject.org/wiki/Packaging/Guidelines#Scriplets_are_only_allowed_to_write_in_certain_directories

    - Furthermore, these created directories are not installed
      when installing this software with rpm anyway.

    So this behavior (i.e creating directories under home directory
    and installing some scripts under there during build) should
    be suppressed.
    - By the way maybe Samplescripts/ should be included as %doc.

* %changelog
  - It is recommended (and useful when using Fedora CVS) to insert
    one line between each %changelog entry like:
---------------------------------------------------------
* Sun Jan 3 2009 Roshan Kumar Singh <singh.roshan08@gmail.com> 0.8.1-3
- Changed Group to a more appropriate one

* Sun Jan 3 2009 Roshan Kumar Singh <singh.roshan08@gmail.com> 0.8.1-2
- Removed glibc-devel and added perl-devel to BR and changed configure

* Sat Jan 2 2009 Roshan Kumar Singh <singh.roshan08@gmail.com> 0.8.1-1
- First RPM for opendchub
---------------------------------------------------------

Comment 6 Roshan Singh 2010-01-07 03:23:47 UTC
(In reply to comment #5)
 
> * perl module dependency
>   https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides
>   - When writing perl module rpm dependency as (Build)Requires,
>     use the virtual provides on the package, not the rpm name itself
>     (i.e. use "BuildRequires: perl(ExtUtils::Embed)"

Done! The very next line in the above links says "packages link to libperl.so, usually to provide embedded perl functionality. All of these packages must also use the versioned MODULE_COMPAT Requires. " So I think this is needed as well

Requires:  perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

> 
> * creating directory under home directory
[snip]
>     Well,
>     - build process should not create any directories under home
>       directory. If these directories are needed, they should be
>       created when the program is actually executed.

The binary also creates the directories. I will remove this thing in the next release.

 
>     So this behavior (i.e creating directories under home directory
>     and installing some scripts under there during build) should
>     be suppressed.

I have done this by 

%build
%configure --enable-switch_user
rm -rf $HOME/.opendchub

Is it okay ?

Other changes have also been made. I will renew the SPEC after you confirm the changes.

Comment 7 Mamoru TASAKA 2010-01-07 16:57:00 UTC
(In reply to comment #6)
> (In reply to comment #5)
> Done! The very next line in the above links says "packages link to libperl.so,
> usually to provide embedded perl functionality. All of these packages must also
> use the versioned MODULE_COMPAT Requires. " So I think this is needed as well
> 
> Requires:  perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

- Because none of the binaries in this package are linked against
  libperl.so and this package does not contain any perl scripts,
  this Requires is not needed.

> > * creating directory under home directory
> I have done this by 
> 
> %build
> %configure --enable-switch_user
> rm -rf $HOME/.opendchub
> 
> Is it okay ?

- This is still undesired when rebuilding this srpm with usual rpmbuild
  because this actually removes ~/.opendchub, which may already be used
  by the user.
  You should apply a patch against configure.in not to create these
  directories and call autotools.

Comment 9 Mamoru TASAKA 2010-01-08 18:22:11 UTC
For -4:

* BR
  - Now this srpm calls autotools, "BR: automake" is needed
  ! Note
    After adding "BR: automake", build.log shows:
--------------------------------------------------------
   145  + make -j4
   146  (CDPATH="${ZSH_VERSION+.}:" && cd . && /bin/sh /builddir/build/BUILD/opendchub-0.8.1/missing --run autoheader)
   147  autoheader: WARNING: Using auxiliary files such as `acconfig.h', `config.h.bot'
--------------------------------------------------------
    It is not preferable that autotools are automatically called
    after configure. So "$ autoheader" should also be called
    before configure.

! Miscs
  - Would you divide a long line (for example with containing more
    than 80 characters) into two lines?

Comment 10 Roshan Singh 2010-01-09 14:17:46 UTC
(In reply to comment #9)
> For -4:
> 
> * BR
>   - Now this srpm calls autotools, "BR: automake" is needed

What about aclocal and autoconf, should they be also added to BR ?

>     After adding "BR: automake", build.log shows:
> --------------------------------------------------------
>    145  + make -j4
>    146  (CDPATH="${ZSH_VERSION+.}:" && cd . && /bin/sh
> /builddir/build/BUILD/opendchub-0.8.1/missing --run autoheader)
>    147  autoheader: WARNING: Using auxiliary files such as `acconfig.h',
> `config.h.bot'
> --------------------------------------------------------
>     It is not preferable that autotools are automatically called
>     after configure. So "$ autoheader" should also be called
>     before configure.

I added autoheader to %build before configure, even then autoheader is being called again.

Comment 11 Mamoru TASAKA 2010-01-09 14:24:07 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > For -4: 
> > * BR
> >   - Now this srpm calls autotools, "BR: automake" is needed
> What about aclocal and autoconf, should they be also added to BR ?

- /usr/bin/aclocal is in automake rpm, and automake has
  "Requires: autoconf"


> I added autoheader to %build before configure, even then autoheader is being
> called again.    

- Would you try aclocal -> autoheader -> autoconf -> automake ?

Comment 12 Roshan Singh 2010-01-09 14:31:41 UTC
(In reply to comment #11)

> > >   - Now this srpm calls autotools, "BR: automake" is needed
> > What about aclocal and autoconf, should they be also added to BR ?
> 
> - /usr/bin/aclocal is in automake rpm, and automake has
>   "Requires: autoconf"

ok.
 
> 
> > I added autoheader to %build before configure, even then autoheader is being
> > called again.    
> 
> - Would you try aclocal -> autoheader -> autoconf -> automake ?    

still the same. BTW how do you save the build.log. I use rpmbuild -ba to create rpm.

Comment 13 Mamoru TASAKA 2010-01-09 16:48:50 UTC
(In reply to comment #12)
> (In reply to comment #11)

> > > I added autoheader to %build before configure, even then autoheader is being
> > > called again.    
> > 
> > - Would you try aclocal -> autoheader -> autoconf -> automake ?    
> 
> still the same. 

- Then forcely modify the timestamps of autotools related files
  to prevent autotools from being called after configure like:
----------------------------------------------------
%prep
%setup -q
%patch0 -p0 -b .configure

aclocal
autoconf
automake
autoheader

touch -r configure \
	config.h.in \
	Makefile.in \
	aclocal.m4
----------------------------------------------------

> BTW how do you save the build.log. I use rpmbuild -ba to create
> rpm.    

- You can use "tee" command like
  $ rpmbuild -ba foo.spec 2>&1 | tee build.log

Comment 14 Roshan Singh 2010-01-09 17:07:14 UTC
Okay it is working fine now.

In the %doc section i think it will be better to copy the entire Samplescript directory instead of copying the files inside it, 

%doc AUTHORS ChangeLog COPYING NEWS README TODO Samplescripts

Is it okay ?

What is you nick on IRC ?

Comment 15 Mamoru TASAKA 2010-01-09 17:14:10 UTC
(In reply to comment #14)
> %doc AUTHORS ChangeLog COPYING NEWS README TODO Samplescripts
> 
> Is it okay ?
- No problem.
 
> What is you nick on IRC ?    
- I have never used IRC...

Comment 17 Mamoru TASAKA 2010-01-09 18:06:08 UTC
Okay.
-----------------------------------------------------
   This package (opendchub) is APPROVED by mtasaka
-----------------------------------------------------

Comment 18 Roshan Singh 2010-01-09 21:59:11 UTC
New Package CVS Request
=======================
Package Name: opendchub
Short Description: A hub software for Direct Connect
Owners: roshansingh
Branches: F-11 F-12

Comment 19 Jason Tibbitts 2010-01-12 06:27:00 UTC
CVS done (by process-cvs-requests.py)

Comment 20 Roshan Singh 2010-01-13 17:37:01 UTC
Today while I was importing the srpm there was some network problem and while upload it quit with and error message. I retried, it went ahead, but there seems to be a problem.

I am able to run 'make build' successfully on the devel branch but am not able to do it in F-11 and F-12. What should I do. There was a patch in the srpm which is not uploaded in F-12 only. However when I run make build Koji is complaining about the patch to be absent in both the branches F-11 and F-12.

What should I do.

Comment 21 Mamoru TASAKA 2010-01-13 19:04:14 UTC
Well,
- If you prefer to use "cvs-import.sh" (although I have never used this)
  * bump the release number to "5%{?dist}.1"
  * create new srpm again
  * and retry to import

If you don't want to use cvs-import.sh (and I don't use cvs-import.sh),
judging from http://cvs.fedoraproject.org/viewvc/rpms/opendchub/ :

For F-12: patch is actually missing
  * cvs add <patch>
  * bump the release number to "5%{?dist}.1"
  * cvs commit -> make tag -> make build

For F-11: patch is there, but not tagged, while spec file is already
tagged.
  * bump the release number to "5{?dist}.1"
  * cvs commit -> make tag -> make build

Comment 22 Mamoru TASAKA 2010-01-15 12:51:10 UTC
Please submit push requests on bodhi for F-12/11.

Comment 23 Fedora Update System 2010-01-17 09:45:31 UTC
opendchub-0.8.1-5.fc12.1 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/opendchub-0.8.1-5.fc12.1

Comment 24 Fedora Update System 2010-01-17 09:47:47 UTC
opendchub-0.8.1-5.fc11.1 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/opendchub-0.8.1-5.fc11.1

Comment 25 Fedora Update System 2010-01-19 00:55:51 UTC
opendchub-0.8.1-5.fc11.1 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update opendchub'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2010-0702

Comment 26 Fedora Update System 2010-01-19 00:56:05 UTC
opendchub-0.8.1-5.fc12.1 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update opendchub'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-0705

Comment 27 Mamoru TASAKA 2010-01-21 17:23:08 UTC
Closing.

Comment 28 Fedora Update System 2010-02-05 23:53:26 UTC
opendchub-0.8.1-5.fc12.1 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 29 Fedora Update System 2010-02-06 00:08:14 UTC
opendchub-0.8.1-5.fc11.1 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.


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