Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 226483 - Merge Review: tcsh
Summary: Merge Review: tcsh
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 21:09 UTC by Nobody's working on this, feel free to take it
Modified: 2009-04-30 11:49 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-04-30 11:49:44 UTC
susi.lehtola: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 21:09:36 UTC
Fedora Merge Review: tcsh

http://cvs.fedora.redhat.com/viewcvs/devel/tcsh/
Initial Owner: mitr@redhat.com

Comment 1 Daniel Kopeček 2007-02-26 13:54:46 UTC
(!!) MUST: rpmlint output:
**** Review message:
W: tcsh invalid-license distributable

********************
(!!) MUST: Package must meet the Package Naming Guidelines
**** Review message:
%{?dist} tag is not present. Release should be: 14%{?dist}

********************
(!!) MUST: License field in spec must match actual license.
**** Review message:
- License: distributable
  According to http://directory.fsf.org/tcsh.html the license should be BSD

********************
(!!) MUST: The package must successfully compile/build on at least 1 architecture.
**** Review message:
- Package does not compile successfully.
  For me, it is due to the missing -ltermcap option when linking.
  Compiles successfully without the tinfo patch.

********************
(!!) MUST: The spec file MUST handle locales properly.
**** Review message:
- The package does not use the %find_lang macro

********************
(!!) SHOULD: Packager should query upstream for license text file.
**** Review message:
- License file is missing.
********************


Comment 2 Miloslav Trmač 2007-02-26 16:02:51 UTC
Thanks for the review.

> (!!) MUST: Package must meet the Package Naming Guidelines
> **** Review message:
> %{?dist} tag is not present. Release should be: 14%{?dist}
The dist tag is optional, see http://fedoraproject.org/wiki/Packaging/DistTag .

> (!!) MUST: License field in spec must match actual license.
> **** Review message:
> - License: distributable
>   According to http://directory.fsf.org/tcsh.html the license should be BSD
Updated.

> (!!) MUST: The package must successfully compile/build on at least 1 architecture.
> **** Review message:
> - Package does not compile successfully.
>   For me, it is due to the missing -ltermcap option when linking.
>   Compiles successfully without the tinfo patch.
Are you perhaps building on FC-6?  -ltinfo is only in rawhide ncurses, and tcsh
seems to build correctly using mock.

> (!!) MUST: The spec file MUST handle locales properly.
> **** Review message:
> - The package does not use the %find_lang macro
%find_lang works only on gettext message files, but tcsh uses catgets message
files.  The generated tcsh.file does mark the message files with the
corresponding %lang macro.

> (!!) SHOULD: Packager should query upstream for license text file.
> **** Review message:
> - License file is missing.
A patch was sent upstream.

Updated package is tcsh-6.14-15.

Comment 3 Susi Lehtola 2009-04-05 09:01:39 UTC
Taking over review.

Comment 4 Susi Lehtola 2009-04-05 09:22:03 UTC
- The patches are not commented. Comments should be added why any specific patch is needed.

- A newer version 6.16 has been released in September.

- Requires(post): grep and Requires(postun): coreutils, grep are a bit silly, aren't these already required by some minimal system rpm?

- Is the autoreconf really necessary?

- Drop the buildroot checks
[ -n "$RPM_BUILD_ROOT" -a "$RPM_BUILD_ROOT" != / ] 
in install and clean phase.

- Consider safening the %post and %postun phases with

%post
if [ ! -f /etc/shells ]; then
	echo "%{_bindir}/tcsh" >> /etc/shells
	echo "%{_bindir}/csh"  >> /etc/shells
else
	grep -q '^%{_bindir}/tcsh$' /etc/shells || \
	echo "%{_bindir}/tcsh" >> /etc/shells
	grep -q '^%{_bindir}/csh$'  /etc/shells || \
	echo "%{_bindir}/csh"  >> /etc/shells
fi

