Bug 229657

I'll take that for a pre-review (I am not a sponsor so I cannot approve the
package).
For now, I have only seen a small inconsistency in %install ($RPM_BUILD_ROOT
versus ${RPM_BUILD_ROOT} in the rest of the spec) which by no means is anything
else but an esthetic problem. The src package builds happily in mock and rpmlint
has no complaints on any of the 4 (source, binary, devel, debuginfo) rpms

I'll test the program Monday, after I get in touch with my colleagues who can
speak Verilog.

(In reply to comment #1)
> I'll take that for a pre-review (I am not a sponsor so I cannot approve the
> package).
> For now, I have only seen a small inconsistency in %install ($RPM_BUILD_ROOT
> versus ${RPM_BUILD_ROOT} in the rest of the spec) which by no means is 
anything
> else but an esthetic problem. The src package builds happily in mock and 
rpmlint
> has no complaints on any of the 4 (source, binary, devel, debuginfo) rpms

updated
Spec URL: http://openrisc.rdsor.ro/iverilog.spec
SRPM URL: http://openrisc.rdsor.ro/iverilog-20070123-2.src.rpm

> I'll test the program Monday, after I get in touch with my colleagues who 
can speak Verilog.

necessary ?!
I succesuful run tests over an large openrisc core.
Anyway this tool cannot be compared with industry comercial tools.

I bet a beer that you wanted to say
http://openrisc.rdsor.ro/iverilog-20070123-3.src.rpm.

Visually seems fine. And yes, since I have a few dozens hardware engineers
around, I prefer to have one of them test the program. And no, I do not expect
it to replace Questa or VCS yet.

ping.
update ?

(pcnet.ro has verilog employee ?!, [*cbalint remark and wonders how an ISP is 
dealing with verilog ?!, maybe in their free time.])

Yes, sorry for confusions, the new path is:
http://openrisc.rdsor.ro/iverilog-20070123-3.src.rpm

> Visually seems fine. And yes, since I have a few dozens hardware engineers

You review this package visualy, by functionality or by the way how its packed 
to meet fedora-standards ?

> around, I prefer to have one of them test the program. And no, I do not 
>expect it to replace Questa or VCS yet.

What to test ?

    As i mentioned this is not kind of from today till tomorrow software, look 
@ http://www.icarus.com/eda/verilog/, you will notice that it was developed 
over few years, with some obvious efforts by some contributors and in recent 
time even enhanched by some well known companies like bsemi.com or sun.com

    And by the way, if a huge project (~1mil gates) prove the functionality
as debugger and compiler do you think it can be worse ?

    1) http://www.opencores.org/projects.cgi/web/s1_core/overview , yes they 
use test scripts as alterative to VCS and seems run fine. Still works are in 
progress over that project (just look inside the project)

    2) Olso, mention as contributor to openrisc cpu that iverilog is used
inside that project in some cases quite exclusive, and its kinda cpu that was 
taped even in real silicon, and it iverilog do great job in simulation.

    3) We are supposed to review the package not compare verilog compilers or 
whatever this kind of task, you cannot expect to compet with whatever 
comercial compilers, so that is task for upstream contributors not for fedora. 
And by the way debian packed it in their distro since few years.

    4) Yes it has problems. There are buch of problems in parser e.g, but
basic verilog compile ans simulation is OK, if you know to avoid tham you can 
avoid in very large projects. But wich opensource project is perfect ?
     iVerilog meant to be a complementary tool for geda-* tools wich are 
already in -extras ! And geda cannot replace altium.orcad or comercial 
software !

     If want to review functionality and contribute please look forward for 
mainstream contributions, (64 arches still have some minor leaks, parser is 
sucky and so on with the list of TODO), otherwise please consider this as a 
fedora-extra package wich i request for submission.


     I am looking forward to push this to -extras for make it aviable for 
fedora folks. And regarding testcases i am right now looking forward to pack 
in -5 the testcases that comes as extra package on icarus website.

