Bug 592672

Summary: Review Request: hct - A HDL complexity tool
Product: [Fedora] Fedora Reporter: Shakthi Kannan <shakthimaan>
Component: Package ReviewAssignee: Petr Pisar <ppisar>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: 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
Spec URL: http://shakthimaan.fedorapeople.org/SPECS/hct.spec
SRPM URL: http://shakthimaan.fedorapeople.org/SRPMS/hct-0.7.60-1.fc12.src.rpm
Description: 
The goal of the HCT is to generate scores that represent
the complexity of the constituent modules of large IC design projects
– i.e. SOCs. The design's complexity scores are useful to verification
teams so as to efficiently focus resources based on the dynamic
complexity profile of a design. The scores are a useful tool to guide
HDL designer's re-factoring efforts.

Comment 1 Shakthi Kannan 2010-05-16 06:03:16 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

Comment 2 Petr Pisar 2010-06-15 14:57:38 UTC
$ 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.

Comment 3 Shakthi Kannan 2010-06-15 15:30:26 UTC
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?

Comment 4 Petr Pisar 2010-06-15 16:58:40 UTC
> 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.

Comment 5 Shakthi Kannan 2010-06-16 05:08:14 UTC
@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

Comment 6 Petr Pisar 2010-06-16 08:43:05 UTC
Looks good, approved.

Comment 7 Shakthi Kannan 2010-06-16 08:57:46 UTC
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

Comment 8 Huzaifa S. Sidhpurwala 2010-06-18 09:39:06 UTC
cvs done

Comment 9 Fedora Update System 2010-06-18 10:04:48 UTC
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

Comment 10 Fedora Update System 2010-06-18 10:04:55 UTC
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

Comment 11 Fedora Update System 2010-06-18 10:05:00 UTC
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

Comment 12 Fedora Update System 2010-06-18 16:42:15 UTC
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

Comment 13 Fedora Update System 2010-06-21 13:04:30 UTC
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

Comment 14 Fedora Update System 2010-06-21 13:09:43 UTC
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

Comment 15 Fedora Update System 2010-07-07 17:37:44 UTC
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.

Comment 16 Fedora Update System 2010-07-07 17:50:28 UTC
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.

Comment 17 Fedora Update System 2010-07-21 20:04:10 UTC
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.