[Bug 1026337] Review Requiest:=?UTF-8?Q?=20nfs=2Dganesha=20=E2=80=94=20a=20user=2Dmode=20file=20server=20for=20NFS=20?=(v3, 4.0, 4.1 pNFS)

[bugzilla at redhat.com]

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



--- Comment #3 from Zbigniew Jędrzejewski-Szmek <zbyszek at in.waw.pl> ---
(In reply to Kaleb KEITHLEY from comment #2)
> > 1. Remove %defattr(-,root,root)
> > 
> > 2. Remove rm -rf %{buildroot}
> > 
> > 3. Remove %clean
> > 
> > 4. Why are libntirpc sources necessary?
> 
> If it is not already obvious, nfs-ganesha doesn't build without them.
> 
> > Shouldn't this be a separate package?
> 
> No, it shouldn't. libntirpc not ready to be a stand-alone package. When it's
> ready it will be packaged separately and removed from the nfs-ganesha build.
OK. It would be nice to have this as a comment in the spec, so people don't
wonder if this is a bundled library.

> > 5. It would be nicer to use direct github url as Source0: https://github.com
> > /%{name}/%{name}/archive/pre-2.0-RC2.tar.gz. This way it's easier to update, > verify sources, etc.
> 
> It doesn't exist. (Using it now would result in an additional rpmlint
> warning.)
It exits, I actually tested the link before posting.

> Eventually it will be there and when it is then it'll be used. I don't
> consider this as a show stopper for the review.
Sure, just nice to have.

> > 
> > 6. %description could become Summary, and please extend the description a 
> > bit, saying a bit more what the project is useful for etc.
> 
> Say more about what an NFS server is useful for?
Well, I think that would be useful: not every user immediately knows what NFS
stands for, some think that it stands for Need For Speed. What I had in mind,
was why *this* NFS server, what does it do better than the other ones.

> > 7. [ nothing here ]
> > 
> > 8. There's no need to say %{__tar}, %{__rm}, %{__make}, %{__chmod}. Just use > plain tar, rm... Such indirection only makes sense for things that are likely 
> > to be substituted at some point, like %{__python2}.
> 
> https://fedoraproject.org/wiki/Packaging:ReviewGuidelines?rd=Packaging/
> ReviewGuidelines says "MUST: ... use macros".  I used macros.
Actually is says "must use macros *consistently*", which is not the same. If
you follow the link it says "use macros instead of hard-coded *directory*
names", "rm should be used in preference to %{__rm}".

> > 9. Please add Provides: bundled(gnulib).
> 
> It doesn't provide gnulib, bundled or otherwise. I don't know what this
> refers to.
Yeah, my error.

> > 10. Please split out big docs into a separate package (size ~ 2MB).
> > 
> > 11. Please change cmake to %cmake.
> 
> This RC doesn't build with %cmake. I will notify the upstream developers and
> maybe they can fix this for the next RC. (I am only kickstarting the
> packaging, as a favor.)
> 
> > 
> > 12. Please change %{__make} to make VERBOSE=1 %{?_smp_mflags}.
> > 
> > 13. libzfswrap cannot be bundled (https://fedoraproject.org
> > /wiki/Packaging:No_Bundled_Libraries). Please remove it in %prep.
> 
> There is no libzfswrap bundled. There is nothing in %prep about libzfswrap.
When I unpack the sources, I have

contrib/libzfswrap

This directory should be removed in %prep, according to
https://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries.
Possibly other steps must be taken later on, but this is the first one.

> > 
> > Oh, and I think you have a very old fedora-review, which is provided outdated > suggestions.
> 
> Do you mean the template? It's the one from 
> https://fedoraproject.org/wiki/PackagingDrafts/ReviewTemplate. It is mostly
> consistent with
> https://fedoraproject.org/wiki/Packaging:ReviewGuidelines?rd=Packaging/
> ReviewGuidelines.
Hm, indeed. So this template is in contradiction to the guidelines, e.g.
because %clean is now discouraged, while it is a MUST in the template. 

> Updated files at
> 
> Spec URL: http://kkeithle.fedorapeople.org/update-1/nfs-ganesha.spec
> SRPM URL:
> http://kkeithle.fedorapeople.org/update-1/nfs-ganesha-2.0.0-0.rc2.fc19.src.
> rpm

There's now -rc3.

-- 
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