%postun
if [ ! -x %{_bindir}/tcsh ]; then
	grep -v '^%{_bindir}/tcsh$'  /etc/shells | \
	grep -v '^%{_bindir}/csh$' > /etc/shells.rpm &&
	mv /etc/shells.rpm /etc/shells
fi


- Package does not handle locales in the right manner. Installing manually is OK, but after that use %{find_lang} to build the file list.

Comment 5 Susi Lehtola 2009-04-05 09:39:13 UTC
rpmlint output:
tcsh.x86_64: W: file-not-utf8 /usr/share/doc/tcsh-6.15/Fixes
tcsh.x86_64: W: dangerous-command-in-%postun rm
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

- Convert Fixes file to utf8 with iconv.


MUST: The spec file for the package is legible and macros are used consistently. ~OK
- Some comments could be nice in the install phase, it would make the spec file a lot easier to read and understand.

MUST: The License field in the package spec file must match the actual license. NEEDSFIX
- The license in source code is 3-clause BSD, not BSD with advertising. Change License tag to BSD.
- Contact upstream to clarify this, since CopyRight is 4-clause.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. ~OK
- The file matches but source URL is bad; now the correct url has old/ before the release. Switch to newest release will fix this.

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. NEEDSFIX

MUST: Optflags are used and time stamps preserved. NEEDSFIX
- SMP make is not enabled.
- Use -p flag to install in install phase.

