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

Re: New package: icmpdn



On 06/28/2005 04:22 PM, Fredrik Tolf wrote:
On Tue, 2005-06-28 at 15:34 +0200, Oliver Falk wrote:
On 06/28/2005 02:46 PM, Fredrik Tolf wrote:

<http://www.dolda2000.com/~fredrik/icmp-dn/icmpdn.spec>
<http://www.dolda2000.com/~fredrik/icmp-dn/icmpdn-0.3-1.src.rpm>

The first look at it, the following things I see:

* Remove Vendor Tag

I'm sure you're right, but what is there supposed to be instead? Surely, the author is supposed to mentioned so that people know who to blame? ;-)

Vendor and Packer is added automatically... If a user want's to blame someone for bugs, he will look at the URL tag and go the the webpage of the software and will find the correct person. But in most cases, the packager is blamed for bugs and the bug is entered into bugzilla (or at least this *would* be the correct way). And the packager is the one who tries to fix it or check with the original author.


* I'm not sure if the license is OK this way...
As far as I know, NSS modules must be LGPL, since they are dynlinked
into glibc, which is LGPL. As for the binaries, I don't see any reason
why they shouldn't be GPL:ed. Since idnlookup is only called by command
from the NSS module (it isn't linked or anything), their respective
licenses shouldn't matter.


Then again though, IANAL.

There might be someone else who can have a look at the package to check the licensing stuff... I'm not very familiar with all those things... :-)


* Source is better written as Source0 and is missing the full URI to the tarball

Again, I'm sure you're right, but what's the difference between Source and Source0?

Some buildsystems (eg. mach) have the problem, that they do not understand Source: correctly, but do with Source0. I'm not sure about mock - which is used actually for the FE buildsys. So to be sure, it's better written this way.


* Strange characters show up in %description: – (after icmpdnd, idnlookup, libnss_icmp.so.2) in firefox; In vim: â~ ~S
Hm. It seems that Oron Peled copied my homepage directly into the README
and specfile when he created them, so those are probably en-dashes in
UTF-8. I never noticed, since all my systems handle UTF-8 correctly.
>
I might as well fix it (dashes and en-dashes look the same in terminal
fonts anyway), but is there anything wrong with using UTF-8 in
specfiles?

I guess it's OK to use UTF8, but you can never be sure if everyone has a working utf8 environment. For example I have it disabled, since I encountered problems with my older machines (rh 6 - 8)... However...


* In %build section, you should use %configure - it should set everything correctly

As I mentioned, I don't know much about building RPMs, so you may well be right, but does this "%configure" set the libdir correctly to /lib? AFAIK, that's pretty important for NSS modules.

Is it a problem if they are under /usr/lib? I'm not sure...

[ ... ]

* In %install section do only use rm -rf "$RPM_BUILD_ROOT" (same for the %clean section)

Again, I'm sure you're right, but it seems weird to me not to check so that the root dir doesn't get removed. Will fix, though. You are referring to just removing the checks, right?

Yes. Just remove the checks. I had a discussing about this just a few hours ago with Michael Schwendt <bugs michael gmx net>. I also recommended it to allways check it, but Michael is correct, all FC packages should add a BuildRoot... I attached the mail from Michael for you...


* %post and %postun should run ldconfig
  And therefor PreReq: /sbin/ldconfig

Would fix, but I have no idea what "PreReq" refers to. Also, are you sure that running ldconfig really is necessary? After all, this library is only dlopen()'ed by the nameswitch, not used by ld.so. For sure, this RPM has worked without ldconfig previously.

If it worked fine yet, the do not add it and remove the %post and %postun sections.


* Missing %changelog section

Forgive me my ignorance, but what is that supposed to be? Should I just insert the contents of the ChangeLog file?

No, the %changelog section is for the rpm changelog... Have a look at other rpms (do a rpm -q --changelog glibc for example).


[ ... ]

* In %files section use %{_libdir} instead of /lib/, since those libs will be installed in %{_libdir} (/usr/lib), if you use %configure and this is the correct place for 'em.

Are you really sure that /usr/lib is the correct place for NSS modules? On all my systems (both RH, FC and Gentoo), all /usr/lib/libnss* are just symlinks to /lib, and even post-installed NSS modules, like nss_ldap, are installed in /lib. Thus, it seems weird if this would be the only NSS module to be installed in /usr/lib.

Seems I need to check that.

[ ... ]

Thanks for your comments!

No problem, u r welcome. I'll have a look at the problematic with the lib location and write you again. But maybe someone else has the correct answer for this!?


Best,
 Oliver
--- Begin Message ---
On Tue, 28 Jun 2005 10:43:52 +0200, Oliver Falk wrote:

> * The rm after %install is more secure, if you write:
> 
>    [ "$RPM_BUILD_ROOT" != "/" ] && rm -rf $RPM_BUILD_ROOT
> 
>    Same for the %clean section, of course.

No, please don't recommend that. Besides decreasing readability, if some
user really tried to override the buildroot which is specified at the
top of the spec file and define it as '/', it would disable several of
rpmbuild's scripts. Additionally, there are other paths which somebody,
who doesn't know what he's doing, could specify with which to mess up a
system: /opt, /usr, //, /root, ~, to name a few. 

-- 
Michael Schwendt <mschwendt users sf net>
Fedora Core release 4 (Stentz) - Linux 2.6.11-1.1369_FC4
loadavg: 1.07 1.71 1.33

--
fedora-extras-list mailing list
fedora-extras-list redhat com
https://www.redhat.com/mailman/listinfo/fedora-extras-list

--- End Message ---

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