Bug 173052 - Review Request: ttywatch
Review Request: ttywatch
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dmitry Butskoy
David Lawrence
http://domsch.com/linux/fedora/extras...
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-11-13 01:00 EST by Matt Domsch
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version: 0.14-4
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-11-29 14:19:12 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Suggested changes for the spec-file (3.10 KB, patch)
2005-11-23 11:39 EST, Dmitry Butskoy
no flags Details | Diff
some little nitpicks for %post sections (956 bytes, patch)
2005-11-29 08:44 EST, Dmitry Butskoy
no flags Details | Diff

  None (edit)
Description Matt Domsch 2005-11-13 01:00:35 EST
Spec Name or Url:
http://domsch.com/linux/fedora/extras/ttywatch/ttywatch.spec 
SRPM Name or Url:
http://domsch.com/linux/fedora/extras/ttywatch/ttywatch-0.14-1.src.rpm

Description:
ttywatch was originally designed to log serial console output from
lots of Linux machines on a single monitor machine.  It handles
log rotation correctly and can be configured both in a configuration
file and on the command line -- and you can mix-and-match at your
convenience.  It can be set up to allow users to interact via the
network with any of the ports being logged.  It can also log output
in arbitrary ways via modules, which can be built with the
ttywatch-devel package.

Builds clean in mock for {FC3,FC4,devel}x{i386,x86_64}.
Comment 1 Dmitry Butskoy 2005-11-23 11:37:32 EST
Suggestions:
- don't make separate devel package. Include 2k include-file into the main
package and add "Provides: ttywatch-devel" tag instead.

Remarks & nitpicks:
- it is possible to compile with $RPM_OPT_FLAGS, use OPTFLAGS=... for make line.
- as install pathes are hardcoded in Makefile, it is better to overwrite them at
"make install" time, setting appropriate Makefile's variables in cmdline with
appropriate rpm macros (%{_sbindir} etc.)
- %defattr(...) is enough, file attributes are set properly by "make install"
- IMO it is better to own /var/log/ttywatch by the package. The similar good
precedents are httpd and ppp ...
- IMHO it is better to use %{name} instead of hardcoded name...
Comment 2 Dmitry Butskoy 2005-11-23 11:39:56 EST
Created attachment 121407 [details]
Suggested changes for the spec-file
Comment 3 Matt Domsch 2005-11-23 15:02:45 EST
Thanks for the suggested changes.  All is fine, except the changing 
of /var/log/ttywatch.  As this is commonly used to log all serial traffic 
(i.e. serial consoles), passwords are highly likely items to be logged, and we 
really don't want rpm mucking with the permissions on upgrade if the sysadmin 
changes them.  So I've backed out that change and applied the rest.

rpmlint now complains about it, but it's got a big fat comment.
W: ttywatch dangerous-command-in-%post install

Likewise, this change causes rpmlint to complain:
W: ttywatch devel-file-in-non-devel-package /usr/include/ttywatch.h

but if that's OK, I'm OK with it.

http://domsch.com/linux/fedora/extras/ttywatch/ttywatch.spec 
http://domsch.com/linux/fedora/extras/ttywatch/ttywatch-0.14-1.src.rpm



Comment 4 Matt Domsch 2005-11-23 15:03:26 EST
That should have been:
http://domsch.com/linux/fedora/extras/ttywatch/ttywatch-0.14-2.src.rpm
Comment 5 Dmitry Butskoy 2005-11-24 04:22:38 EST
> we really don't want rpm mucking with the permissions on upgrade if the 
> sysadmin changes them.
I still not quite understand it.

By default, the /var/log/ttywatch permissions is "rwx------ root root". It is
the same default as, for example, /var/log/ppp and /var/log/httpd permissions.
But both "ppp" and "httpd" packages own its log directories...

What reasons can lead to necessity to change /var/log/ttywatch perms/owners? Why
the approximately similar Fedora stuff, for example /var/log/ppp, is not
affected by such reasons?

Anyway, according to http://fedoraproject.org/wiki/PackageReviewGuidelines, 
> - 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. The exception to this are directories listed explicitly 
> in the Filesystem Hierarchy Standard


