Bug 502614
| Summary: | Review Request: stfl - STFL implements a curses-based widget set for text terminals | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Thomas Janssen <th.p.janssen> |
| Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
| Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | byron, fedora, fedora-package-review, jan.klepek, mtasaka, notting, susi.lehtola, thomasj |
| Target Milestone: | --- | Flags: | mtasaka:
fedora-review+
kevin: fedora-cvs+ |
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2009-10-23 14:15:18 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Thomas Janssen
2009-05-26 13:35:18 UTC
Scratch koji-build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1380908 -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers 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 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 I was really only packaging it for newsbeuter, but I would be happy to maintain the package. Ok. It's all yours. Thanks for your contribution :) -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers 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. 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. 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. (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 Note that currently I sponsor Byron. 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 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? 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? You must install the package and then run "rpmlint stfl" to see the final set of rpmlint checks. What is the status of this bug? 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 (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. I don't see any unanswered questions or concerns, what further information is needed? (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). Those warnings are present becuase users of libstfl.so must also link against libncursesw.so. Is this something that needs to be fixed? Are the any reason why libstfl.so _itself_ is not linked against libncursesw.so? ping? Sorry, I'm swamped. Thomas: do you want to take this one back? 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 Newsbeuter is blocked till this is reviewed Heh, yeah, i know that. My bad.. Byron, should i take Newsbeuter as well? -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers Thomas, newsbeuter is all yours also. 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
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 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.
(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 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.
(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 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). (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. 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 It's the maintainer's call whether to use patch or sed. This should not block the review. 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). 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)
ping? 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
(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. 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 -------------------------------------------------------
This package (stfl) is APPROVED by mtasaka
-------------------------------------------------------
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 cvs done. Closing. |