Bug 250040 - Review Request: fmt-ptrn - A simple template system
Review Request: fmt-ptrn - A simple template system
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-29 21:19 EDT by W. Michael Petullo
Modified: 2007-12-23 15:21 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-12-23 15:21:48 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description W. Michael Petullo 2007-07-29 21:19:36 EDT
Spec URL: http://flyn.org/SRPMS/new.spec
SRPM URL: http://flyn.org/SRPMS/new-1.3.9-1.fc8.src.rpm
Description:
New is a template system, especially useful in conjunction with a simple text editor such as vi. The user maintains templates which may contain format strings. At run time, new replaces the format strings in a template with appropriate values to create a new file.

New has been orphaned for six months so it needs to be reviewed again.  I would like to un-orphan it.
Comment 1 Mamoru TASAKA 2007-08-06 22:41:28 EDT
Rebuilding this package failed.
http://koji.fedoraproject.org/koji/taskinfo?taskID=91592
Comment 2 Mamoru TASAKA 2007-08-13 02:00:15 EDT
ping?
Comment 3 W. Michael Petullo 2007-08-14 21:20:00 EDT
Spec URL: http://flyn.org/SRPMS/new.spec
SRPM URL: http://flyn.org/SRPMS/new-1.3.9-2.fc8.src.rpm
Comment 4 Mamoru TASAKA 2007-08-15 07:22:07 EDT
Before checking -2:
-------------------------------------------------------------
[tasaka1@localhost SRPMS]$ ls -al new-1.3.9-*/*gz
-rw-rw-r-- 1 tasaka1 tasaka1 686079 2007-07-30 10:02
new-1.3.9-1.fc8/new-1.3.9.tar.gz
-rw-rw-r-- 1 tasaka1 tasaka1 416816 2007-08-15 10:06
new-1.3.9-2.fc8/new-1.3.9.tar.gz
-------------------------------------------------------------
What happened??

Are you the upstream developer of this package? If so, please
don't alter the original tarball without changing version number
(so restore the original 1.3.9 tarball and release new one
 as 1.3.9.1, for example).

If not, where did you actually download them?
Comment 5 W. Michael Petullo 2007-08-16 19:43:02 EDT
I am the upstream maintainer of new.  New 1.3.9 hadn't been released yet.  1.3.9
is now available for download and the -2 package uses this released version.
Comment 6 Mamoru TASAKA 2007-08-18 11:06:06 EDT
For -2:

* rpmlint
  - rpmlint (for rpms) complaints about:
--------------------------------------------------
E: new non-executable-script /usr/share/new/templates/missing/default 0644
E: new non-executable-script /usr/share/new/templates/py/default 0644
E: new non-executable-script /usr/share/new/templates/sh/default 0644
W: new invalid-license GPL
W: new invalid-license GPL
W: new setup-not-quiet
W: new macro-in-%changelog files
W: new macro-in-%changelog doc
W: new mixed-use-of-spaces-and-tabs (spaces: line 22, tab: line 112)
W: new-debuginfo invalid-license GPL
W: new-devel no-documentation
W: new-devel invalid-license GPL
W: new-java no-documentation
E: new-java library-without-ldconfig-postin /usr/lib/libnewfmt_ptrnjni.so.1.3.9
E: new-java library-without-ldconfig-postun /usr/lib/libnewfmt_ptrnjni.so.1.3.9
E: new-java library-without-ldconfig-postin /usr/lib/libnewfmt_ptrnjava.so.1.3.9
E: new-java library-without-ldconfig-postun /usr/lib/libnewfmt_ptrnjava.so.1.3.9
W: new-java invalid-license GPL
------------------------------------------------------
   * Summary
     - Some scripts have shebangs but don't have executable permission.
       * If the scripts are to be executed by users directly, then
         the scripts should have executable permission
       * If they are to be called by other programs and are not to be
         executed directory, then shebangs should be removed.
     - Update License tag according to:
       http://fedoraproject.org/wiki/Packaging/LicensingGuidelines
       http://fedoraproject.org/wiki/Licensing
     - Supress verbose messages on %setup. i.e. use
       %setup -q
     - In the %changelog, please use %% (i.e. %%files, %%doc)
       to prevent macros from being expanded.
     - For identation, please use spaces OR tabs, not both.
     - Call ldconfig for libnewfmt_ptrnjni.so and so on (and
       check the section "dependency between main <-> subpackages"
       written below)

* parallel make support:
  - Rebuild failed on ppc (-j8)
    http://koji.fedoraproject.org/koji/taskinfo?taskID=108280
    When I disabled parallel make support, it seems okay
    http://koji.fedoraproject.org/koji/taskinfo?taskID=108298

    So, please either
    * temporally disable parallel make support
    * or fix to support parallel make correctly

* Unneeded dependency
----------------------------------------------------
Requires: zlib
----------------------------------------------------
  - This must be removed. rpmbuild checks and adds the dependency
    of libraries automatically and the dependency against libz.so
    should automatically pulls zlib.

* %makeinstall
  - Please don't use %makeinstall unless this is surely
    needed
    (check the section "Why the %makeinstall macro should not be used"
     of http://fedoraproject.org/wiki/Packaging/Guidelines )

* defattr
  - Now we recommand %defattr(-,root,root,-)

* dependency between main <-> subpackages
----------------------------------------------------
[tasaka1@localhost new]$ rpm -qp --requires new-1.3.9-2.fc8.1.i386.rpm | grep libnew
libnewfmt_ptrn.so.1  
libnewfmt_ptrnjava.so.1  
libnewfmt_ptrnjni.so.1  
libnewtemplate.so.1  
[tasaka1@localhost new]$ rpm -qp --provides new-1.3.9-2.fc8.1.i386.rpm | grep libnew
libnewfmt_ptrn.so.1  
libnewtemplate.so.1  
----------------------------------------------------
  - This is undesirable. This shows that new main package
    require libnewfmt_ptrnjava.so, but it is not provided by
    itself and so main package requires -java subpackage.
Comment 7 W. Michael Petullo 2007-08-18 20:37:51 EDT
Spec URL: http://flyn.org/SRPMS/new.spec
SRPM URL: http://flyn.org/SRPMS/new-1.3.10-1.fc7.src.rpm
Comment 8 Mamoru TASAKA 2007-08-19 08:42:07 EDT
For 1.3.10-1

* description
-------------------------------------------------------
// Copyright (C) 1999 %(FULLNAME) %(EMAIL)
-------------------------------------------------------
  - Please use %%(FULLNAME) %%(EMAIL) so that build log
    does not warn as below:
-------------------------------------------------------
sh: FULLNAME: command not found
sh: EMAIL: command not found
Installing /builddir/build/SRPMS/new-1.3.10-1.fc8.src.rpm
Building target platforms: i386
Building for target i386
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.30592
-------------------------------------------------------

* Documents
  - The file "INSTALL" is for people who want to rebuild
    this package by themselves and not needed for people
    who want to install by rpm method.

* rpmlint issues
  - Still some issues mentioned in the comment 6 are not fixed
    Also another issues are found.

-------------------------------------------------------
E: new non-executable-script /usr/share/new/templates/missing/default 0644
E: new non-executable-script /usr/share/new/templates/py/default 0644
E: new non-executable-script /usr/share/new/templates/sh/default 0644
W: new undefined-non-weak-symbol /usr/lib/libnewfmt_ptrn.so.1.3.10 template_find
W: new-java no-documentation
E: new-java library-without-ldconfig-postin /usr/lib/libnewfmt_ptrnjava.so.1.3.10
E: new-java library-without-ldconfig-postun /usr/lib/libnewfmt_ptrnjava.so.1.3.10
E: new-java library-without-ldconfig-postin /usr/lib/libnewfmt_ptrnjni.so.1.3.10
E: new-java library-without-ldconfig-postun /usr/lib/libnewfmt_ptrnjni.so.1.3.10
W: new-java unused-direct-shlib-dependency /usr/lib/libnewfmt_ptrnjava.so.1.3.10
/lib/libz.so.1
W: new-java unused-direct-shlib-dependency /usr/lib/libnewfmt_ptrnjava.so.1.3.10
/lib/libgcc_s.so.1
W: new-java unused-direct-shlib-dependency /usr/lib/libnewfmt_ptrnjava.so.1.3.10
/lib/libm.so.6
W: new-java unused-direct-shlib-dependency /usr/lib/libnewfmt_ptrnjava.so.1.3.10
/lib/libpthread.so.0
W: new-java unused-direct-shlib-dependency /usr/lib/libnewfmt_ptrnjava.so.1.3.10
/lib/librt.so.1
W: new-java unused-direct-shlib-dependency /usr/lib/libnewfmt_ptrnjava.so.1.3.10
/lib/libdl.so.2
W: new-java undefined-non-weak-symbol /usr/lib/libnewfmt_ptrnjni.so.1.3.10
fmt_ptrn_get_keys
W: new-java undefined-non-weak-symbol /usr/lib/libnewfmt_ptrnjni.so.1.3.10
fmt_ptrn_init
W: new-java undefined-non-weak-symbol /usr/lib/libnewfmt_ptrnjni.so.1.3.10
fmt_ptrn_close
W: new-java unused-direct-shlib-dependency /usr/lib/libnewfmt_ptrnjni.so.1.3.10
/lib/libz.so.1
W: new-devel no-documentation
--------------------------------------------------------
  * Summary
    * About /usr/share/new/templates/*/default:
      - Please fix the permission of these scripts or remove shebangs
        of these scripts.
    * libnewfmt_ptrnjni.so.1 and /usr/lib/libnewfmt_ptrnjava.so.1
      - Please call ldconfig for these libraries (i.e. -java subpackage
        also must call /sbin/ldconfig on %post and %postun)
    * Undefined non-weak symbols
      - are found on libnewfmt_ptrn.so and libnewfmt_ptrnjni.so.
        (can be checked as below:)