Also, IMHO it is possible to use the recommended %{_smp_mflags} for the %build
stage.
Comment 6 Ralf Corsepius 2005-11-24 04:53:34 EST
(In reply to comment #3)
> Likewise, this change causes rpmlint to complain:
> W: ttywatch devel-file-in-non-devel-package /usr/include/ttywatch.h
> 
> but if that's OK, I'm OK with it.
Not OK with me. Shipping a header without a corresponding library is non-sense.
Comment 7 Dmitry Butskoy 2005-11-24 05:02:55 EST
(for comment #6):

> Shipping a header without a corresponding library is non-sense.

There is no any library at all. The header file is shipped just for building
additional plugins for the main executable.

BTW this header file is 2392-bytes only, shipping an extra "devel" package for
such amount of bytes is an ugly way. (There are precedents in FE for omitting of
separate devel package in such situations).
Comment 8 Ralf Corsepius 2005-11-24 05:15:07 EST
(In reply to comment #7)
> (for comment #6):
> 
> > Shipping a header without a corresponding library is non-sense.
> 
> There is no any library at all. The header file is shipped just for building
> additional plugins for the main executable.
Headers describe APIs to something. Therefore, shipping a header without the
corresponding implementation therefore is nonsense.

> BTW this header file is 2392-bytes only, shipping an extra "devel" package for
> such amount of bytes is an ugly way.
Shipping such a file is simply pollution.

> (There are precedents in FE for 
> omitting of > separate devel package in such situations).
Your package is missing the implementation of the devel package.

I don't want to fret about this, but unless you can provide an example or
describe how this header file is supposed to be used, I consider to veto against
this package. - I definitely won't approve it.
Comment 9 Dmitry Butskoy 2005-11-24 05:48:44 EST
> Headers describe APIs to something.
Sure. "something" is /usr/sbin/ttywatch itself.

ttywatch is linked properly with -ldl, i.e. can dlopen(3) plugins at runtime. To
write a plugin, a user should follow some io-data layouts/bits, which are
specified in /usr/include/ttywatch.h .

As there are no any referenses in /usr/sbin/ttywatch usable for extern plugins,
"-rdynamic" option at link time is not needed.

> I don't want to fret about this, but unless you can provide an example or
> describe how this header file is supposed to be used
Matt, could you write some example? BTW, it can be useful to include into %doc 

> I consider to veto against this package
Could you gpg-sign a text with veto report and reasons for such veto? ;)
Comment 10 Ville Skyttä 2005-11-24 07:02:04 EST
If you decide to keep the -devel provision, it should be made versioned, ie. 
Provides: %{name}-devel = %{version}-%{release} 
Comment 11 Matt Domsch 2005-11-28 14:36:31 EST
OK, I brought back the -devel package, and made ttywatch own /var/log/ttywatch.
$ rpmlint RPMS/x86_64/ttywatch-0.14-3.x86_64.rpm
E: ttywatch non-standard-dir-perm /var/log/ttywatch 0700

http://domsch.com/linux/fedora/extras/ttywatch/ttywatch.spec 
http://domsch.com/linux/fedora/extras/ttywatch/ttywatch-0.14-3.src.rpm
Comment 12 Dmitry Butskoy 2005-11-29 07:05:31 EST
> I brought back the -devel package
Why?..
IMHO there was good reasons to not split devel stuff as the separate package...

If you worry about rpmlint reports, try to check some popular Fedora Core's
packages for example. You will be surprised :) Actually the recommendation to
provide clean rpmlint output is not so strict. Sometimes Fedora behaviour
differs from rpmlint preferences. Sometimes (like 0700 "non-standard-dir-perm")
it "disallows" obviously needed things. Therefore follow rpmlint until it breaks
really useful things.

I still suggest to omit the separate devel package. Just specify "Provides" tag
as described in the comment #10

Comment 13 Matt Domsch 2005-11-29 07:56:01 EST
The upstream author (Michael K. Johnson, former Red Hatter and former Fedora
project leader) had a separate devel package in his original spec file.  It's
13KB as a separate RPM, and not I thought worth a long debate over including it
or breaking it out.  Could go either way, but may as well stay consistent with
the original.
Comment 14 Dmitry Butskoy 2005-11-29 08:15:30 EST
> It's 13KB as a separate RPM
It packages just 2.4KB header, accompanied with 18KB license file (because of
separate packaging).

Package splitting is useful for saving disk space/network traffic etc, but this
benefit takes effect when the package size is not too small. In the case the
splitted package is too small (and surely not requires extra -devel packages),
the splitting does not save the user disk space, but takes a cost as additional
file in the repository (and in the repository databases).

If you want to keep separate devel packages nevertheless, let it be so :/, it is
not a blocker.
Comment 15 Ville Skyttä 2005-11-29 08:22:32 EST
The devel package is missing a dependency on glib2-devel (#include <glib.h>).
Comment 16 Dmitry Butskoy 2005-11-29 08:31:42 EST
> glib2-devel
Oops!

Ville, thanks a lot!

Matt, save devel package! :) Just add "Requires: glib2-devel" to it.

Comment 17 Dmitry Butskoy 2005-11-29 08:44:08 EST
Created attachment 121581 [details]
some little nitpicks for %post sections

IMO it is better to do "Requires(post): /sbin/chkconfig" etc., as
/sbin/chkconfig and /sbin/service are required exactly. (Theoretically it is
possible that these utilities will go to another packages).
Comment 18 Dmitry Butskoy 2005-11-29 09:45:16 EST
Works fine!

MUST/SHOULD items will be OK after some required changes (comment #15 - comment #17)

Comment 20 Dmitry Butskoy 2005-11-29 10:26:12 EST
APPROVED!
Comment 21 Matt Domsch 2005-11-29 14:19:12 EST
Builds for devel, FC-4, FC-3 complete.  Closing.

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