Bug 502614 - Review Request: stfl - STFL implements a curses-based widget set for text terminals
Review Request: stfl - STFL implements a curses-based widget set for text te...
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: 2009-05-26 09:35 EDT by Thomas Janssen
Modified: 2009-10-23 12:16 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-10-23 10:15:18 EDT
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 Thomas Janssen 2009-05-26 09:35:18 EDT
Spec URL: http://thomasj.fedorapeople.org/stfl.spec
SRPM URL: http://thomasj.fedorapeople.org/stfl-0.20-1.fc10.src.rpm
Description: STFL is a library which implements a curses-based widget set for text
 terminals.
It is needed for another (the main app i want to package for fedora) package called Newsbeuter.
Comment 1 Thomas Janssen 2009-05-29 14:00:05 EDT
Scratch koji-build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1380908

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 2 Byron Clark 2009-05-31 09:00:57 EDT
I started on this package a while ago and just sent my patches upstream last night.  If you're interested, here is a version with those patches.  It gets rid of all the sed trickery.

Spec URL: http://theclarkfamily.name/fedora/stfl.spec
SRPM URL: http://theclarkfamily.name/fedora/stfl-0.20-3.fc11.src.rpm
Comment 3 Thomas Janssen 2009-05-31 10:01:33 EDT
You started earlier on that. You should maintain it. I just need it for Newsbeuter. So if you're interested to maintain it in Fedora, please take it.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 4 Byron Clark 2009-05-31 10:10:16 EDT
I was really only packaging it for newsbeuter, but I would be happy to maintain the package.
Comment 5 Thomas Janssen 2009-05-31 11:29:55 EDT
Ok. It's all yours. Thanks for your contribution :)

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 6 Susi Lehtola 2009-06-06 14:29:57 EDT
Devel packages (normally) require the main package, so there's no need to BR perl, python, ruby since you're already BR'ing perl-devel, python-devel and ruby-devel.
Comment 7 Byron Clark 2009-06-06 19:11:34 EDT
I removed the python and perl BuildRequires.  Unfortunately, I think the ruby BuildRequire needs to stay because I need the ruby executable and ruby-devel only seems to pull in ruby-libs.

Spec URL: http://theclarkfamily.name/fedora/stfl.spec
SRPM URL: http://theclarkfamily.name/fedora/stfl-0.20-4.fc11.src.rpm

Also, this is my first package so I'll need a sponsor.
Comment 8 Susi Lehtola 2009-06-10 08:54:07 EDT
You are not supposed to use both python_sitelib and python_sitearch. If the package contains architecture specific components you need to use sitearch, else sitelib.