/cristian

pong :)
Look for the review a bit later. 
WRT your questions:
1. I see no reason to bring pcnet into discussion. I am in no way related to them.
2. visually == visual inspection of the new spec.
3. In a review I try to take into account all the aspects you have mentioned. By
accident (or not) I happen to work in a hardware company so I can also do more
tests then just "this program does not segfault at start". Yes, emphasize is on
how it is packed but a non-functional program would not be useful, no matter how
well packaged it is.
4.As of testing, I just want to see it compiling a Verilog program created by
one of my colleagues. Nothing fancy. During the review I have absolutely no
intention to compare its performances with commercial programs with the same
functionality.
5. I happen to know quite well the program, I've watched it (together with gEDA)
with very great interest over the years, exactly because I would like to have
our current tools (which require licenses which go monthly for amounts expressed
with 5 digits) replaced.

Since you intend to extend a bit the package (I suggest creating a separate
package for test cases, BTW) maybe you could include the FAQ -
http://www.icarus.com/eda/verilog/FAQ.html in the main package)

And last but not least, since you need a sponsor, I MAY NOT approve your
package, no matter how well packed it is.

(In reply to comment #5)
> pong :)
> Look for the review a bit later. 
> WRT your questions:
> 1. I see no reason to bring pcnet into discussion. I am in no way related to 
them.

dont take as offence, i just reminded my old times ...
ok, lets dont imply parties. (anyway, in older time i worked at RDS ;-) i 
weared pretty the same 'shoe', but have a bad-remind of those days, you know 
best why)

> 2. visually == visual inspection of the new spec.
> 3. In a review I try to take into account all the aspects you have 
mentioned. By
> accident (or not) I happen to work in a hardware company so I can also do 
more
> tests then just "this program does not segfault at start". Yes, emphasize is 
on
> how it is packed but a non-functional program would not be useful, no matter 
how
> well packaged it is.
> 4.As of testing, I just want to see it compiling a Verilog program created 
by
> one of my colleagues. Nothing fancy. During the review I have absolutely no
> intention to compare its performances with commercial programs with the same
> functionality.

ok. sounds interesting, looking forward for the positiveness of the results.

> 5. I happen to know quite well the program, I've watched it (together with 
gEDA)
> with very great interest over the years, exactly because I would like to 
have
> our current tools (which require licenses which go monthly for amounts 
expressed
> with 5 digits) replaced.
> 
> Since you intend to extend a bit the package (I suggest creating a separate
> package for test cases, BTW) maybe you could include the FAQ -
> http://www.icarus.com/eda/verilog/FAQ.html in the main package)

I thinked at a separate package, i work right now to pack.

> And last but not least, since you need a sponsor, I MAY NOT approve your
> package, no matter how well packed it is.

    Well is see no reason to not approve. :-(. I hope my and your main 
intention (you taked the sponsorship) is to push forward this compiler not to 
sentence it to dead, i see no reason why not. I am sure reviewing it we can 
find an acceptable way reasoning the usefullnes of this package, i allready 
see the usefullness of it.  If geda was approved, but compared with comercial 
packs is realy,really baby, i think iverilog is quite usable for small 
projects, and some experts can adapt even for huge ones. Every things must 
start somewhere, gnu gcc booted with baby shoes and now can outperform many 
comercial compilers, its a question of time and interest of parties.

ok, i wait for your tests, util than i try to came up with the test-suite 
pack.

I am removing the NEEDSPONSOR flag and assigning the bug to myself, because
Patrice has agreed to sponsor you.

GOOD:
- rpmlint check comes back empty both on source and binary rpms:
[wolfy@wolfy iverilog]$ rpmlint iverilog-devel-20070123-3.fc6.x86_64.rpm
[wolfy@wolfy iverilog]$ rpmlint iverilog-20070123-3.fc6.x86_64.rpm
[wolfy@wolfy iverilog]$ rpmlint iverilog-20070123-3.fc6.src.rpm
[wolfy@wolfy iverilog]$
- package meets naming guidelines
- package meets packaging guidelines
- license (GPL) OK, text in %doc, matches source
- spec file legible, in am. English
- source matches latest available upstream version, sha1sum 
6b737279fe876e039322a6c31457372073366ec1  verilog-20070123.tar.gz
- package compiles on devel (x86_64), RPM_OPT_FLAGS are used
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions are sane
- %clean ok
- macro use consistent
- code, not content
- no need for -docs (there are many small text files, but all of them together
occupy <150K)
- nothing in %doc affects runtime
- not a GUI, so no need for .desktop file

- devel package ok (contains 2 libs and some examples)
- no .la files
- no need for any scriptlets
- devel requires base package n-v-r 

So far everything seems fine, tomorrow I'll test the program (did not have time
for that today) and most probably end the review.

