Bug 592672
Summary: | Review Request: hct - A HDL complexity tool | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Shakthi Kannan <shakthimaan> |
Component: | Package Review | Assignee: | Petr Pisar <ppisar> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, notting, perl-devel, ppisar, shakthimaan |
Target Milestone: | --- | Flags: | ppisar:
fedora-review+
huzaifas: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | hct-0.7.60-2.el5 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2010-07-07 17:37:50 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
Shakthi Kannan
2010-05-16 05:53:37 UTC
$ rpmlint hct-0.7.60-1.fc12.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. $ rpmlint ../SPECS/hct.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. $ rpmlint ../RPMS/noarch/hct-0.7.60-1.fc12.noarch.rpm hct.noarch: E: useless-provides perl(HCT::Std::IO) 1 packages and 0 specfiles checked; 1 errors, 0 warnings. Successful Koji builds for F-12, F-13, F-14 and EL-5: http://koji.fedoraproject.org/koji/taskinfo?taskID=2190724 http://koji.fedoraproject.org/koji/taskinfo?taskID=2190731 http://koji.fedoraproject.org/koji/taskinfo?taskID=2190737 http://koji.fedoraproject.org/koji/taskinfo?taskID=2190733 $ rpmlint -i hct.spec ../RPMS/noarch/hct-0.7.60-1.fc13.noarch.rpm hct.noarch: E: useless-provides perl(HCT::Std::IO) This package provides 2 times the same capacity. It should only provide it once. hct.noarch: W: no-manual-page-for-binary hct.pl Each executable in standard binary directories should have a man page. 1 packages and 1 specfiles checked; 1 errors, 1 warnings. The error is because of double package HCT definition: $ grep -Hnr 'package HCT::Std::IO' lib/ lib/HCT/Std.pm:45:package HCT::Std::IO; lib/HCT/Std/IO.pm:19:package HCT::Std::IO; lib/HCT/Std/IO.pm:60:package HCT::Std::IO::Handle; This is upstream bug. Not fatal for Fedora. Things that I'd like to see corrected: * The homepage URL should end with slash. * Description: Please expand the IC abbreviation to full words `integrated circuit'. The description should be understandable even for guys who do not want to install the package. * The big clean-up with %{__rm} -rf `find . -name 'config*' is dangerous. (Imagine a file name contained a white space). Use "find -name 'config*' -depth -exec rm -rf -- '{}' \+" or something like that. * Why the hct.pl has '.pl' extension? Is it necessary? Original build system delivers "hct" wrapper (if it worked). What about just "hct" name or symlink to hct.pl? The spec file looks good otherwise. Please, show me updated spec file. Upstream was forked into couple of new designs, and hasn't made much progress. But, this release of the package has over 800 downloads, and is a very useful tool. So, have chosen this release, and as of now we are upstream. <quote> The big clean-up with %{__rm} -rf `find . -name 'config*' is dangerous. (Imagine a file name contained a white space). Use "find -name 'config*' -depth -exec rm -rf -- '{}' \+" or something like that. </quote> Sorry, I didn't understand the difference. In both cases, we still use 'config*' within quotes. Should I change each removal to use "find -name 'config*' -exec rm -rf -- '{}' \+" ? <quote> * Why the hct.pl has '.pl' extension? Is it necessary? Original build system delivers "hct" wrapper (if it worked). What about just "hct" name or symlink to hct.pl? </quote> The original wrapper has hard-coded the path as #!/bin/sh /Users/smaurer/Downloads/0.7.60/hct.pl $@ Since, we are packaging, we only need the hct.pl file. Since, it is a Perl script, it has the .pl extension, and I thought it was ok to have it, and thus haven't changed it. Do you want me to rename the file to simply hct? > Sorry, I didn't understand the difference. Problem is not with 'config*'. Problem is with shell substitution (the back-ticks). Lets have code: rm -rf `command` If command expands to 'filename', resulting command will be: 'rm' '-rf' 'filename' If command expands to 'file name', resulting command will be: 'rm' '-rf' 'file' 'name'. And this is not something we wanted. You can have dangerous file names like '/tmp/ /home' that ticks your script to remove '/tmp' and '/home' directories instead of 'home' directory under ' ' directory under '/tmp'. > Should I change each removal to use "find -name 'config*' -exec rm -rf -- '{}' \+" ? You can use all the logical ORs, but you should pass the file names to 'rm' arguments using find utility that does not break strings on white spaces. Original code: %{__rm} -rf `find . \( -name 'config*' -o \ -name 'windows' -o \ -name 'Misc' -o \ -name 'Pod' -o \ -name '.svn' -o \ -name '*.svn' \)` New code: find . -depth \( -name 'config*' -o \ -name 'windows' -o \ -name 'Misc' -o \ -name 'Pod' -o \ -name '.svn' -o \ -name '*.svn' \) \ -exec %{__rm} -rf -- '{}' + I think the plus symbol does not need to be escaped. Notice: I did not try the code. Just for completeness: * The '-depth' argument forces find to order file names from leaf to root of directory tree. This is good not to get warning about removal of files from already removed directories. * The '--' argument of 'rm' delimits 'rm' options and file name arguments. This is good not to confuse 'rm' if a file name starts with a hyphen character. * The '{}' string is substituted by 'find' with found file names. * The '+' character marks end of '-exec' statement of find. In addition, it means to pass to one 'rm' command as much file names as possible. It avoids executing rm for each file name. (If you wanted to run 'rm' for each file name separately, use ';'. Do not forget to escape it because semicolon is special shell token.) > Do you want me to rename the file to simply hct? Exactly. If there are no other tools that expect 'hct.pl', the extension in UN*X word will be useless and make users to type more. (E.g. 'yum' is a python script and does not have '.py' extensions.) Actually we need to provide extensionless name because upstream (even dead state) intends so and users used for HCT from other distribution will expect it in Fedora too. In other words, we should not divert from upstream. @Petr: Thanks for the wonderful explanation and prompt reply. I have updated the .spec with the following changes: 1. Appended slash to URL. 2. Expanded 'integrated circuit' in the description. 3. Changed use of "rm -rf" in %prep to use "find . -depth" 4. Renamed hct.pl in Makefile.PL to use hct. SPEC: http://shakthimaan.fedorapeople.org/SPECS/hct.spec SRPM: http://shakthimaan.fedorapeople.org/SRPMS/hct-0.7.60-2.fc13.src.rpm Package tested fine! Builds fine in Koji for F12, F13, F14, EL-5 and EL-6. http://koji.fedoraproject.org/koji/taskinfo?taskID=2252866 http://koji.fedoraproject.org/koji/taskinfo?taskID=2252868 http://koji.fedoraproject.org/koji/taskinfo?taskID=2252869 http://koji.fedoraproject.org/koji/taskinfo?taskID=2252874 http://koji.fedoraproject.org/koji/taskinfo?taskID=2252878 Looks good, approved. New Package CVS Request ======================= Package Name: hct Short Description: A Hardware Description Language complexity tool Owners: shakthimaan chitlesh Branches: F-12 F-13 EL-5 EL-6 cvs done hct-0.7.60-2.fc12 has been submitted as an update for Fedora 12. http://admin.fedoraproject.org/updates/hct-0.7.60-2.fc12 hct-0.7.60-2.fc13 has been submitted as an update for Fedora 13. http://admin.fedoraproject.org/updates/hct-0.7.60-2.fc13 hct-0.7.60-2.el5 has been submitted as an update for Fedora EPEL 5. http://admin.fedoraproject.org/updates/hct-0.7.60-2.el5 hct-0.7.60-2.el5 has been pushed to the Fedora EPEL 5 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update hct'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/hct-0.7.60-2.el5 hct-0.7.60-2.fc12 has been pushed to the Fedora 12 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update hct'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/hct-0.7.60-2.fc12 hct-0.7.60-2.fc13 has been pushed to the Fedora 13 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update hct'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/hct-0.7.60-2.fc13 hct-0.7.60-2.fc13 has been pushed to the Fedora 13 stable repository. If problems still persist, please make note of it in this bug report. hct-0.7.60-2.fc12 has been pushed to the Fedora 12 stable repository. If problems still persist, please make note of it in this bug report. hct-0.7.60-2.el5 has been pushed to the Fedora EPEL 5 stable repository. If problems still persist, please make note of it in this bug report. |