https://bugzilla.redhat.com/show_bug.cgi?id=980851 --- Comment #3 from Dario Faggioli <raistlin@xxxxxxxx> --- First of all, thanks a lot for the review! (In reply to Christopher Meng from comment #1) > 1. Why this? > "%define _binaries_in_noarch_packages_terminate_build 0" > AhAh, here's an easy one! The reason why it is there is that I forgot to remove it! :-( Thanks for noticing it, I'll kill it and update the spec. > 2. No need for "BuildRoot" tag and "%defattr(-,root,root)" and "rm -rf > %{buildroot}", Unless you are packaging for EPEL, but please, when creating > the branch of el6, reedit the file, try not to preserve such lines in modern > packaging system. > Mmm... sorry but I'm not sure I got everything you said. I put/kept those lines in the spec file because I found similar ones in most of the spec files I looked at while seeking "inspiration" for writing mine. :-) Also, I'm not sure I want to package this for EPEL yet. Anyway, if you're saying that I do not need those lines for now, and I should only re-consider adding them later, if/when I decide to package this for EPEL, then I'm fine with that and will remove them. If, OTOH, you were saying something different, could you please excuse me and clarify? > 3. For the files warned by rpmlint, are those just configs which can be > replaced when updating the package? If not add noreplace > Yes, I have a couple of (noreplace), for things in /etc that I want preserved across updates. Those ones it complains about are either sample, template or explanatory files that I actually _do_ want to be updated, so I feel like I should not (noreplace) them. Should I suppress the warning somehow else? > 4. %setup -q is ok, no need for -n with a name which is the default as -q. > Yes, I knew it was redundant, and it was there for the reasons Michael (Comment 2) is mentioning. However, I agree, and I'll kill it until I'll need it. Thanks again. > 5. I haven't tested the package, but are these perl requires cannot be > detected by RPM when installing: > > Requires: perl(Text::Template) > Requires: perl(Config::IniFiles) > Requires: perl(File::Which) > Requires: perl(File::Slurp) > Requires: perl(Data::Dumper) > Oh, I see... I'll double check that and remove the unnecessary 'Requires:', thanks. > 6. Hmm..a README under /etc, will you consider a move of it? > I see what you mean. However, that files provides specific instructions on what happens to the files in /etc/xen-tools/role.d/. It's a lot like '/etc/grub.d/README'. So, yes, I can think about moving it, if that is considered preferrable, but it looks to me that it can actually live there too. I'm living it there for now, let's see what other thinks... > 7. Consider the glob, I think you can shrink the size of spec. And use > %{name} to replace xen-tools in %files if you like. > What do you mean by "Consider the glob, I think you can shrink the size of spec"? About using %{name}, good point, I will do that. > I'm not a sponsor, but these reviews hope can be considered. > Well, I'm certainly really happy about it... Thanks a lot for doing this, revised spec and pkgs coming soon. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=1CzYFeeEOh&a=cc_unsubscribe _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review