https://bugzilla.redhat.com/show_bug.cgi?id=1234791 --- Comment #6 from Jon Kerr Nilsen <j.k.nilsen@xxxxxxxxxxx> --- (In reply to Petr Šabata from comment #5) Thanks for clear and thorough review! > First of all, familiarise yourself with Fedora Packaging guidelines: > https://fedoraproject.org/wiki/Packaging:Guidelines > There's quite a lot to learn, I know. Feel free to ask if you need anything. Thanks, I've read it, but still working on understanding it :) Your review already clarifies quite a bit for me. > > > Okay, so, the review... > > > #1 Since this is only going to el6+, you can drop the BuildRoot tag, > buildroot cleaning in %install and the whole %clean section. These aren't > really needed nowadays. > https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag Noted and dropped. > > > #2 There's no need to explitly declare %defattr anymore. You may drop that > too. > https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions Dropped. > > > #3 There used to be a list of packages guaranteed to be present in every > buildroot. This is no longer the case and every package must declare all > its direct build dependencies explicitly. To ensure your package builds in > the future too, you need to add quite a few to your list of build > requirements (BuildRequires): > - coreutils -- `rm' is called in the spec > - findutils -- `find' is called in the spec > - make -- called in the spec > - perl -- ditto > - perl(base) -- t/25py_sub.t:70 > - perl(Carp) -- Python.pm:3 > - perl(Config) -- Makefile.PL:2, t/11factor.t:3 > - perl(Cwd) -- Makefile.PL:3, Makefile.PL:307 > - perl(DynaLoader) -- Python.pm:5 > - perl(Exporter) -- Python.pm:6 > - perl(File::Path) -- t/00init.t:5 > - perl(Getopt::Long) -- Makefile.PL:5 > - perl(Inline::denter) -- Python.pm:184, Python.pm:215 > - perl(Inline::Filters) -- Python.pm:69 > - perl(overload) -- Python.pm:313, Python.pm:370, Python.pm:378 > - perl(Parse::RecDescent) -- t/03parse.t:3 > - perl(POSIX) -- t/30floats.t:6 > - perl(strict) -- all over the place > - perl(Test) -- most tests > - perl(Test::Simple) -- t/19testref.t:1, t/26undef.t:1 > - perl(utf8) -- t/20unicode.t:4 > - perl(warnings) -- many tests I see I was a bit sloppy there yes. Added all but Inline::Filters now. > > Inline::Filters appears to be optional. > Parse::RecDescent is optional. > (However, it's good practice to run as many tests as possible so unless it's > causing some problems, for example dependency cycles, it's a good idea to > require optional dependencies as well.) Inline::Filters isn't available in EPEL, so since it's optional I suggest not to include it. > > One has to go through the sources to figure these out. There are tools that > can help you with it, e.g. `tangerine' for perl code. Ah, tangerine helped a lot, thanks! (I'm not a native perl speaker, so got a bit confused there.) > > Next, since you want to build against python 2.5+, you need to add a version > constraint to the python-devel build dependency: > BuildRequires: python-devel >= 2.5 Got it. > > > #4 You don't actually need to buildrequire perl(Digest::MD5). It's not > used anywhere in the code. Upstream metadata is incorrect. Removed Digest::MD5 requirement and notified upstream. > > > #5 Next, runtime dependencies. After the package is built, rpmbuild scans > the resulting files and attempts to generate various `requires' and > `provides' for you. This works for perl too, to some extent. The majority > of your dependencies will be found automatically and you don't have to worry > about declaring them explicitly with `Requires'. It doesn't find > everything, though. You'll have to add the following to your list of > runtime dependencies (`Requires'): > - perl(Inline::denter) > - perl(Inline::Filters) -- possibly; this one is optional, up to you Added denter. > > You may drop the following from your list: > - perl(Data::Dumper) -- not used at runtime > - perl(Digest::MD5) -- not needed at all > - perl(Test::More) -- not used at runtime > - python -- rpmbuild generates a library dependency from ldd for you, so > the package will always require the library against which it was built Dropped, thanks. > > The generators also add an unversioned dependency on perl(Inline). You > explicitly require `perl(Inline) >= 0.46', which is correct. To remove the > underspecified generated dependency, you need to add a filter like this: > > %global __requires_exclude > %{?__requires_exclude:%__requires_exclude|}^perl\\(Inline\\)$ > > The most common place to put this is between the dep list and the > %description. > > > #6 The following line isn't needed, feel free to drop it: > find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \; Dropped. > > > #7 Consider using DESTDIR instead of PERL_INSTALL_ROOT. Considered and accepted :) > > > #8 Your changelog format isn't valid (missing e-mail address). > https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs Fixed. > > > #9 Drop META.json and TESTED from %doc. These files aren't really useful > to the end users. Dropped. > > > I hope it's all clear. If not, let me know :) Crystal clear, thanks :) Uploaded new spec and srpm same place as before. -- 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