[Bug 226483] New: Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226483

Summary: Merge Review: tcsh
           Product: Fedora Extras
           Version: devel
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: normal
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail]

Fedora Merge Review: tcsh

http://cvs.fedora.redhat.com/viewcvs/devel/tcsh/
Initial Owner: [e-mail]

[Bug 226483] Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: tcsh

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226483

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

------- Additional Comments From [e-mail]  2007-02-26 08:54 EST -------
(!!) 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.
********************

[Bug 226483] Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: tcsh

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226483

------- Additional Comments From [e-mail]  2007-02-26 11:02 EST -------
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.

[Bug 226483] Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226483

Jussi Lehtola  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |[e-mail]
         AssignedTo|[e-mail]    |[e-mail]
               Flag|                            |fedora-review?

--- Comment #3 from Jussi Lehtola   2009-04-05 05:01:39 EDT ---
Taking over review.

[Bug 226483] Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226483

--- Comment #4 from Jussi Lehtola   2009-04-05 05:22:03 EDT ---
- 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.

[Bug 226483] Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226483

--- Comment #5 from Jussi Lehtola   2009-04-05 05:39:13 EDT ---
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

[Bug 226483] Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226483

--- Comment #6 from Jussi Lehtola   2009-04-24 16:32:40 EDT ---
vcrhonek: please rectify.

[Bug 226483] Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226483

--- Comment #7 from Vitezslav Crhonek   2009-04-28 11:39:18 EDT ---
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).

[Bug 226483] Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226483

--- Comment #8 from Vitezslav Crhonek   2009-04-28 11:50:23 EDT ---
(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!

[Bug 226483] Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226483

Jussi Lehtola  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+

--- Comment #9 from Jussi Lehtola   2009-04-29 02:04:43 EDT ---
- 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

[Bug 226483] Merge Review: tcsh

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=226483

Vitezslav Crhonek  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |RAWHIDE

--- Comment #10 from Vitezslav Crhonek   2009-04-30 07:49:44 EDT ---
Built in tcsh-6.16-1.fc12