MUST: A package must own all directories that it creates or require the package that owns the directory. OK
- Please change %{_mandir}/*/* to %{_mandir}/man1/*.1, since it's safer this way.

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSFIX
- Include BUGS and WishList in the package. Remember to convert the files to utf8.

SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. 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: Packages containing shared library files must call ldconfig. OK
MUST: Files only listed once in %files listings. 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. OK
MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. OK
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. OK
MUST: If a package contains library files with a suffix then library files  ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
MUST: Packages does not contain any .la libtool archives. OK
MUST: Desktop files are installed properly. OK
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: The package builds in mock. OK

Comment 6 Susi Lehtola 2009-04-24 20:32:40 UTC
vcrhonek: please rectify.

Comment 7 Vitezslav Crhonek 2009-04-28 15:39:18 UTC
Hi Jussi,

Thanks for adding me to the CC.

(In reply to comment #4)
> - The patches are not commented. Comments should be added why any specific
> patch is needed.

I think it's good idea with new patches, but I don't want comment them retrospectively.

> 
> - A newer version 6.16 has been released in September.

Rebased.

> 
> - Requires(post): grep and Requires(postun): coreutils, grep are a bit silly,
> aren't these already required by some minimal system rpm?

Probably not (e. g. when you have busybox, then you don't need coreutils). We should rewrite these scripts to RPM Lua maybe...:)

> 
> - Is the autoreconf really necessary?

Yes, it is. Because of using ltinfo instead of ltermcap. We need to refresh configure.

> 
> - Drop the buildroot checks
> [ -n "$RPM_BUILD_ROOT" -a "$RPM_BUILD_ROOT" != / ] 
> in install and clean phase.

Fixed.

> 
> - Consider safening the %post and %postun phases with
> 
> %post
> if [ ! -f /etc/shells ]; then
>  echo "%{_bindir}/tcsh" >> /etc/shells
>  echo "%{_bindir}/csh"  >> /etc/shells
> else
>  grep -q '^%{_bindir}/tcsh$' /etc/shells || \
>  echo "%{_bindir}/tcsh" >> /etc/shells
>  grep -q '^%{_bindir}/csh$'  /etc/shells || \
>  echo "%{_bindir}/csh"  >> /etc/shells
> fi
> 
> %postun
> if [ ! -x %{_bindir}/tcsh ]; then
>  grep -v '^%{_bindir}/tcsh$'  /etc/shells | \
>  grep -v '^%{_bindir}/csh$' > /etc/shells.rpm &&
>  mv /etc/shells.rpm /etc/shells
> fi

Done.

> 
> 
> - Package does not handle locales in the right manner. Installing manually is
> OK, but after that use %{find_lang} to build the file list.  

See comment #2 from Miroslav, %{find_lang} doesn't work here. The file list (tcsh.lang) is assembled in %install, locales manually installed here too and finally packed in %files (taken from the file list).

Comment 8 Vitezslav Crhonek 2009-04-28 15:50:23 UTC
(In reply to comment #5)
> rpmlint output:
> tcsh.x86_64: W: file-not-utf8 /usr/share/doc/tcsh-6.15/Fixes
> tcsh.x86_64: W: dangerous-command-in-%postun rm
> 3 packages and 0 specfiles checked; 0 errors, 2 warnings.
> 
> - Convert Fixes file to utf8 with iconv.

Fixed.

> 
> 
> MUST: The spec file for the package is legible and macros are used
> consistently. ~OK
> - Some comments could be nice in the install phase, it would make the spec file
> a lot easier to read and understand.
> 
> MUST: The License field in the package spec file must match the actual license.
> NEEDSFIX
> - The license in source code is 3-clause BSD, not BSD with advertising. Change
> License tag to BSD.
> - Contact upstream to clarify this, since CopyRight is 4-clause.

Fixed (I was probably confused with this Copyrigt file).

> 
> MUST: The sources used to build the package must match the upstream source, as
> provided in the spec URL. ~OK
> - The file matches but source URL is bad; now the correct url has old/ before
> the release. Switch to newest release will fix this.

Fixed (rebase).

> 
> MUST: The package MUST successfully compile and build into binary rpms. OK
> MUST: The spec file MUST handle locales properly. NEEDSFIX

Commented before.

> 
> MUST: Optflags are used and time stamps preserved. NEEDSFIX
> - SMP make is not enabled.
> - Use -p flag to install in install phase.

Fixed.

> 
> MUST: A package must own all directories that it creates or require the package
> that owns the directory. OK
> - Please change %{_mandir}/*/* to %{_mandir}/man1/*.1, since it's safer this
> way.

Fixed.

> 
> MUST: All relevant items are included in %doc. Items in %doc do not affect
> runtime of application. NEEDSFIX
> - Include BUGS and WishList in the package. Remember to convert the files to
> utf8.

Fixed.

> 
> SHOULD: If the package does not include license text(s) as separate files from
> upstream, the packager should query upstream to include it. 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: Packages containing shared library files must call ldconfig. OK
> MUST: Files only listed once in %files listings. 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. OK
> MUST: Header files must be in a -devel package. OK
> MUST: Static libraries must be in a -static package. OK
> MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. OK
> MUST: If a package contains library files with a suffix then library files 
> ending in .so must go in a -devel package. OK
> MUST: In the vast majority of cases, devel packages must require the base
> package using a fully versioned dependency. OK
> MUST: Packages does not contain any .la libtool archives. OK
> MUST: Desktop files are installed properly. OK
> 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: The package builds in mock. OK  


Changes commited to the devel branch. Please check it and let me know your opinion. Thanks!

Comment 9 Susi Lehtola 2009-04-29 06:04:43 UTC
- Time stamps are not conserved. 

for i in Fixes WishList; do
    iconv -f iso-8859-1 -t utf-8 < "$i" > "${i}_"
    mv "${i}_" "$i"
done

should be

for i in Fixes WishList; do
    iconv -f iso-8859-1 -t utf-8 "$i" > "${i}_" && \
    touch -r "$i" "${i}_" && \
    mv "${i}_" "$i"
done

- Defattr should be
 (-,root,root,-)
not
 (-,root,root)


Fix these before building the new release. The package has been

APPROVED

Comment 10 Vitezslav Crhonek 2009-04-30 11:49:44 UTC
Built in tcsh-6.16-1.fc12


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