---------------------------------------------------------
[tasaka1@localhost ~]$ ldd -r /usr/lib/libnewfmt_ptrn.so
undefined symbol: template_find (/usr/lib/libnewfmt_ptrn.so)
        linux-gate.so.1 =>  (0x00110000)
        libglib-2.0.so.0 => /lib/libglib-2.0.so.0 (0x0011a000)
        libz.so.1 => /lib/libz.so.1 (0x001e3000)
        libc.so.6 => /lib/libc.so.6 (0x001f6000)
        /lib/ld-linux.so.2 (0x80000000)
---------------------------------------------------------
        This is not allowed when you want to provide -devel subpackage
        because leaving undefined non-weak symbols causes
        linkage failure.

* defattr
  - We recommend %defattr(-,root,root,-)
Comment 9 W. Michael Petullo 2007-08-19 18:27:08 EDT
Spec URL: http://flyn.org/SRPMS/new.spec
SRPM URL: http://flyn.org/SRPMS/new-1.3.10-2.fc7.src.rpm

The shebangs are not an issue.  These are templates, so the shebangs should
remain, but they are not in themselves executable scripts.

As far as the undefined symbols, this is really an issue in new.  New provides
two libraries, libnewfmt_ptrn and libnewtemplate.  I'm in the process of
determining how to decouple the fmt_ptrn library from the template library --
template_find is the one sticking point.

