Bug 509739 - Review Request: daemonize - run a command as a Unix daemon
Review Request: daemonize - run a command as a Unix daemon
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-05 12:40 EDT by Gary T. Giesen
Modified: 2009-08-25 00:16 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-08-25 00:16:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
susi.lehtola: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)
Patch to make build respect CFLAGS given by configure (913 bytes, patch)
2009-07-05 16:51 EDT, Susi Lehtola
no flags Details | Diff
New patch to have make honour CFLAGS passed to configure, as well as support DESTDIR and install man pages (1.23 KB, text/plain)
2009-07-05 18:20 EDT, Gary T. Giesen
no flags Details

  None (edit)
Description Gary T. Giesen 2009-07-05 12:40:30 EDT
Spec URL: http://dirtypackets.net/software/rpm/daemonize/daemonize.spec
SRPM URL: http://dirtypackets.net/software/rpm/daemonize/daemonize-1.5.4-1.src.rpm
Description: daemonize runs a command as a Unix daemon.
Comment 1 Gary T. Giesen 2009-07-05 12:53:41 EDT
This is a new package and I require a sponsor.


daemonize runs a command as a Unix daemon. As defined in W. Richard Stevens’
1990 book, Unix Network Programming (Addison-Wesley, 1990), a daemon is “a
process that executes ‘in the background’ (i.e., without an associated
terminal or login shell) either waiting for some event to occur, or waiting
to perform some specified task on a periodic basis.” Upon startup, a typical
daemon program will:

- Close all open file descriptors (especially standard input, standard output
  and standard error)
- Change its working directory to the root filesystem, to ensure that it
  doesn’t tie up another filesystem and prevent it from being unmounted
- Reset its umask value
- Run in the background (i.e., fork)
- Disassociate from its process group (usually a shell), to insulate itself
  from signals (such as HUP) sent to the process group
- Ignore all terminal I/O signals
- Disassociate from the control terminal (and take steps not to reacquire one)
- Handle any SIGCLD signals

Most programs that are designed to be run as daemons do that work for
themselves. However, you’ll occasionally run across one that does not.
When you must run a daemon program that does not properly make itself into a
true Unix daemon, you can use daemonize to force it to run as a true daemon.
Comment 2 Susi Lehtola 2009-07-05 14:53:23 EDT
- You don't need to specify
 -n daemonize-%{version}
in %setup, as this is the default value.

- What is
 $common_flags
doing in %configure?

- You are using incorrect characters in the description: change ’ to ' and “ to ".

- The provide
 Provides: daemonize
is redundant, since by default all packages provide themself (with a versioned provide).


