Bug 579925
Summary: | Review Request: tcl-tclreadline - GNU Readline extension for Tcl/Tk | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Robert Scheck <redhat-bugzilla> | ||||
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> | ||||
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | low | ||||||
Version: | rawhide | CC: | crosa, e, fedora-package-review, herrold, j, redhat-bugzilla, redhat | ||||
Target Milestone: | --- | Keywords: | Reopened | ||||
Target Release: | --- | Flags: | herrold:
fedora-review+
gwync: fedora-cvs+ |
||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | tcl-tclreadline-2.1.0-3.el6 | Doc Type: | Bug Fix | ||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2014-08-16 00:32:02 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: | |||||||
Attachments: |
|
Description
Robert Scheck
2010-04-06 23:15:24 UTC
I don't know much about tcl and we don't get many tcl packages submitted, but we do have a guidelines page. I suppose that anyone who was interested in tcl would have reviewed this already, so I'll go ahead and take care of it. The package does not meet the naming guidelines, which require that tcl packages have a "tcl-" prefix. You can have Provides: tclreadline if you want. 2.1.0 still seems to be the latest version. In fact, upstream seems to be thoroughly dead; the last commit was something like nine years ago. Perhaps you and the Debian folks could get together and fork the package. You've already changed the API of the package (with the prompt2 stuff) so it's not much of a stretch. Otherwise, who is going to ensure compatibility with future tcl development? Can you comment on the 20+ rpmlint complaints of the form tclreadline.x86_64: W: undefined-non-weak-symbol /usr/lib64/libtclreadline-2.1.0.so Tcl_DoOneEvent Regarding the naming guidelines: I was too much focussed on how the packaging itself is done, sorry. I've no idea regarding the rpmlint complaints so far, I will have to investigate here, but: I won't fork tclreadline, because I'm using it to rarely and I'm not really familiar with tcl programming. The patches applied are mostly from Debian, so I would like to keep everything how it is, because there are some further tclreadline users out there. If that makes the review from your point of view a no-go, please let me know. Spec URL: http://labs.linuxnetz.de/bugzilla/tcl-tclreadline.spec SRPM URL: http://labs.linuxnetz.de/bugzilla/tcl-tclreadline-2.1.0-2.src.rpm I guess the primary question is why you would want to push unmaintained software into the distribution when by your own admission you aren't sufficiently familiar with the software to maintain it if problems arise. For example, what happens if Fedora updates to a readline version which is no longer ABI compatible? As to the package itself, I think it looks fine assuming you intend to push this to EL4 or EL5; otherwise you can drop several bits. I'm not sure why you apply memuse.patch; did our tcl become threaded at some point? (I guess it doesn't really hurt anything, but generally you patch to fix something that's actually broken.) prompt2.patch seems too actually change the API of the library. Are you really sure you didn't intend to fork this software? Debian policies may permit this kind of thing but it's really a bad idea. There are Fedora users out there that expect tclreadline. And no, not every Fedora users wants to get a contributor. That may has to do with new target audience of Fedora... In case Fedora updates to a API incompatible version (I think, you meant API rather ABI), I'll try to solve things, otherwise the package gets orphaned. The package is intended to be pushed to EPEL branches. The tcl in Fedora was from time to time threaded by accident, more or less each time the maintainer on Red Hat side switched and the new one wanted to give a try to the threaded tcl. I'm not going to create a new upstream, even it's "in" at Fedora nowadys to fork any software. From my point of view, the prompt2.patch changes API, but it only adds something new & optional. Thus backward compatibility is given. Shouldn't reviews that are stuck after a certain amount of time be closed? I'm not aware of formal policies, but if the original submitter (Robert Scheck) agrees this should be put out of the way. IMHO this is better than introducing a package and then little time later deprecate it. Robert, is this package still relevant after so much time without any upstream activity? Answering myself and justifying the action here: https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews?rd=Extras/Policy/StalledReviews So I'm closing this BZ. Reopen if this is still a itch you want to scratch. I am still interested in introducing this package into Fedora EPEL (and maybe also into Fedora), reopening. MUST rpmlint - fails please address malformed conditional in spec ; devel package has no documentation ; debuginfo has non-binaries in /usr/lib rpmlint /var/lib/mock/epel-7-x86_64/result/tcl-tclreadline-2.1.0-2.el7.centos.src.rpm tcl-tclreadline.src: W: spelling-error Summary(en_US) Readline -> Deadline, Headline, Breadline tcl-tclreadline.src: W: spelling-error Summary(en_US) Tk -> Tl, T, K tcl-tclreadline.src: W: spelling-error %description -l en_US Readline -> Deadline, Headline, Breadline tcl-tclreadline.src: W: spelling-error %description -l en_US tk -> kt, t, k tcl-tclreadline.src: W: malformed-fedora-conditional tcl-tclreadline.spec:22 1 packages and 0 specfiles checked; 0 errors, 5 warnings. rpmlint /var/lib/mock/epel-7-x86_64/result/tcl-tclreadline-2.1.0-2.el7.centos.x86_64.rpm tcl-tclreadline.x86_64: W: spelling-error Summary(en_US) Readline -> Deadline, Headline, Breadline tcl-tclreadline.x86_64: W: spelling-error Summary(en_US) Tk -> Tl, T, K tcl-tclreadline.x86_64: W: spelling-error %description -l en_US Readline -> Deadline, Headline, Breadline tcl-tclreadline.x86_64: W: spelling-error %description -l en_US tk -> kt, t, k tcl-tclreadline.x86_64: W: incoherent-version-in-changelog 2.1.0-2 ['2.1.0-2.el7.centos', '2.1.0-2.centos'] 1 packages and 0 specfiles checked; 0 errors, 5 warnings. rpmlint /var/lib/mock/epel-7-x86_64/result/tcl-tclreadline-devel-2.1.0-2.el7.centos.x86_64.rpm tcl-tclreadline-devel.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 0 errors, 1 warnings. rpmlint /var/lib/mock/epel-7-x86_64/result/tcl-tclreadline-debuginfo-2.1.0-2.el7.centos.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. MUST Package Naming Guidelines - the tcl- requirement has been addressed and met. I find no other issue, but this is my first informal review. MUST Specfile Naming - PASS MUST Packaging Guidelines - FAIL - rpmlint issues ; debuginfo package issue No External Kernel Modules - OK No Inclusion of Pre-Built Binaries or Libraries - OK Spec Legibility - OK Architecture Support Builds for Fedora 21 - koji task 7204827 - PASS Builds for Fedora 20 - koji task 7204082 - PASS Builds for Fedora 19 - koji task 7204835 - PASS Builds for EPEL-7 - koji task 7204882 - PASS Builds for EPEL-6 - kpjo task 7204885 - PASS Builds for EPEL-5 - kpjo task 7204923 - PASS Filesystem Heirarchy Standard Compliance - OK rpmlint on installed packages <mock-chroot>[root@mythbox /]# rpmlint -i tcl-tclreadline tcl-tclreadline.x86_64: W: incoherent-version-in-changelog 2.1.0-2 ['2.1.0-2.el7.centos', '2.1.0-2.centos'] The latest entry in %changelog contains a version identifier that is not coherent with the epoch:version-release tuple of the package. 1 packages and 0 specfiles checked; 0 errors, 1 warnings. <mock-chroot>[root@mythbox /]# rpmlint -i tcl-tclreadline-debuginfo tcl-tclreadline-debuginfo.x86_64: W: only-non-binary-in-usr-lib There are only non binary files in /usr/lib so they should be in /usr/share. 1 packages and 0 specfiles checked; 0 errors, 1 warnings. <mock-chroot>[root@mythbox /]# rpmlint -i tcl-tclreadline-devel tcl-tclreadline-devel.x86_64: W: no-documentation The package contains no documentation (README, doc, etc). You have to include documentation files. 1 packages and 0 specfiles checked; 0 errors, 1 warnings. Changelog readability - OK Tag usage check - OK Packager Tag - not present GOOD Vendor Tag - not present GOOD Copyright Tag - not present GOOD License Tag used - GOOD Summary Tag does not end in period - GOOD PreReq Tag not used - GOOD Requires Tag used - GOOD Source Tag - used and resovles to full URL - GOOD BuildRequires - OK mock used and no missing build requires found Trademarks in summary or description - GOOD Documentation - No obvious irregulararities found DebugInfo - This Package is made by rpmbuild and yet not addressed in the .spec. rpmlint is unhappy with it composure so th spec needs to address this. Devel Package - Other than the missing documentation for this package the spec looks good. Library usage and linking - Can find no issues as the spec pays good attention to this Macros - are used as intended as far as reviewer can tell MUST Licensing Guidelines - PASS license is found in the COPYING file and meets esablished BSD Format. MUST license and spec file license must agree - PASS MUST docs must include license file - PASS MUST spec file uses American English - PASS MUST spec file must be legible - PASS MUST sources must match upstream source - PASS SOURCE0: sha256sum matches SOURCE1: sha256sum matches MUST compile and build on at least one architecture MUST work on architecture or exclude in spec - PASS MUST BuildRequires used -PASS MUST handle locales - UNKNOWN no %find_lang macro is present,but reviewer is unable to test this package for multiple locales so it may be okay. MUST call ldconfig in %post and %postun - PASS MUST not package system libraries - PASS MUST specify if relocatable - PASS MUST package must own all directories it creates - PASS MUST no duplicate files in %files - PASS MUST set file permissions correctly - PASS MUST consistently use macros - PASS MUST codes or permissable content - PASS MUST documentation absence does not affect usage - UNKNOWN - reviewer was not given a test case protocol and is not familiar enough to create one MUST handle static libraries in a -static package - DOES not appear to apply MUST handle devleopment linraries in a -devel package - PASS NOT a GUI Package MUST filenames in UTF-8 format - PASS SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. - PASS SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. - UNKNOWN No translations provided but not sure if this applies SHOULD: The reviewer should test that the package builds in mock - PASS See above was tested in mock and koji scratch build SHOULD: The package should compile and build into binary rpms on all supported architectures - PASS See koji results above SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example. - UNKNOWN The description of how it should operate is unclear to this reviewer SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. - PASS Scripts appear sanely used SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. - PASS No subpackages other than devel seen SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. - PASS No .pc files used SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. - PASS This condition appears met. SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense. - PASS manpage does define the usage of libraries I am not yet approved as a packager yet but have provided the preliminary review above to assist any packager who would want to undetake a full review. The packager may also want to address the rpmlint and locale / language issues discovered. Just read another review and should also point out that COPYING probably belongs in %license not %doc. Hi, Robert Lightfoot I get a different set of objections from my rpmlint evaluation ... obviously some of the 'spelling error' stuff are inapplicable [herrold@centos-6 tclreadline]$ rpmlint /home/herrold/rpmbuild/SRPMS/tcl-tclreadline-2.1.0-2.orc6.src.rpm /home/herrold/rpmbuild/RPMS/x86_64/tcl-tclreadline-2.1.0-2.orc6.x86_64.rpm /home/herrold/rpmbuild/RPMS/x86_64/tcl-tclreadline-devel-2.1.0-2.orc6.x86_64.rpm tcl-tclreadline.src: W: spelling-error %description -l en_US tk -> kt, t, k tcl-tclreadline.src:57: W: macro-in-comment %doc tcl-tclreadline.src:72: W: macro-in-comment %{_libdir} ... addressed in a patch I shall apply in a moment tcl-tclreadline.x86_64: W: spelling-error %description -l en_US tk -> kt, t, k tcl-tclreadline-devel.x86_64: W: no-documentation .... placeholder 'documentation' in a sub-package makes no sense, as it is tied to its parent package when properly installed: Requires: %{name} = %{version}-%{release} -- PASS 3 packages and 0 specfiles checked; 0 errors, 5 warnings. [herrold@centos-6 tclreadline]$ rpmlint -V rpmlint version 1.5 Copyright (C) 1999-2007 Frederic Lepied, Mandriva [herrold@centos-6 tclreadline]$ I am not seeing locate issues in my run ... what version of rpmlint are you running? Robert Scheck Could you please apply the patch if you find it acceptable, and note a URL where the revised package set might be pulled from, and we can move this along directly With the patched spec file locally I now get: [herrold@centos-6 tclreadline]$ rpmlint /home/herrold/rpmbuild/SRPMS/tcl-tclreadline-2.1.0-3.orc6.src.rpm /home/herrold/rpmbuild/RPMS/x86_64/tcl-tclreadline-2.1.0-3.orc6.x86_64.rpm tcl-tclreadline.src: W: spelling-error %description -l en_US tk -> kt, t, k tcl-tclreadline.x86_64: W: spelling-error %description -l en_US tk -> kt, t, k 2 packages and 0 specfiles checked; 0 errors, 2 warnings. Created attachment 923994 [details]
patch at proposed rel 3 to address remainint rpmlint issues
I was running the current version of rpmlint for centos6. That system no longer exists having been migrated to Centos7. Spec URL: http://labs.linuxnetz.de/bugzilla/tcl-tclreadline.spec SRPM URL: http://labs.linuxnetz.de/bugzilla/tcl-tclreadline-2.1.0-3.src.rpm looks good [herrold@centos-6 tclreadline]$ rpmlint /home/herrold/rpmbuild/SRPMS/tcl-tclreadline-2.1.0-3.orc6.src.rpm /home/herrold/rpmbuild/RPMS/x86_64/tcl-tclreadline-2.1.0-3.orc6.x86_64.rpm tcl-tclreadline.src: W: spelling-error %description -l en_US tk -> kt, t, k tcl-tclreadline.x86_64: W: spelling-error %description -l en_US tk -> kt, t, k 2 packages and 0 specfiles checked; 0 errors, 2 warnings. [herrold@centos-6 tclreadline]$ PASS flip on the satisfactory review flag Thank you very much for the review! New Package SCM Request ======================= Package Name: tcl-tclreadline Short Description: GNU Readline extension for Tcl/Tk Upstream URL: http://tclreadline.sourceforge.net/ Owners: robert Branches: f19 f20 f21 el5 el6 epel7 InitialCC: RP Herrold - I still get an error on the src.rpm from rpmlint. [mock@mythbox ~]$ rpmlint /var/lib/mock/epel-7-x86_64/result/tcl-tclreadline-2.1.0-3.el7.centos.src.rpm /var/lib/mock/epel-7-x86_64/result/tcl-tclreadline-2.1.0-3.el7.centos.x86_64.rpm tcl-tclreadline.src: E: specfile-error sh: tclsh: command not found tcl-tclreadline.x86_64: W: incoherent-version-in-changelog 2.1.0-3 ['2.1.0-3.el7.centos', '2.1.0-3.centos'] 2 packages and 0 specfiles checked; 1 errors, 1 warnings. [mock@mythbox ~]$ rpm -qi rpmlint Name : rpmlint Version : 1.5 Release : 4.el7 Architecture: noarch Install Date: Mon 04 Aug 2014 08:22:55 PM EDT Group : Development/Tools Size : 1225006 License : GPLv2 Signature : RSA/SHA256, Fri 04 Jul 2014 12:50:56 AM EDT, Key ID 24c6a8a7f4a80eb5 Source RPM : rpmlint-1.5-4.el7.src.rpm Build Date : Mon 09 Jun 2014 11:28:19 PM EDT Build Host : worker1.bsys.centos.org Relocations : (not relocatable) Packager : CentOS BuildSystem <http://bugs.centos.org> Vendor : CentOS URL : http://sourceforge.net/projects/rpmlint/ Summary : Tool for checking common errors in RPM packages Description : rpmlint is a tool for checking common errors in RPM packages. Binary and source packages as well as spec files can be checked. [mock@mythbox ~]$ (In reply to Robert Lightfoot from comment #19) > tcl-tclreadline.src: E: specfile-error sh: tclsh: command not found This is caused by tclsh(1) being not available on your system. Usually this is also case on builders until the build environment has been set up. Caused by this line: %{!?tcl_version: %global tcl_version %((echo '8.5'; echo 'puts $tcl_version' | tclsh) | tail -1)} If it makes you more happy, we can silent it by using instead: %{!?tcl_version: %global tcl_version %((echo '8.5'; echo 'puts $tcl_version' | tclsh 2> /dev/null) | tail -1)} However this is just hiding the error message from STDERR. I'll defer to others wiser than I as you response just confuses me. If tclsh is needed on the build system then why is there no BuildRequires in the spec file. This was done on a fresh Centos7 system with mock. Command tclsh is provided by package tcl which is a build requires. The "build" in mock usually consists out of two steps (see "ENTER do") in the build.log; it is usually there twice. The first one is to build the *.src.rpm and the second one is for the binary package. The build requires are satisfied at the second one while at the first there is only a minimal environment. In that minimal environment there's usually no tclsh, thus the "echo '8.5'" from above macro wins. If the binary package gets build the build requires of the source rpm have been definately satisfied and building happens against exactly that specified version that is returned by tclsh. Git done (by process-git-requests). tcl-tclreadline-2.1.0-3.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/tcl-tclreadline-2.1.0-3.fc20 tcl-tclreadline-2.1.0-3.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/tcl-tclreadline-2.1.0-3.fc19 tcl-tclreadline-2.1.0-3.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/tcl-tclreadline-2.1.0-3.el6 tcl-tclreadline-2.1.0-3.el5 has been submitted as an update for Fedora EPEL 5. https://admin.fedoraproject.org/updates/tcl-tclreadline-2.1.0-3.el5 tcl-tclreadline-2.1.0-3.el5 has been pushed to the Fedora EPEL 5 testing repository. tcl-tclreadline-2.1.0-3.fc19 has been pushed to the Fedora 19 stable repository. tcl-tclreadline-2.1.0-3.fc20 has been pushed to the Fedora 20 stable repository. tcl-tclreadline-2.1.0-3.el5 has been pushed to the Fedora EPEL 5 stable repository. tcl-tclreadline-2.1.0-3.el6 has been pushed to the Fedora EPEL 6 stable repository. |