[Bug 1107441] Review Request: udt - UDP based Data Transfer Protocol

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

 



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



--- Comment #4 from Mattias Ellert <mattias.ellert@xxxxxxxxxxxx> ---
(In reply to Florian "der-flo" Lehner from comment #3)
> (In reply to Mattias Ellert from comment #2)
> > > [!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
> > >      beginning of %install.
> > >      Note: rm -rf %{buildroot} present but not required
> > >    ---> Please remove 'rm -rf %{buildroot}'
> > 
> > Sorry, I should have mentioned that I am planning to build this for EPEL 5.
> > (I know I didn't do this properly - I need to add some additional stuff that
> > is no longer part of the emacs generated template to get it to properly
> > build on EPEL 5.)
> 
> Please clarify this by a comment above the line

I have added a comment to the specfile.

> > 
> > > [!]: Permissions on files are set properly.
> > >    ---> Please replace 'install -p -m 644' with 'install -pm 0644'
> > 
> > Why is "-pm" better than "-p -m"? Personally I think that stacking all
> > option together instead of listing them separately obscures what you are
> > doing and makes the call harder to understand. This is especially true in
> > case one of the options takes an argument as in this case.
> 
> I see your point and I agree with you.
> It's the leading 0 of 0644 that I wanted to point to. Using 0644 instead of
> 644 is just a good style.

I don't believe there is consensus about this. I for one have the opposite
opinion and find the version with the 0 in front slightly more confusing. Or at
least used to before I knew the meaning of the forth digit. I usually avoid
using the fourth digit unless really necessary (and it rarely is since you
rarely need to set the setuid/setgid bits on files).

I agree that this is a matter of style, but I do not believe it is a matter of
bad vs. good style and that one style must be used and the other must not.

> > > [!]: SourceX is a working URL.
> > >    ---> please append '#/%{name}-%{version}.tar.gz' to Source0, to get
> > >         a properly named source-tarball
> > 
> > Is this a serious complaint? The name of the upstream tarball is important
> > for identifying the upstream sources. Arbitrarily renaming it looses the
> > pedigree of the file, and gives rise to questions about why is the tarball
> > named differently from upstream's name in the source RPM? Has it been
> > modified in some way? I disagree with this suggestion.
> 
> Appending '#/%{name}-%{version}.tar.gz' to Source0, will let you get a
> properly named source-tarball After this change the use of `spectool -g [-R]
> udt.spec` will give you a properly named src-tarball automatically.
> ---> https://fedoraproject.org/wiki/Packaging:SourceURL#Troublesome_URLs

The guideline you are referring to is an exception that only should be applied
when specific condition are met, The condition stated in the guideline for when
it should be applied is not met in this case.

The condition for when to apply this guideline as stated in the guideline
itself is "When upstream has URLs for the download that do not end with the
tarball name". In this case the URL does end with the name of the tarball. So
this exception is not applicable.

Applying the exceptional guideline when the condition for applying it is not
met would be a guideline violation. I would consider it to be a severe
guideline violation and if I would review a package that did that I would block
it until it was removed. Since you are asking me to do something that would
make me block my own package review I will not do that.

-- 
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://admin.fedoraproject.org/mailman/listinfo/package-review





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