I can sponsor you, if you show me you know the Fedora guidelines. You need to read and understand the packaging and review guidelines, and to demonstrate your knowledge you need to make at least one other submission, and do unofficial package reviews of packages of other people.
Comment 3 Gary T. Giesen 2009-07-05 15:37:03 EDT
(In reply to comment #2)
> - You don't need to specify
>  -n daemonize-%{version}
> in %setup, as this is the default value.
> 
> - What is
>  $common_flags
> doing in %configure?

I had used another package spec file as a template and I believe that was some garbage leftover.

> 
> - You are using incorrect characters in the description: change ’ to ' and “ to
> ".

Was a cut and paste from the package site. Corrected in updated spec file.

> 
> - The provide
>  Provides: daemonize

I believe I was following the convention in the template spec I was using, I have since reviewed that particular guideline, and corrected accordingly
.
> is redundant, since by default all packages provide themself (with a versioned
> provide).
> 
> 
> I can sponsor you, if you show me you know the Fedora guidelines. You need to
> read and understand the packaging and review guidelines, and to demonstrate
> your knowledge you need to make at least one other submission, and do
> unofficial package reviews of packages of other people.  

Understood. Will submit at least one other package and submit some reviews. I've also reviewed the packaging guidelines (again), as well as the review guidelines (http://fedoraproject.org/wiki/Packaging/ReviewGuidelines). I believe the updated package now meets all of those.

Thanks,

GG
Comment 4 Susi Lehtola 2009-07-05 15:49:32 EDT
Please post the updated package spec & srpm (increment the release tag and add a changelog entry).

Also, you might want to change the original entry from "RHEL" to "Fedora".
Comment 5 Gary T. Giesen 2009-07-05 15:51:50 EDT
The eventual goal for this package is EPEL, should I use a combined Fedora/EPEL or should I maintain two seperate spec files?
Comment 6 Susi Lehtola 2009-07-05 15:54:26 EDT
(In reply to comment #5)
> The eventual goal for this package is EPEL, should I use a combined Fedora/EPEL
> or should I maintain two seperate spec files?  

EPEL is a part of Fedora, not RHEL. And the primary target of Fedora is Fedora, EPEL is sort of an addon.

Also,
 install -m 0644 daemonize.1 %{buildroot}%{_mandir}/man1
should be
 install -p -m 0644 daemonize.1 %{buildroot}%{_mandir}/man1
to preserve timestamps, see
http://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

Quite often one needs to add INSTALL="install -p" as an argument to 'make
install' to make it preserve time stamps too.

Is the man page not installed automatically by make install..?

P.S. You don't have to create the directory, instead of 
 mkdir -p %{buildroot}%{_mandir}/man1
 install -p -m 0644 daemonize.1 %{buildroot}%{_mandir}/man1
you can
 install -D -p -m 0644 daemonize.1 %{buildroot}%{_mandir}/man1/daemonize.1
Comment 7 Gary T. Giesen 2009-07-05 16:27:00 EDT
Strangely enough, it's not installed automatically. A quick perusal of
Makefile.in reveals nothing that even touches that file.

Incremented release tag and posted update files to
http://dirtypackets.net/software/rpm/daemonize/

Made the changes to install as you suggested.
Comment 8 Susi Lehtola 2009-07-05 16:50:22 EDT
Here's the review:

rpmlint output is clean.

MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- Optflags are not used in compilation. (Patch to follow.)

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSWORK
- Don't include INSTALL, it is of no use in the package (it only contains building instructions).

MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
Comment 9 Susi Lehtola 2009-07-05 16:51:30 EDT
Created attachment 350551 [details]
Patch to make build respect CFLAGS given by configure
Comment 10 Susi Lehtola 2009-07-05 16:51:47 EDT
Please send the updated patch upstream.
Comment 11 Susi Lehtola 2009-07-05 16:54:03 EDT
The man file install is still missing from the makefile, you can add that as well to the patch.
Comment 12 Gary T. Giesen 2009-07-05 18:17:43 EDT
New build available at http://dirtypackets.net/software/rpm/daemonize/

Includes patch to Makefile that respects CFLAGS and installs man page. Adjusted spec file accordingly. Bumped release tag. Emailed new patch to upstream.
Comment 13 Gary T. Giesen 2009-07-05 18:20:00 EDT
Created attachment 350559 [details]
New patch to have make honour CFLAGS passed to configure, as well as support DESTDIR and install man pages
Comment 14 Susi Lehtola 2009-07-05 18:33:14 EDT
You missed the .c.o part:
 .c.o:
-	$(CC) -c $< -I.
+	$(CC) $(CFLAGS) -c $< -I.

(Also, you don't need to put the patches here too since you are the one who makes the srpms :)
Comment 15 Gary T. Giesen 2009-07-05 18:53:08 EDT
Let's try this one more time. Updated files are up. Fixed missing $(CFLAGS) in patch. Bumped rev. Everything should be good now.
Comment 16 Susi Lehtola 2009-07-05 19:02:59 EDT
Okay, but now the install process isn't preserving the time stamps. Add INSTALL="install -p" as argument to make install.

After you've fixed this the package should be good to go. I won't give an official approval yet, since to import you need to be sponsored, which I won't do before you fill the criteria (other submission and informal reviews).
Comment 17 Gary T. Giesen 2009-07-06 12:41:58 EDT
Updated package incorporating new release (1.5.6), which upstream has applied Makefile patches for. Incorporated changes to INSTALL="install -p"

Package is available at:

http://dirtypackets.net/software/rpm/daemonize/daemonize-1.5.6-1.src.rpm
http://dirtypackets.net/software/rpm/daemonize/daemonize.spec
Comment 18 Gary T. Giesen 2009-07-06 13:56:37 EDT
(In reply to comment #16)
> Okay, but now the install process isn't preserving the time stamps. Add
> INSTALL="install -p" as argument to make install.
> 
> After you've fixed this the package should be good to go. I won't give an
> official approval yet, since to import you need to be sponsored, which I won't
> do before you fill the criteria (other submission and informal reviews).  

Would you like me to add you to the CC for my other submissions?
Comment 19 Susi Lehtola 2009-07-06 17:08:47 EDT
Sure, and for the informal reviews too. Or you can just put the bug numbers here.
Comment 20 Gary T. Giesen 2009-07-06 19:52:45 EDT
Submitted new package for review:

Bug 509883 -  Review Request: sipcalc - "advanced" console based ip subnet calculator
Comment 21 Gary T. Giesen 2009-07-07 03:43:35 EDT
Submitted new package for review:

 Bug 509965 -  Review Request: snmptt - SNMPTT (SNMP Trap Translator) is an SNMP trap handler written in Perl
Comment 22 Gary T. Giesen 2009-07-08 01:02:32 EDT
Submitted review of someone else's package:

 Bug 510038 -  Review Request: python-icalendar - Parser and generator library for iCalendar files
Comment 23 Susi Lehtola 2009-07-08 05:16:15 EDT
I have now sponsored you. The package has been

APPROVED


Now you can do official reviews of Fedora packages. Which I hope you will, since
a) we have a huge review queue
and
b) reviewing is an easy way to improve Fedora to your liking; besides it's one-time-only work when compared to maintaining packages of your own.
Comment 24 Gary T. Giesen 2009-07-08 06:46:54 EDT
Thanks Jussi, you've been extremely helpful (and patient). I'll definitely continue with the reviews, since I found for myself it was invaluable way to learn the proper way to do packaging and improve my own packages.
Comment 25 Gary T. Giesen 2009-07-08 08:19:54 EDT
New Package CVS Request
=======================
Package Name: daemonize
Short Description: Run a command as a Unix daemon
Owners: giesen
Branches: F-10 F-11 EL-4 EL-5
InitialCC:
Comment 26 Jason Tibbitts 2009-07-08 12:32:49 EDT
CVS done.

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