I hope that I can get this package released in the mean time.
Comment 10 Mamoru TASAKA 2007-08-20 07:21:06 EDT
Well, actually
* libnewfmt_ptrn.so has undefined non-weak symbol
* And libnewfmt_ptrnjni.so also has undefined non-weak symbols

Apart from these issues:
* one-line-command-in-%post(un)
  - If you want to call only /sbin/ldconfig on %post(un), please
    don't call unneeded shell. i.e. write as
---------------------------------------------------
%post -p /sbin/ldconfig
%post java -p /sbin/ldconfig
---------------------------------------------------
    for example, not
---------------------------------------------------
%post
/sbin/ldconfig
---------------------------------------------------
    The latter format calls unneeded shell (here /bin/bash).
Comment 11 W. Michael Petullo 2007-08-20 19:38:32 EDT
Spec URL: http://flyn.org/SRPMS/new.spec
SRPM URL: http://flyn.org/SRPMS/new-1.3.10-3.fc7.src.rpm
Comment 12 Mamoru TASAKA 2007-08-21 03:18:42 EDT
Still undefined non-weak symbols remain.
Also new.pc returns nothing..

--------------------------------------------
[root@localhost ~]# pkg-config --libs new
 
--------------------------------------------
Comment 13 W. Michael Petullo 2007-08-21 20:57:29 EDT
I updated new.pc so that "pkg-config --libs new" displays "-L/usr/lib
-lnewfmt_ptrn -lnewtemplate -lglib-2.0."  As I mentioned, I'd like to postpone
fixing the undefined non-weak symbols issue.

Spec URL: http://flyn.org/SRPMS/new.spec
SRPM URL: http://flyn.org/SRPMS/new-1.3.11-1.fc7.src.rpm
Comment 14 Ralf Corsepius 2007-08-21 23:59:39 EDT
Some remarks:

* configure script spews out a pretty drastic warning:
checking for check - version >= 0.8.2... no
*** Could not run check test program, checking why...
*** The test program failed to compile or link. See the file config.log for
*** the exact error that occured.
configure: WARNING: Check not found; cannot run unit tests!

Dunno how serious this warning is - If it shall be taken serious, then there the
spec lacks some dependencies, if it isn't serious, then this configure script is
a bit too "loud".


* pass --disable-static to %configure,
spares wasting time on building the static libs.


Finally, something more general, you will not like:

 I find calling a package "new" to not be a "clever decision", because naming
files and directories "new" is a pretty sure way to cause conflicts and clashes
in future.

Consider your package is shipping this: /usr/include/new
Would your package be named "list", you now would clash with c++.
"new" isn't in a better position. 
Comment 15 Mamoru TASAKA 2007-09-24 09:16:52 EDT
Michael, how do you think we should treat this review?
Comment 16 W. Michael Petullo 2007-10-01 22:01:23 EDT
Spec URL: http://flyn.org/SRPMS/new.spec
SRPM URL: http://flyn.org/SRPMS/new-1.3.11-2.fc8.src.rpm

