Spec URL: http://brooks.nu/~lane/verilator.spec SRPM URL: http://brooks.nu/~lane/verilator-3.680-2.fc10.src.rpm Description: Verilator is the fastest free Verilog HDL simulator. It compiles synthesizable Verilog (not test-bench code!), plus some PSL, SystemVerilog and Synthesis assertions into C++ or SystemC code. It is designed for large projects where fast simulation performance is of primary concern, and is especially well suited to create executable models of CPUs for embedded software design teams. Because of systemc licensing issues, the verilator dependancies on systemc are not included with this package. This means the perl-verilog, perl-systemc, and systemc functionality that is integrated into verilator will only work if those packages are installed separately. This package, therefore, supports the verilog features of verilator (including vcd generation via the --trace option). This is my first Fedora package. Chitlesh Goorah will sponsor this package as it will become part of FEL.
I will do the review and sponsoring :)
#001: Release: 2%{?dist} This requires that first - first, for the version 3.680, this is the first src.rpm, thus your Release should be 1 and not 2. . Release: 1%{?dist} - second, every changelog entry should entail the version-release tag Here is how your changelog should be: * Thu Oct 16 2008 Lane Brooks <lane [AT] brooks DOT nu> - 3.680-1 - Initial package based on SUSE packages from Guenter Dannoritzer <dannoritzer{%}web{*}de> For more info : https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs You have noticed that I have set your email address to: "lane [AT] brooks DOT nu". This is to limit unwanted spam to your email address. #002: Licensing: With Fedora's strict packaging policies, all spec files should entail the exact license and its version. In your case it will be: License: GPLv2 For more info: https://fedoraproject.org/wiki/Licensing #003: Replace tabs with spaces Line 6 in your spec file : Group: Applications/Engineering #004: This line should be removed, as your name is already listed in the changelogs Packager: Lane Brooks <lane> #005: Referencing the SourceX: All the SourceX: tag should have their complete urls: For more info: https://fedoraproject.org/wiki/Packaging/SourceURL #006: Remove Autoreqprov: On #007: Build requires Exceptions In accordance to https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions, you can safely remove the following buildrequires: BuildRequires: gcc, gcc-c++ #008: Add a "-q" to the following: %setup -n %{name}-%{version} #009: Use macros as much as you can in your spec file: /usr/share/ --> %{_datadir} ./configure --> %configure /usr --> %{_prefix} cp -> %{__cp} rm -> %{__rm} for more info: https://fedoraproject.org/wiki/Packaging/RPMMacros #010: Keep timestamps use -p with %{__cp} such as %{__cp} -pr Please take some time reading the fedora packaging guidelines again and update the actual spec file. Everytime you update and publish a new spec file, don't forget to increment the Release tag and update the changelog. Once you have updated the above issues, I will dig in-depth about verilator's compilation.
Actually, generally speaking, there is no rule saying that the first release submitted in Fedora must be "1". 2 is just fine. One might have done/used some previous testing specs before the one submitted. Or it might be a new version of a rpm already in use from another source. I am looking forward to see this package in Fedora.
True, but since it's his first package for fedora, I prefer that he adopts some good practices :)
I have incorporated Chitlesh's feedbackc into an updated spec file and have a new release "3" available for download from: http://www.brooks.nu/~lane/verilator.spec http://www.brooks.nu/~lane/verilator-3.680-3.fc10.src.rpm The previous release was "2" but was not documented in the changelog. I added the correct changelog entry to document releases "1", "2", and now "3". We have been using these releases at my work, thus I do not want to reset to "1" or it will cause problems for our users. I have built and tested this new release 3 on our project regression test suite at work on F10 rawhide, F8, and Centos 5.2. Lane
I thought that it was considered good practice to bump the release for changes made during review, so unless you submit a perfect package in the first place it was quite _common_ for the first package to have a release other than -1?
#001 You can replace %setup -q -n %{name}-%{version} by %setup -q #002 add %{?_smp_mflags} SYSTEMPERL=%{_datadir}/verilator/perl-systemc %{__make} %{?_smp_mflags} #003: use fedora optflags: add the following after configure %{__sed} -i "s|CPPFLAGSNOWALL +=|CPPFLAGSNOWALL +=%{optflags}|" \ {src,test_c,test_regress,test_sc,test_sp,test_verilated}/Makefile_obj #004: Are these useful ? /usr/share/verilator/bin /usr/share/verilator/bin/verilator_includer #005: move BUIDROOT/usr/share/verilator/examples to examples/ then %doc examples/ #006: Are these important: chitlesh(SPECS)[1]$rpm -ql verilator | grep -v examples | grep .cpp /usr/share/verilator/include/verilated.cpp /usr/share/verilator/perl-systemc/src/SpTraceVcdC.cpp #007 can you give me quick case study of how to use verilator ? It might be helpful for those who don't know, but want to do the review.
Ping ?
#001: Summary: Verilator is a fast simulator for synthesizable Verilog Summary should not include the %{name} Instead it should be "A fast synthesizable Verilog simulator" #002: For rpmfusion, I'm packaging systemc, however the "libsystemc-devel" is simply systemc. #003: Verilator 3.681 has been released . Lane are you still interested with verilator ?
I am swamped right now with a tape out. I will try to get some time over the next two weeks to incorporate your comments.
> #004: Are these useful ? > /usr/share/verilator/bin > /usr/share/verilator/bin/verilator_includer Yes. As far as I know, verilator uses these. > #005: move BUIDROOT/usr/share/verilator/examples to examples/ > then %doc examples/ I don't understand this comment. > #006: Are these important: > chitlesh(SPECS)[1]$rpm -ql verilator | grep -v examples | grep .cpp > /usr/share/verilator/include/verilated.cpp > /usr/share/verilator/perl-systemc/src/SpTraceVcdC.cpp Yes. These are included in all verilator simulation builds.
Created attachment 328036 [details] simple verilator test case Attached is a simple verilator test case per Chitlesh's request. Here is how to run this: 1. untar the file and cd into the verilator_test_case directory. 2. Compile the simulation by running 'make'. This generates an executable called obj_dir/Vcounter. 3. Run the simulation: 'obj_dir/Vcounter' This will output the following to stdout: Hello World from counter! Time 0: count = 0x0 Time 2: count = 0x0 Time 4: count = 0x0 Time 6: count = 0x1 Time 8: count = 0x2 Time 10: count = 0x3 Time 12: count = 0x4 Time 14: count = 0x5 Time 16: count = 0x6 Time 18: count = 0x7 Time 20: count = 0x8 Time 22: count = 0x9 Time 24: count = 0xa SUMMARY OF TEST CASE: This test case implements a counter in the verilog file counter.v. The C++ file tb.cpp implements the testbench that provides the input, including the clock. This example does not turn on any tracing. See the verilator documentation on how to enable tracing to generate a VCD file for viewing with gtkwaves.
I have an updated spec and src file you can download from: Spec URL: http://brooks.nu/~lane/verilator.spec SRPM URL: http://brooks.nu/~lane/verilator-3.681-3.fc10.src.rpm I incorporated Chitlesh's feedback from Comments #7 and #9 except for #004 from Comment #7 as I do not understand this comment.
Chitlesh, I would like to get verilator into Fedora 11 if it is not already too late. Can you give me a summary of the milestones and dates that are required to accomplish that? This will help me as I am still extremely swamped with a tapeout. With the milestone list I can make the necessary time to get this done. Lane
This is a URL correction of Comment #13 I have an updated spec and src file you can download from: Spec URL: http://brooks.nu/~lane/verilator.spec SRPM URL: http://brooks.nu/~lane/verilator-3.681-1.fc10.src.rpm I incorporated Chitlesh's feedback from Comment #7 and Comment #9 except for item #004 from Comment #7 as I do not understand this suggestion.
The suggestion was to package the example files (i.e. the content of /usr/share/verilator/examples), which needed two steps - move the folder /usr/share/verilator/examples directly below $BUILDROOT - use the "%doc" directive to include the above mentioned folder
Lane, at https://fedoraproject.org/wiki/Releases/11/Schedule is the schedule for F11.
I updated the spec file to move the examples from the data directory to the doc directory. You can download the updates from: Spec URL: http://brooks.nu/~lane/verilator.spec SRPM URL: http://brooks.nu/~lane/verilator-3.681-2.fc10.src.rpm
Lane, you have commented perl-verilog on #BuildRequires: perl-verilog, perl-systemc, systemc I'm packaging perl-Verilog https://bugzilla.redhat.com/show_bug.cgi?id=476386 I haven't yet looked at the details, do you think enabling perl-Verilog our verilator will provide more "features" ?
#001: These should not be shipped /usr/share/verilator/include/verilated.mk.in --> duplicate with /usr/share/verilator/include/verilated.mk #002: do we need to ship these with this package ? usr/share/verilator/perl-systemc/src /usr/share/verilator/perl-systemc/src/SpCommon.h /usr/share/verilator/perl-systemc/src/SpTraceVcdC.cpp /usr/share/verilator/perl-systemc/src/SpTraceVcdC.h /usr/share/verilator/perl-systemc/src/systemperl.h Wouldn't it be wise to package perl-SystemPerl ? I have already started packaging perl-SystemPerl. Soon I'll post a package review for perl-SystemPerl
(In reply to comment #20) > #002: > > do we need to ship these with this package ? > usr/share/verilator/perl-systemc/src > /usr/share/verilator/perl-systemc/src/SpCommon.h > /usr/share/verilator/perl-systemc/src/SpTraceVcdC.cpp > /usr/share/verilator/perl-systemc/src/SpTraceVcdC.h > /usr/share/verilator/perl-systemc/src/systemperl.h > > Wouldn't it be wise to package perl-SystemPerl ? I have already started > packaging perl-SystemPerl. Soon I'll post a package review for perl-SystemPerl Are you packaging the system perl from Wilson Snyder at www.veripool.org? These files are included because they are get compiled into the verilator models whenever you want to do tracing to see your waveforms. If you are packaging the one from Wilson Snyder, then I can remove these files.
(In reply to comment #21) > If you are > packaging the one from Wilson Snyder, then I can remove these files. Yes, I'll package the ones from Wilson Snyder. I'm also trying to package everything from veripool for F-11.
(In reply to comment #11) > > #004: Are these useful ? > > /usr/share/verilator/bin > > /usr/share/verilator/bin/verilator_includer > > Yes. As far as I know, verilator uses these. > /usr/share/verilator/bin /usr/share/verilator/bin/verilator_includer is a duplicate of /usr/bin/verilator_includer You will need to remove the duplicates as well %{__rm} -rf %{buildroot}%{_datadir}%{name}/bin
Use perl-SystemPerl as from now on: Bug 478759 - Review Request: perl-SystemPerl - SystemPerl Perl module
New files available: Spec URL: http://brooks.nu/~lane/verilator.spec SRPM URL: http://brooks.nu/~lane/verilator-3.700-1.fc10.src.rpm Changes include: - Removal of duplicate file per Comment #23. verilator depends on the file /usr/share/verilator/bin/verilator_includer, so I removed the duplicate in /usr/bin. - Added dependency of perl-SystemPerl-devel for both building and installing. For verilator to work correctly with perl-SystemPerl-devel, however, you must make the change to the perl-SystemPerl-devel spec file as documented on my comment on Bug 478759 and put the src files in /usr/include/SystemPerl/src - Updated to newly released verilator 3.700
in file (of verilator) src/V3Options.cpp: && !V3Options::fileStatNormal(var+"/src/systemperl.h")) { if you remove /src, I believe it should pull systemperl.h from perl-SystemPerl-devel. Can you check if please ? in the 3.700 release notes, you are listed for: - Add limited support for tristate inouts. Written by Lane Brooks. This allows common pad ring and tristate-mux structures to be Verilated. See the documentation for more information on supported constructs. - Fix 'bad select range' warning missing some cases, bug43. [Lane Brooks] good job Lane.
(In reply to comment #26) > > in file (of verilator) src/V3Options.cpp: > > && !V3Options::fileStatNormal(var+"/src/systemperl.h")) { > > if you remove /src, I believe it should pull systemperl.h from > perl-SystemPerl-devel. Can you check if please ? This is not a sufficient solution. There are a additional files that verilator pulls from system perl when you turn on tracing that occur during the simulation build. I am not in favor of the approach you are proposing here for the following reasons: - Neither of us have enough background with the complete scope of verilator or system-perl to know how far such a change would reach. I can see this causing unforeseen bugs that reach into tools even beyond verilator. In addition to trying to find all places that verilator references the src/ directory, people likely have tools beyond verilator that use the systemperl files in the src/ directory (we have one, for example). - This puts added burden of maintenance on us as the packagers. Everytime a new release of verilator and system-perl come out we will have to verify our patch(s) are still valid and verify that any additional functionality added in the new releases is not broken. I prefer to stay as close to upstream as possible as the upstream project is much broader than my limited use of it. I use the tool for verilog simulation, but the tool is much broader than that. I feel inadequate putting my stamp that a change such a seemingly benign change is not problematic.
Chitlesh notified me of this thread. The src/ prefix is because many users have multiple versions of SystemPerl installed (generally in a repository) and just point to the one they want. Lane has a the right solution for the present version, as makefiles etc also have the src/ path hardcoded. If there's a strong objection to the extra src/ in the path, I can add a new envvariable that will set the location.
FYI Verilator 3.702 allows you to set and compile SYSTEMPERL_INCLUDE into verilator so the src/ suffix hack (see the earlier comments here) isn't needed.
ping Lane ?
I updated verilator to the latest 3.710 and also switched to using the SYSTEMPERL_INCLUDE env variable to be compatible with the systemperl installation location. Spec URL: http://brooks.nu/~lane/verilator.spec SRPM URL: http://brooks.nu/~lane/verilator-3.710-1.fc10.src.rpm
The build is failing under F-11. You can use "mock" to verify it. http://fedoraproject.org/wiki/Projects/Mock g++ -O2 -g -march=i386 -mtune=i686 -I/usr/include -O2 -g -march=i386 -mtune=i686 -MMD -O2 -g -march=i386 -mtune=i686 -I. -I.. -I../../include -O2 -g -march=i386 -mtune=i686 -DYYDEBUG -O2 -g -march=i386 -mtune=i686 -O -DDEFENV_SYSTEMC=\"\" -DDEFENV_SYSTEMC_ARCH=\"\" -DDEFENV_SYSTEMPERL=\"\" -DDEFENV_SYSTEMPERL_INCLUDE=\"/usr/include/perl-SystemPerl\" -DDEFENV_VERILATOR_ROOT=\"/usr/share/verilator\" -c ../V3Param.cpp g++ -O2 -g -march=i386 -mtune=i686 -I/usr/include -O2 -g -march=i386 -mtune=i686 -MMD -O2 -g -march=i386 -mtune=i686 -I. -I.. -I../../include -O2 -g -march=i386 -mtune=i686 -DYYDEBUG -O2 -g -march=i386 -mtune=i686 -ggdb -DVL_DEBUG -c ../V3PreProc.cpp ../V3PreProc.cpp: In member function ‘virtual std::string V3PreProcImp::getline()’: ../V3PreProc.cpp:995: error: invalid conversion from ‘const char*’ to ‘char*’ make[2]: *** [V3PreProc.o] Error 1 make[2]: Leaving directory `/home/chitlesh/rpmbuild/BUILD/verilator-3.710/src/obj_dbg' make[1]: *** [../verilator_bin_dbg] Error 2 make[1]: *** Waiting for unfinished jobs.... g++ -O2 -g -march=i386 -mtune=i686 -I/usr/include -O2 -g -march=i386 -mtune=i686 -MMD -O2 -g -march=i386 -mtune=i686 -I. -I.. -I../../include -O2 -g -march=i386 -mtune=i686 -DYYDEBUG -O2 -g -march=i386 -mtune=i686 -O -DDEFENV_SYSTEMC=\"\" -DDEFENV_SYSTEMC_ARCH=\"\" -DDEFENV_SYSTEMPERL=\"\" -DDEFENV_SYSTEMPERL_INCLUDE=\"/usr/include/perl-SystemPerl\" -DDEFENV_VERILATOR_ROOT=\"/usr/share/verilator\" -c ../V3PreShell.cpp
Grr, every GCC version has slightly different things it complains about... This will patch it, let me know if you want a new release instead. If there's another bug, please try "make -k" so they'll all show up rather than one at a time. Thanks diff --git a/src/V3PreProc.cpp b/src/V3PreProc.cpp index 84856dc..4f76ba8 100644 --- a/src/V3PreProc.cpp +++ b/src/V3PreProc.cpp @@ -990,7 +990,7 @@ int V3PreProcImp::getToken() { string V3PreProcImp::getline() { // Get a single line from the parse stream. Buffer unreturned text until the newline. if (isEof()) return ""; - char* rtnp; + const char* rtnp; bool gotEof = false; while (NULL==(rtnp=strchr(m_lineChars.c_str(),'\n')) && !gotEof) { int tok = getToken();
Lane, the package is ready. please update the package with respect to the following minor details. Then I'll approve. #1 add the Artistic file as %doc #2 compiler fix from Wilson: sed -i "s|char\* rtnp;|const char\* rtnp;|" src/V3PreProc.cpp #3: rpmlint warning spurious-executable-perm chmod 0644 %{buildroot}%{_mandir}/man1/%{name}.1.gz
New files available that address the three issues in #34: Spec URL: http://brooks.nu/~lane/verilator.spec SRPM URL: http://brooks.nu/~lane/verilator-3.711-1.fc11.src.rpm
- MUST: The package is named according to the Package Naming Guidelines. - MUST: The spec file name matches the base package %{name} - MUST: The package meets the Packaging Guidelines. - MUST: The package is licensed (GPLv2) with an open-source compatible license and meet other legal requirements as defined in the legal section of Packaging Guidelines. - MUST: The License field in the package spec file matches the actual license. - MUST: the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. - MUST: The spec file must be written in American English. - MUST: The spec file for the package is be legible. - MUST: The sources used to build the package must matches the upstream source, as provided in the spec URL. - MUST: The package successfully compiles and builds into binary rpms on at least i386. - MUST: All build dependencies is listed in BuildRequires. - MUST: The spec file handles locales properly. - MUST: If the package does not contain shared library files located in the dynamic linker's default paths - MUST: the package is not designed to be relocatable - MUST: the package owns all directories that it creates. - MUST: the package does not contain any duplicate files in the %files listing. - MUST: Permissions on files are set properly. - MUST: The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). - MUST: The package consistently uses macros, as described in the macros section of Packaging Guidelines. - MUST: The package contains code, or permissable content. This is described in detail in the code vs. content section of Packaging Guidelines. - MUST: There are no Large documentation files - MUST: %doc does not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. - MUST: There are no Header files or static libraries - MUST: The package does not contain library files with a suffix - MUST: Package does NOT contain any .la libtool archives - MUST: Package containing GUI applications includes a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. - MUST: Package does not own files or directories already owned by other packages. SHOULD Items: - SHOULD: The source package does include license text(s) as LICENSE - SHOULD: mock builds succcessfully in i386. - SHOULD: The reviewer tested that the package functions as described. A package should not segfault instead of running, for example. - SHOULD: No scriptlets were used, those scriptlets must be sane. - SHOULD: No subpackages present. APPROVED
Who are you pinging? What are the next steps? Is the procedure documented somewhere?
Since the package is approved, it is up to the submitter of the ticket to take the next step, that being a CVS request so the package can be imported, built and pushed out. The procedure is fully documented in http://fedoraproject.org/wiki/PackageMaintainers/Join Every new packager should read over that before submitting packages.
hehe to you Lane. No the process isn't over yet. You have to put verilator on fedora cvs, then build it on koji(fedora's build system) then finally push to mirrors. Currently you are here at "Add_Package_to_CVS_and_Set_Owner" https://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_CVS_and_Set_Owner Can you please add me as one of the owners please ?
There is a new upstream release
The "fedora_cvs" flag is not editable for me. How do I make it editable? Perhaps I am not part of the "fedorabugs" group? Or perhaps I am misunderstanding the instructions.
Can you give me your Fedora FAS username please ? I will sponsor you. https://admin.fedoraproject.org/accounts
username: dirjud I just applied to the packager group
You are now sponsored. It might be that you will have to wait one or two hours before the services grant you your packager rights.
New Package CVS Request ======================= Package Name: verilator Short Description: A fast simulator of synthesizable Verilog HDL Owners: dirjud chitlesh Branches: F-10 F-11 EL-5 InitialCC:
CVS done.
Chitlesh, When I build the EL-5 release of verilator using koji it fails with this message: DEBUG util.py:280: Executing command: /usr/bin/yum --installroot /var/lib/mock/dist-5E-epel-build-527553-80064/root/ resolvedep 'perl-SystemPerl-devel' 'flex' 'bison' 'perl' DEBUG util.py:256: No Package Found for perl-SystemPerl-devel Is SystemPerl not available for EL-5?
Give me a few days. I'm in holidays. I'll push it to EL-5 when I return.
verilator-3.712-1.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/verilator-3.712-1.fc10
verilator-3.712-1.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/verilator-3.712-1.fc11
verilator-3.712-1.fc11 has been pushed to the Fedora 11 stable repository. If problems still persist, please make note of it in this bug report.
verilator-3.712-1.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report.