Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=712624 --- Comment #2 from Ankur Sinha <sanjay.ankur@xxxxxxxxx> 2011-06-30 14:10:47 EDT --- (In reply to comment #1) > If you don't need a sponsor then I can review your package. Here's a quick and > dirty review of the spec file. I'll add my comments inline with OK stuff > snipped out: Thanks Richard. I don't need a sponsor. I'm a packager already :) > > # Note: devel package does not contain any headers. Do I need to add them > # .so files are shared libraries, I need to call /sbin/ldconfig, but where? In > a > # post section for the devel package? > # Schemas handling also needs to be looked at. > > I'm not sure if you need a devel package. Is there any reason a program would > want to build against this one? If so, then it probably should contain the > headers, if not, then probably no. I doubt it. I'll get rid of the devel package. > > I don't think you need to call ldconfig since you're putting the libraries in a > named directory and not in the default (i.e. /usr/lib{,64}). Okay. > > I'm not qualified to review the schema portion so we'll need some help on that > part. > > > # applied all the patches from the debian package > # svn export svn://svn.debian.org/svn/debian-med/trunk/packages/aeskulap/trunk/ > aeskulap-debian > Patch0: %{name}-circular-svg.patch > Patch2: %{name}-DcmElement.patch > Patch3: %{name}-desktop.patch > Patch4: %{name}-findAndCopyElement.patch > Patch5: %{name}-gcc.patch > Patch6: %{name}-i18n.patch > # This is used to update the configure.in before running autoreconf fo update > the autotoolization. > > fo -> to > > > # Not used in spec file. It's listed here so it's there with the sources > Patch7: %{name}-configure.patch > Patch8: %{name}-oflog.patch > Patch9: %{name}-patientNames.patch > # applied patch7, ran autreconf -if, and then took a diff to generate this > patch > # The "-if" flag is to update the libtool macros. > Patch10: %{name}-autotools.patch > > I wonder if this is the best way to handle this. I've run into a problem like > this with a package I maintain on RPM Fusion. I ended up calling autoconf from > %build, i.e.: > """ > %build > # Necessary due to patched configure.in > /bin/bash ./autogen.sh > """ > > My way is pretty ugly too. Not sure which way is better. I went through this: http://fedoraproject.org/wiki/PackagingDrafts/AutoConf It says it isn't suggested to run autoconf during the build. What I've done is listed as one of the solutions :) > > > BuildRequires: dcmtk-devel > BuildRequires: gettext intltool libpng-devel libjpeg-turbo-devel > BuildRequires: libtiff-devel gtkmm24-devel libglademm24-devel > BuildRequires: gconfmm26-devel libtool > BuildRequires: openssl-devel > BuildRequires: gettext-devel > BuildRequires: tcp_wrappers-devel > BuildRequires: desktop-file-utils > BuildRequires: GConf2 > Requires(pre): GConf2 > Requires(post): GConf2 > Requires(preun): GConf2 > > I couldn't find anything definitive on this so hopefully someone more > knowledgeable will chime in. Since you are already requiring "gettext-devel", > I'm not sure you need "gettext" Hrm, I just copied that from the find lang section in the guidelines. No harm leaving it there, just for info. . > > Also, since GConf2 is a BR: I don't think the BR(pre,post,preun)'s are needed > or even do anything. Again, copied from the guidelines. Letting it be. http://fedoraproject.org/wiki/Packaging:ScriptletSnippets > > > rm -rvf dcmtk > > This line should have a comment above it. I assume you're removing a bundled > library? Yes. Added a comment. > > > touch ./NEWS configure fails in the absence of NEWS. :/ > > If there's no NEWS file upstream I don't think adding an empty one is > necessary, in fact I believe rpmlint will complain. > > # remove .la files. Is this sufficient? > find $RPM_BUILD_ROOT -name "*.la" -exec rm -fv '{}' \; > > Yup, that will do it. http://ankursinha.fedorapeople.org/aeskulap/aeskulap.spec http://ankursinha.fedorapeople.org/aeskulap/aeskulap-0.2.2-0.2beta1.fc15.src.rpm Thanks! Ankur -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review