If you're building on 32-bit, these will point to the same location. On 64-bit architectures the build will fail.
Comment 9 Byron Clark 2009-06-10 09:29:35 EDT
(In reply to comment #8)
> You are not supposed to use both python_sitelib and python_sitearch. If the
> package contains architecture specific components you need to use sitearch,
> else sitelib.

The build now only uses python_sitearch.  This also included modifying a patch to the stfl buildsystem.  The patch has been submitted upstream.

Spec URL: http://theclarkfamily.name/fedora/stfl.spec
SRPM URL: http://theclarkfamily.name/fedora/stfl-0.20-5.fc11.src.rpm
Comment 10 Mamoru TASAKA 2009-06-11 23:08:23 EDT
Note that currently I sponsor Byron.
Comment 11 Byron Clark 2009-06-28 22:25:43 EDT
New upstream version.  Should be rpmlint clean.

Spec URL: http://theclarkfamily.name/fedora/stfl.spec
SRPM URL: http://theclarkfamily.name/fedora/stfl-0.21-1.fc11.src.rpm
Comment 12 Jason Tibbitts 2009-07-12 20:59:49 EDT
I get the following rpmlint complaints:

stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 COLORS                                         
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 stdscr                                         
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 acs_map
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 COLOR_PAIRS
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 newwin
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 waddch
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 wmove
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 keypad
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 endwin
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 use_default_colors
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 waddnwstr
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 doupdate
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 delwin
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 werase
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 initscr
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 pair_content
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 nonl
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 wvline
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 wget_wch
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 wrefresh
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 init_pair
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 wtimeout
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 cbreak
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 start_color
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 wcolor_set
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 whline
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 noecho
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 wbkgdset
stfl.x86_64: W: undefined-non-weak-symbol /usr/lib64/libstfl.so.0.21 keyname

I guess there's some library that this one isn't linked against, and its expected that anything which will use this library will link it in instead.  Is that the case?
Comment 13 Byron Clark 2009-07-12 21:49:32 EDT
Yes, users of stfl should be linking libcursesw.so.  Just a curiousity, I'm not seeing those warnings from rpmlint on my system, am I missing some configuration?
Comment 14 Jason Tibbitts 2009-07-12 22:07:01 EDT
You must install the package and then run "rpmlint stfl" to see the final set of rpmlint checks.
Comment 15 Mamoru TASAKA 2009-08-27 03:24:20 EDT
What is the status of this bug?
Comment 16 Thomas Janssen 2009-08-27 04:53:53 EDT
Good question. You flagged me with needinfo. I have no idea what's up with Byron. He wanted to maintain the package. If he gives up, i can take it.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 17 Mamoru TASAKA 2009-08-27 06:52:24 EDT
(In reply to comment #16)
> Good question. You flagged me with needinfo. I have no idea what's up with
> Byron. He wanted to maintain the package. If he gives up, i can take it.

Ah, okay. Setting NEEDINFO for Byron.
Comment 18 Byron Clark 2009-08-27 08:22:13 EDT
I don't see any unanswered questions or concerns, what further information is needed?
Comment 19 Mamoru TASAKA 2009-08-27 12:01:25 EDT
(In reply to comment #18)
> I don't see any unanswered questions or concerns, what further information is
> needed?  

At least you should fix the issue raised by Jason (comment 12).
You can get these rpmlint warnings by

$ rpmlint stfl

after installing stfl binary rpm (note that rpmlint can also be
used for installed rpms).
Comment 20 Byron Clark 2009-08-27 23:51:36 EDT
Those warnings are present becuase users of libstfl.so must also link against libncursesw.so.  Is this something that needs to be fixed?
Comment 21 Mamoru TASAKA 2009-08-28 04:46:20 EDT
Are the any reason why libstfl.so _itself_ is not linked against
libncursesw.so?
Comment 22 Mamoru TASAKA 2009-09-28 13:59:46 EDT
ping?
Comment 23 Byron Clark 2009-09-28 17:08:55 EDT
Sorry, I'm swamped.  Thomas: do you want to take this one back?
Comment 24 Thomas Janssen 2009-09-29 07:02:55 EDT
I can take it back yes. Will check/fix it tomorrow and update then here.
What's with Newsbeuter? https://bugzilla.redhat.com/show_bug.cgi?id=503336

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 25 Jan Klepek 2009-09-29 07:30:01 EDT
Newsbeuter is blocked till this is reviewed
Comment 26 Thomas Janssen 2009-09-29 09:32:46 EDT
Heh, yeah, i know that. My bad..

Byron, should i take Newsbeuter as well?

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 27 Byron Clark 2009-09-29 13:13:49 EDT
Thomas, newsbeuter is all yours also.
Comment 28 Thomas Janssen 2009-10-02 05:26:15 EDT
Ok. Did some minor spec changes like ($RPM_BUILD_ROOT to %{buildroot}), converting to tabs instead of spaces. Added Requires pkgconfig.

Noted the rpmlint output after installation to upstream:
http://www.rocklinux.net/pipermail/stfl/2009-October/000115.html
Waiting for answer there.

According to some threads from IIRC fedora-devel ML, is
"W: undefined-non-weak-symbol" not a blocker criteria.
Though i will take care that it gets fixed.

By the way, the libstfl.so is linked against libncursesw.so
At least according to the Makefile: export LDLIBS += -lncursesw
From my understanding.

Spec URL: http://thomasj.fedorapeople.org/reviews/stfl.spec
SRPM URL: http://thomasj.fedorapeople.org/reviews/stfl-0.21-2.fc10.src.rpm

[thomas@tusdell SPECS]$ rpmlint stfl.spec ../SRPMS/stfl-0.21-2.fc10.src.rpm ../RPMS/x86_64/stfl*-2*
7 packages and 1 specfiles checked; 0 errors, 0 warnings.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1723919

I'm already sponsored.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 29 Thomas Janssen 2009-10-02 09:58:30 EDT
Fixed installed rpmlint output.

Spec URL: http://thomasj.fedorapeople.org/reviews/stfl.spec
SRPM URL: http://thomasj.fedorapeople.org/reviews/stfl-0.21-3.fc10.src.rpm

[thomas@tusdell SPECS]$ rpmlint stfl.spec ../SRPMS/stfl-0.21-3.fc10.src.rpm ../RPMS/x86_64/stfl*-3*
7 packages and 1 specfiles checked; 0 errors, 0 warnings.

[thomas@tusdell ~]$ rpmlint stfl
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1724203


-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 30 Mamoru TASAKA 2009-10-04 14:33:14 EDT
Some random notes

* Requires
  - There is no needed that main package (stfl) should have
    "Requires: pkgconfig".

  - "Requires: python" is not needed for -python subpackage. 
    python(abi) dependency is automatically added by rpmbuild itself.

* About sed/patch
  - Well, I am one of the person who uses sed command many times
    in spec files, however as you already created Patch0/1,
    I think it is better that you create Patch2 instead of
    using sed (and also see below: at least one more patch
    is needed)

* Fedora specific compilation flags
  - are not honored correctly (check the arguments passed to gcc in
    build.log). For example:
-------------------------------------------------------------------------
    57  gcc -pthread -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC   -c -o public.o public.c
    58  gcc -pthread -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC   -c -o base.o base.c
    59  gcc -pthread -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC   -c -o parser.o parser.c
    60  gcc -pthread -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC   -c -o dump.o dump.c
    61  gcc -pthread -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC   -c -o style.o style.c
....
-------------------------------------------------------------------------

* pkgconfig file
  - stfl.pc.in contains:
-------------------------------------------------------------------------
     5  libdir=${exec_prefix}/lib
-------------------------------------------------------------------------
    This is wrong on 64 bits architecure because libdir should be
    /usr/lib64 there.

* Duplicate documents
  - You don't have to include the same documents in each subpackage
    when other package which the package depends on (usually the main
    package) already contains the documents.

* %exclude
  - I prefer to remove unneeded files at %install instead of using
    %exclude unless unavoided.
Comment 31 Thomas Janssen 2009-10-05 10:43:04 EDT
(In reply to comment #30)
> Some random notes
> 
> * Requires
>   - There is no needed that main package (stfl) should have
>     "Requires: pkgconfig".

Fixed
 
>   - "Requires: python" is not needed for -python subpackage. 
>     python(abi) dependency is automatically added by rpmbuild itself.

Fixed

> * About sed/patch
>   - Well, I am one of the person who uses sed command many times
>     in spec files, however as you already created Patch0/1,
>     I think it is better that you create Patch2 instead of
>     using sed (and also see below: at least one more patch
>     is needed)

Fixed, but i converted the patches to sed. I like sed over patch (Byron did the patch stuff).

> * Fedora specific compilation flags
>   - are not honored correctly (check the arguments passed to gcc in
>     build.log). For example:
> -------------------------------------------------------------------------
>     57  gcc -pthread -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC   -c -o public.o
> public.c
>     58  gcc -pthread -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC   -c -o base.o
> base.c
>     59  gcc -pthread -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC   -c -o parser.o
> parser.c
>     60  gcc -pthread -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC   -c -o dump.o
> dump.c
>     61  gcc -pthread -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC   -c -o style.o
> style.c
> ....
> -------------------------------------------------------------------------

No idea what to do here. If i sed the export CFLAGS from the Makefile and use §RPM_OPT_FLAGS it fails miserably to build. If i add the optflags and dont sed nothing changes.

> * pkgconfig file
>   - stfl.pc.in contains:
> -------------------------------------------------------------------------
>      5  libdir=${exec_prefix}/lib
> -------------------------------------------------------------------------
>     This is wrong on 64 bits architecure because libdir should be
>     /usr/lib64 there.

Fixed and patch sent to upstream.

> * Duplicate documents
>   - You don't have to include the same documents in each subpackage
>     when other package which the package depends on (usually the main
>     package) already contains the documents.

Removed. Of course i get a rpmlint warning about the missing docs for those packages.
 
> * %exclude
>   - I prefer to remove unneeded files at %install instead of using
>     %exclude unless unavoided.  

I was able to rm -f one of the three. Two %exlude are still in since i honestly dont know exactly where they come from. I'm not a coder. They just come up as:

Writing /home/thomas/rpmbuild/BUILDROOT/stfl-0.21-4.fc10.x86_64/usr/lib64/perl5/vendor_perl/5.10.0/x86_64-linux-thread-multi/auto/stfl/.packlist                                            
Appending installation info to /home/thomas/rpmbuild/BUILDROOT/stfl-0.21-4.fc10.x86_64/usr/lib64/perl5/5.10.0/x86_64-linux-thread-multi/perllocal.pod

Nothing i was able to find out with checking the source code. But as i said, i'm not a coder. I can find *some* stuff and fix it. Not yet good enough to understand everything.
So i leave it in.

Thanks by the way for reviewing it.

Spec URL: http://thomasj.fedorapeople.org/reviews/stfl.spec
SRPM URL: http://thomasj.fedorapeople.org/reviews/stfl-0.21-4.fc10.src.rpm

http://koji.fedoraproject.org/koji/taskinfo?taskID=1728311

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 32 Mamoru TASAKA 2009-10-05 14:35:23 EDT
For -4:

* About sed/patch
  - Well, I would say that sed lines used in your spec file
    are very difficult to read. If no better usage of sed
    usage is found, please create patches again.

    ( Again I frequently use sed, however even for me patches
      seems preferable for this case )

* cflags
(In reply to comment #31)
> No idea what to do here. If i sed the export CFLAGS from the Makefile and use
> §RPM_OPT_FLAGS it fails miserably to build. If i add the optflags and dont sed
> nothing changes.
  - Actually it is incorrect. You should add %optflags to CFLAGS, not
    replace CFLAGS completely. For this Makefile, try below:
---------------------------------------------------
export CFLAGS="%{optflags}"
make prefix=%{_prefix} libdir=%{_lib}
---------------------------------------------------

* %exclude -> rm
> >   - I prefer to remove unneeded files at %install instead of using
> >     %exclude unless unavoided.  
> 
> I was able to rm -f one of the three. Two %exlude are still in since i honestly
> dont know exactly where they come from. I'm not a coder. They just come up as:
  - This is normal when installing perl modules (i.e. these files are created
    automatically), and you can just remove these files at the end of %install
    ( like "rm -f %{buildroot}%{_libdir}/libstfl.a" )

* Documents
  - Empty %doc is not needed.
Comment 33 Thomas Janssen 2009-10-06 03:31:26 EDT
(In reply to comment #32)
> For -4:
> 
> * About sed/patch
>   - Well, I would say that sed lines used in your spec file
>     are very difficult to read. If no better usage of sed
>     usage is found, please create patches again.

I changed and documented the sed lines. If you think they're still not good enough, then please educate me.

> * cflags
> (In reply to comment #31)
> > No idea what to do here. If i sed the export CFLAGS from the Makefile and use
> > §RPM_OPT_FLAGS it fails miserably to build. If i add the optflags and dont sed
> > nothing changes.
>   - Actually it is incorrect. You should add %optflags to CFLAGS, not
>     replace CFLAGS completely. For this Makefile, try below:
> ---------------------------------------------------
> export CFLAGS="%{optflags}"
> make prefix=%{_prefix} libdir=%{_lib}
> ---------------------------------------------------

/me bangs head on desk.. I fiddled around with it but haven't seen the obviously, thanks.

> * %exclude -> rm
> > >   - I prefer to remove unneeded files at %install instead of using
> > >     %exclude unless unavoided.  
> > 
> > I was able to rm -f one of the three. Two %exlude are still in since i honestly
> > dont know exactly where they come from. I'm not a coder. They just come up as:
>   - This is normal when installing perl modules (i.e. these files are created
>     automatically), and you can just remove these files at the end of %install
>     ( like "rm -f %{buildroot}%{_libdir}/libstfl.a" )

Done.

> * Documents
>   - Empty %doc is not needed.  

Removed.

Spec URL: http://thomasj.fedorapeople.org/reviews/stfl.spec
SRPM URL: http://thomasj.fedorapeople.org/reviews/stfl-0.21-5.fc10.src.rpm

http://koji.fedoraproject.org/koji/taskinfo?taskID=1729999

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 34 Susi Lehtola 2009-10-06 03:44:58 EDT
Use a patch instead of sed, since sed will quietly do nothing if the makefile changes in the future, whereas patch will fail and let you know.

**

I'd say you have to modify the default CFLAGS:
 export CFLAGS += -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC
You were probably failing due to the missing -I. -D_GNU_SOURCE which modifies compilation behaviour when you tried
 export CFLAGS += $(RPM_OPT_FLAGS)
Just replace this occurrence with
 export CFLAGS += -I. -D_GNU_SOURCE -fPIC
You need to do this, since -Os will override the default Fedora optimization flag (-O2).
Comment 35 Mamoru TASAKA 2009-10-06 04:31:41 EDT
(In reply to comment #34)
> Use a patch instead of sed, since sed will quietly do nothing if the makefile
> changes in the future, whereas patch will fail and let you know.

I always think that even if patches created against the older version
"seem" to apply to the newer version "correctly", it does not guarantee
that the modified code is what we expect and  we should check them
_anyway_ so I don't see so much advantage on using patch over using 
sed for this purpose

> I'd say you have to modify the default CFLAGS:
>  export CFLAGS += -I. -Wall -Os -ggdb -D_GNU_SOURCE -fPIC
> You were probably failing due to the missing -I. -D_GNU_SOURCE which modifies
> compilation behaviour when you tried
>  export CFLAGS += $(RPM_OPT_FLAGS)
> Just replace this occurrence with
>  export CFLAGS += -I. -D_GNU_SOURCE -fPIC
> You need to do this, since -Os will override the default Fedora optimization
> flag (-O2).  

Well, I ignored this because for me -Os and -O2 doesn't differ so
much.
Comment 36 Thomas Janssen 2009-10-06 05:32:02 EDT
I keep track on every change i make to the source with sed. Not just in the spec file, i have a local file as well. I post every change i make to upstream and have it included into the next release, if possible.
When upstream gives out a new release i check again what has changed and remove then the needed workarounds.
I think that's enough to make sure there's no unneeded stuff hanging out in a spec.

I hope i fixed the CFLAGS now. I'm sorry that i dont understand them so well. I will work harder on that and try to find a webpage, explaining those flags. Or what flag overrides another flag needed.

Spec URL: http://thomasj.fedorapeople.org/reviews/stfl.spec
SRPM URL: http://thomasj.fedorapeople.org/reviews/stfl-0.21-6.fc10.src.rpm

http://koji.fedoraproject.org/koji/taskinfo?taskID=1730080

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 37 Kevin Kofler 2009-10-07 06:26:08 EDT
It's the maintainer's call whether to use patch or sed. This should not block the review.
Comment 38 Ben Boeckel 2009-10-07 10:02:03 EDT
FWIW, with sed as complex as that, I would suggest using a patch, but I do agree with Kevin here. The patch would be much easier to maintain than sed commands (if a flag gets added, the sed line has to be fixed then tested to make sure something else doesn't get chomped where the patch just needs updated).
Comment 39 Mamoru TASAKA 2009-10-08 14:11:24 EDT
For -6:

* Again about sed
  - Well, still I would like to see more smart usage of sed (if
    you want to use sed instead of patch), like:
--------------------------------------------------------
sed -i.path \
	-e '/mkdir.*lib-dynload/d' \
	-e '/cp/s|lib-dynload||' \
	python/Makefile.*

sed -i.soname \
	-e 's|\(.*ln -fs.*/\)\(libstfl\.so\)$|\1\2\n\1\$(SONAME)|' \
	Makefile

sed -i.ldflags -e 's|\(-shared\)|\1 \$(LDLIBS)|' Makefile

sed -i.path -e 's|libdir=.*|libdir=%{_libdir}|' stfl.pc.in
sed -i.cflags -e 's|-Os||' Makefile
--------------------------------------------------------
    There may be more smart ways.
    ! By the way
--------------------------------------------------------
sed -i 's,libdir=${exec_prefix}/lib,libdir=${exec_prefix}/${libdir},' stfl.pc.in
--------------------------------------------------------
      is wrong. "libdir=${exec_prefix}/${libdir}" is invalid because
      ${libdir} cannot be defined with this line 
      ( try "$ pkg-config --libs stfl " and what returns )

* Redundant macros
  - Empty %doc still remains (in -perl subpackage)
Comment 40 Mamoru TASAKA 2009-10-19 13:50:30 EDT
ping?
Comment 41 Thomas Janssen 2009-10-19 14:17:25 EDT
I could change the sed stuff. I removed the empty %doc in -perl subpackage.
But since ${libdir} is wrong and i have no idea what should it be then, i have to wait for upstream to enlighten me. I sent a query but no answer yet.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 42 Mamoru TASAKA 2009-10-20 11:31:28 EDT
(In reply to comment #41)
> But since ${libdir} is wrong and i have no idea what should it be then, 

As I said on the comment 39, stfl.pc.in should contain:
------------------------------------------
libdir=/usr/lib
------------------------------------------
on ix86, ppc and
------------------------------------------
libdir=/usr/lib64
------------------------------------------
on x86_64, ppc64. So (In reply to comment #39)
> sed -i.path -e 's|libdir=.*|libdir=%{_libdir}|' stfl.pc.in
is enough.
Comment 43 Thomas Janssen 2009-10-20 12:35:39 EDT
I was totally blind. Sorry.

Removed empty %doc
Changed sed commands. Wow by the way.

[thomas@tusdell SPECS]$ rpmlint stfl.spec ../SRPMS/stfl-0.21-7.fc10.src.rpm ../RPMS/x86_64/stfl-*-7*
stfl-devel.x86_64: W: no-documentation
stfl-perl.x86_64: W: no-documentation
stfl-python.x86_64: W: no-documentation
stfl-ruby.x86_64: W: no-documentation
7 packages and 1 specfiles checked; 0 errors, 4 warnings.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1757694

Spec URL: http://thomasj.fedorapeople.org/reviews/stfl.spec
SRPM URL: http://thomasj.fedorapeople.org/reviews/stfl-0.21-7.fc10.src.rpm

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 44 Mamoru TASAKA 2009-10-20 13:26:07 EDT
-------------------------------------------------------
    This package (stfl) is APPROVED by mtasaka
-------------------------------------------------------
Comment 45 Thomas Janssen 2009-10-20 14:29:26 EDT
Thank you for the review Mamoru Tasaka and everybody else. I learned a lot with it.

New Package CVS Request
=======================
Package Name: stfl
Short Description: A library which implements a curses-based widget set for text terminals
Owners: thomasj
Branches: F-10 F-11 F-12
InitialCC:

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 46 Kevin Fenzi 2009-10-22 00:33:45 EDT
cvs done.
Comment 47 Mamoru TASAKA 2009-10-23 10:15:18 EDT
Closing.

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