I have noticed that you have not included a couple of the doc files:
attributes.txt,extensions.txt,glossary.txt, ivl_target.txt ivlpp.txt,
iverilog-fpga.man, tgt-vvp/README.txt,  vvp/README.txt, and neither the examples
shipped in the vvp directory. Maybe it would be worth to include all those (with
the examples in the devel package, just like the already included set of examples) ?

here we go with the :SHOULD: part from
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines. Conventions for below:
OK = it's OK as it is
MUSTFIX = there is a problem which needs fixing
NA = not available/ does not apply

 If the source package does not include license text(s) as a separate file from
upstream, the packager SHOULD query upstream to include it - OK
 The description and summary sections in the package spec file should contain
translations for supported Non-English languages, if available - NA
 The reviewer should test that the package builds in mock. - OK, builds in mock
for devel/x86_64
- SHOULD: The package should compile and build into binary rpms on all supported
architectures - OK, builds in mock for devel/x86_64 and i386 (no ppc to test on)
- SHOULD: The reviewer should test that the package functions as described. A
package should not segfault instead of running, for example - OK, works as
advertised
- SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague,
and left up to the reviewers judgement to determine sanity. - NA
- SHOULD: Usually, subpackages other than devel should require the base package
using a fully versioned dependency. - OK
- 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. - NA


There are no blockers so the package is APPROVED. Cristi, unless someone spots
something that I have missed (I hope I did not...), you can proceed with CVS
request.

Thanks Christian for pushing electronics applications in Fedora. I maintain the
geda suite. Perhaps we might work together to push more and more electronic apps
inside.

#001
I've seen during the %install /usr/bin/install -c -m . You should preserve
timestamps.

%{__make} prefix=%{buildroot}%{_prefix} \
        bindir=%{buildroot}%{_bindir} \
        libdir=%{buildroot}%{_libdir} \
        libdir64=%{buildroot}%{_libdir} \
        includedir=%{buildroot}%{_includedir} \
        mandir=%{buildroot}%{_mandir}  \
        vpidir=%{buildroot}%{_libdir}/ivl/ \
        INSTALL="install -p" install

#002
The tarball is mal-packaged.
It ships an autom4te.cache/.

I'd suggest to rm -rf autom4te.cache in %prep

You can delete them by
%{__rm} -rf autom4te.cache

for f in cadpli driver driver-vpi examples ivlpp libveriuser solaris tgt-fpga \
         tgt-null tgt-pal tgt-stub tgt-verilog tgt-vvp vpi vpip vvm vvp; do
    pushd $f
        %{__rm} -rf autom4te.cache
    popd
done

#003
You shouldn't ship %{_libdir}/*.a in the -devel package
Delete them
%{__rm} -f %{buildroot}%{_libdir}/{libveriuser,libvpi}.a

#004
# WARNING !!!
# %{?_smp_mflags} broken
Can you explain in one/two line (in the spec file) why it's broken?

#005 I disagree on how %{version} has been filled.

