[Bug 1333531] Review Request: opa-ff - OPA Fast Fabric Tools

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

 



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



--- Comment #5 from Michal Schmidt <mschmidt@xxxxxxxxxx> ---
(In reply to Erik E. Kahn from comment #4)
I see you made some changes, but most of the issues I mentioned in comment #3
are still present.

Looking at the updated opa-ff.spec:

> Name: opa

Still inconsistent with spec file name.
Or is the spec file to be called "opa.spec" when imported to Fedora?

> Summary: Intel Omni-Path basic tools and libraries for fabric managment.

Still with the typo and the dot.

> # tarball created by:
> # git clone https://github.com/01org/opa-ff.git
> # cd opa-ff
> # tar czf opa-ff.tar.gz --exclude-vcs .
> Source: opa.tgz

These steps to recreate the tarball are not deterministic. If I repeat them a
year from now, I'd get a tarball with different contents. The steps should
identify the exact git commit.

> %description
> This package contains the tools necessary to manage an Intel(R) Omni-Path 
> Architecture fabric.

Please do not use the "(R)" symbol in %description. See
http://fedoraproject.org/wiki/Packaging:Guidelines#Trademarks_in_Summary_or_Description

> BuildRequires: ibacm-devel, %{?BuildRequires}

What is the purpose of the %{?BuildRequires} macro?

> Requires: opa-basic-tools

Explicit Requires between subpackages of the same source package must be fully
versioned and arch-specific.

> %setup -q -c

Still using -c. So... is there no chance of improving the tarball creation
steps to have a top-level directory?

> if [ -d OpenIb_Host ]

Again, why?
As a packager, you should know whether the tarball contains this directory or
not. No such conditional should be necessary.

The %install part still contains a long script. Oh well, it's unfortunate,
but not a blocking issue.

Still uses generated file lists. I strongly dislike them.
It's not idiomatic and makes it hard to review what the packages contain.

Still uses /etc/sysconfig (not a blocker, but a bad choice, IMO).

Still installs files under /opt. BLOCKER.

> %changelog
> * Thu Jun 2 2016 Scott Breyer <scott.j.breyer@xxxxxxxxx> - 10.1.0-ifs
> - Update to latest from build 10.1.0.0.145 (FF 10.1.0.0.126)

The changelog entry again does not match the actual package version.
And you deleted the previous changelog entry.

-- 
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
https://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx




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