I'd like to get this thing approved.

The objection regarding the configure/check warning is more appropriate to be
submitted as a bug against new. I fail to see why this would be relevant to
approving the package, other than violating Ralf's personal preferences. The
message clearly says "warning," so it is obviously not fatal.

I added --disable-static to my %configure line.

As far as the name, new has been in development since 1999. It has been included
in several distributions, including Fedora (I am only resubmitting it because
the package was orphaned). No one has complained about the name until now. 
Comment 17 Ralf Corsepius 2007-10-02 01:49:13 EDT
(In reply to comment #16)

>The objection regarding the configure/check warning is more appropriate to be
>submitted as a bug against new. I fail to see why this would be relevant to
>approving the package, other than violating Ralf's personal preferences. The
>message clearly says "warning," so it is obviously not fatal.

A configure script's warning normally indicates that a package is missing
something which will cause it to functionally regress. 

With you seemingly being the upstream, I would have expected you to improve this
rpm or to improve upstream's sources.

> As far as the name, new has been in development since 1999. It has been
> included
> in several distributions, including Fedora (I am only resubmitting it because
> the package was orphaned).
Shipped by you, seeminly maintained in Fedora by you.
Last patch applied to CVS on 2007-08-14 by you, submitted for review by you on
2007-07-29, ... ?!?

> No one has complained about the name until now. 
Well, the package name is non-critical.

What is critical is shipping /usr/include/new and /usr/bin/new. This is very
likely to clash with future development out of your control. 

To put it bluntly: It's a short-sighted design. 

I am inclined to think you only have been lucky to not see your package clash
with other packages, because all other developers in 30 years of *nix history
have avoided to implement "new" and thereby left a gap which you now are jumping
into.
Comment 18 W. Michael Petullo 2007-12-19 14:57:04 EST
Spec URL: http://flyn.org/SRPMS/fmt-ptrn.spec
SRPM URL: http://flyn.org/SRPMS/fmt-ptrn-1.3.12-1.fc8.src.rpm

- Removed use of AC_MSG_WARN from configure.in.
- Changed package name to fmt-ptrn.
- Changed /usr/bin/new to /usr/bin/nf.
- Changed C header location to /usr/include/fmt-ptrn.
- libnewfmt-ptrn.so no longer requires libnewtemplate.so.
Comment 19 Mamoru TASAKA 2007-12-21 12:31:00 EST
I will check this on this weekends.
Comment 20 Mamoru TASAKA 2007-12-22 08:21:16 EST
For 1.3.12-1:

* License
  - License tag must be "GPLv2+".

* Timestamps
  - Please use
-----------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
-----------------------------------------------------
    to keep timestamps on installed files. While sometimes
    this does not work, this method usually works on recent
    Makefiles.

* %defattr
  - Please unify to %defattr(-,root,root,-)

* undefined-non-weak-symbol rpmlint
  - From "$ rpmlint fmt-ptrn-java":
------------------------------------------------------
fmt-ptrn-java.i386: W: undefined-non-weak-symbol
/usr/lib/libnewfmt-ptrnjni.so.1.3.12 fmt_ptrn_get_keys
fmt-ptrn-java.i386: W: undefined-non-weak-symbol
/usr/lib/libnewfmt-ptrnjni.so.1.3.12 fmt_ptrn_init
fmt-ptrn-java.i386: W: undefined-non-weak-symbol
/usr/lib/libnewfmt-ptrnjni.so.1.3.12 fmt_ptrn_close
------------------------------------------------------
    Please fix this (perhaps linking against libnewfmt-ptrn.so should
    fix this issue)
Comment 21 W. Michael Petullo 2007-12-22 11:16:00 EST
Spec URL: http://flyn.org/SRPMS/fmt-ptrn.spec
SRPM URL: http://flyn.org/SRPMS/fmt-ptrn-1.3.13-1.fc8.src.rpm

This should address the issues raised in the previous comment.
Comment 22 Mamoru TASAKA 2007-12-22 12:53:08 EST
Okay, good.

-------------------------------------------------------------
    This package (fmt-ptrn) is APPROVED by me
-------------------------------------------------------------
Comment 23 W. Michael Petullo 2007-12-23 03:50:53 EST
New Package CVS Request
=======================
Package Name: fmt-ptrn
Short Description: Template system
Owners: mikep
Branches: F-8
InitialCC: 
Cvsextras Commits: yes
Comment 24 Kevin Fenzi 2007-12-23 13:11:34 EST
cvs done.

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