"iverilog -v" shows:
Icarus Verilog version 0.9.devel ($Name: s20070123 $)

You missed the 0.9.XXXXXX

I'll rather opt for :
%define snapshot 20070123

Name:          iverilog
Version:       0.9.0.%{snapshot}

I would also recommend you to add examples on the iverilog package instead of
-devel package. Its size is small and a normal user would not install a -devel
package.

Spec URL: http://openrisc.rdsor.ro/iverilog.spec
SRPM URL: http://openrisc.rdsor.ro/iverilog-0.9.20070123-5.src.rpm

- clean junks from tarball
- exlude static library
- smp build seems fine
- use snapshot instead of cvsver macro
- follow package n-v-r from fedora standard

(In reply to comment #9)
> Thanks Christian for pushing electronics applications in Fedora. I maintain 
the
> geda suite. Perhaps we might work together to push more and more electronic 
apps
> inside.
> 
> #001
> I've seen during the %install /usr/bin/install -c -m . You should preserve
> timestamps.
> 
> %{__make} prefix=%{buildroot}%{_prefix} \
>         bindir=%{buildroot}%{_bindir} \
>         libdir=%{buildroot}%{_libdir} \
>         libdir64=%{buildroot}%{_libdir} \
>         includedir=%{buildroot}%{_includedir} \
>         mandir=%{buildroot}%{_mandir}  \
>         vpidir=%{buildroot}%{_libdir}/ivl/ \
>         INSTALL="install -p" install

updated.

> 
> #002
> The tarball is mal-packaged.
> It ships an autom4te.cache/.
> 
> I'd suggest to rm -rf autom4te.cache in %prep
> 
> You can delete them by
> %{__rm} -rf autom4te.cache
> 
> for f in cadpli driver driver-vpi examples ivlpp libveriuser solaris 
tgt-fpga \
>          tgt-null tgt-pal tgt-stub tgt-verilog tgt-vvp vpi vpip vvm vvp; do
>     pushd $f
>         %{__rm} -rf autom4te.cache
>     popd
> done

i did it. olso '.cvsignore; are cleaned, verifyed UTF-8 stuff and if files
contain tab mixage with spaces.

> 
> #003
> You shouldn't ship %{_libdir}/*.a in the -devel package
> Delete them
> %{__rm} -f %{buildroot}%{_libdir}/{libveriuser,libvpi}.a

i excluded tham from %file devel.

> 
> #004
> # WARNING !!!
> # %{?_smp_mflags} broken
> Can you explain in one/two line (in the spec file) why it's broken?

smp seems to be fine. re-enabled.

> #005 I disagree on how %{version} has been filled.
> 
> "iverilog -v" shows:
> Icarus Verilog version 0.9.devel ($Name: s20070123 $)
> 
> You missed the 0.9.XXXXXX

updated.

right. wasnt sure to do 0.9 or not.

> I'll rather opt for :
> %define snapshot 20070123
> 
> Name:          iverilog
> Version:       0.9.0.%{snapshot}

updated that macro.

(In reply to comment #10)
> I would also recommend you to add examples on the iverilog package instead 
of
> -devel package. Its size is small and a normal user would not install 
a -devel
> package.

olso fixed in -5

Follow http://fedoraproject.org/wiki/CVSAdminProcedure for the CVS Request
procedure.

New Package CVS Request
=======================
Package Name: iverilog
Short Description: Icarus Verilog is a verilog compiler and simulator
Owners: cbalint, cgoorah.au
Branches: FC-5 FC-6 devel
InitialCC:

Balint Cristian, you are not yet sponsored ?

i am sposored in cvsextras group in my fedora account.
(have another package already)

I set flag '?' on fedora cvs, for this bz, is there more things to do for 
creation of CVS ?

I see. For some reason, I received a mail for your request. Forget it.

All you need to do now is wait :)

Hello Balint,
Can you close this bug as next release now ? :)