[Bug 1375765] Review Request: yosys - Yosys Open SYnthesis Suite, including Verilog synthesizer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1375765

Randy Barlow <randy@xxxxxxxxxxxxxxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-review?              |fedora-review+
                   |                            |needinfo?(spacewar@xxxxxxxx
                   |                            |m)



--- Comment #6 from Randy Barlow <randy@xxxxxxxxxxxxxxxxxxxxx> ---
(In reply to Eric Smith from comment #5)
> Spec URL: https://fedorapeople.org/~brouhaha/yosys/yosys.spec
> SRPM URL:
> https://fedorapeople.org/~brouhaha/yosys/yosys-0.6.0-2.20160923git8f5bf6d.
> fc24.src.rpm
> 
> Note that no action was taken for the following:

Thanks for pointing these out!

> * bundled viz.js  - The file viz.js isn't bundled into the generated RPMs,
> so the bundling policy, which is based primarily on security concerns, is
> not applicable. In particular, following the bundling policy would require a
> "Provides: bundled(viz.js)=0.0.3", but that would clearly be wrong, as the
> package doesn't provide it.

I'm convinced and I agree with you now.

> * /usr/share/yosys/python3/smtio.py - Not moved. There is considerable
> precedent for application-specific Python files to be in /usr/share/%{name},
> e.g., firewalld, hplip,  qtcreator, setroubleshoot, virt-manager, etc. I've
> reviewed FHS 3.0 and am not convinced that having application-specific
> Python files in /usr/share/%{name} actually contravenes any FHS 3.0
> requirement.

I still think this *should* be moved, but due to your citations we can keep it
the way it is.

> * tarball without URL - No actual problem. Perhaps was triggered by outdated
> URL for Debian pool.

OK. FYI, the current URL is also 404, but I see the problem with the
disappearing files. I recommend working with upstream to get that man page
included there so we don't depend on Debian's man pages (and then Debian can
also get them from upstream!)

> * %check tests directory - No actual problem. Tests are present and are
> correctly tested in %check section.

OK

> These requested changes have been made:
> 
> * license tag - Changed to include additional licenses.

I read the issue you filed upstream, and it seems that some of the licenses
included in the current spec file only apply to tests and not to the
distributed package. I recommend trimming the licenses down to what is in the
distributed code, but I'm not sure whether the license field should be about
the entire source tree, or just the bits we distribute. I'll leave that
decision up to you.

> * /usr/share/yosys/python3/smtio.p - Had previously marked executable to fix
> rpmlint error. Removed the chmod, so now rpmlint reports that error. Which
> is better, having the rpmlint error, or having the file unnecessarily marked
> as executable? Since it contains no main program, I felt that having it
> marked executable was inconsequential.

It doesn't make sense for the file to be executable so I think the current
state is better. If you want to silence rpmlint, you can drop the
#!/usr/bin/python3 from the top of the source file so rpmlint doesn't think
it's a script. It isn't a script anyway, so that line really isn't necessary.

> * versioned dependency in subpackages:
>  - Subpackage doc dependency on main package *removed*, as docs can stand
> alone.
>  - Subpackage devel dependency on main package made arch-specific.
> * /usr/share/yosys is architecture-independent:
>  - Moved /usr/share/yosys to noarch subpackage.
>  - Main package made dependent on -share subpackage.

The share subpackage is nice! There's a new message from fedora-review with the
current spec that says that there is a large amount of data (3143680 bytes) in
/usr/share, and it suggests moving that data to a noarch subpackage. Since you
now have a -share package, perhaps that would be a nice improvement.

One other message from fedora-review that you might consider fixing - it says
that gawk isn't needed in BuildRequires. You can change that or not at your
option.

There is one thing you do need to fix for sure:

[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/doc/yosys
     randy: You can probably change the doc files section to just have
%{_docdir}/%{name}

Once you have that directory ownership fixed let me know!

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]