https://bugzilla.redhat.com/show_bug.cgi?id=1275888 Neal Gompa <ngompa13@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ngompa13@xxxxxxxxx --- Comment #1 from Neal Gompa <ngompa13@xxxxxxxxx> --- Hey Chris, So I'm not picking up and doing a formal review/sponsorship, but I took a look at your spec and there are a few things I'd like to point out: * Your summary confuses me. Where does the "bad intentions" come in? It doesn't sound good. Please reword it in a more useful manner. Likewise, the "bad intentions" probably should be stricken from the description too. * Please use "Source0" instead of "Source". It makes it much easier to deal with if you ever have to have more sources, and it's just a good practice to have. Also note it's a good idea to not use "Patch" and instead "Patch0" for declaring a patch as well. When attached with a number, you can keep incrementing it as you add more of them. * Your BuildRequires, while "legal", are difficult to read as it is all in one line. The currently preferred style is to put each BuildRequire on its own line, but I would suggest that at the minimum to chunk them up such that you have three or four per BuildRequire line. * I am impressed you used the pkgconfig() virtual provides, as not many people use them. Kudos! However, as these can get rather long, it is usually recommended that these are split out into one per line, simply for readability purposes. It's certainly not required, merely a suggestion. * If you are targeting exclusively Fedora, I'd strongly recommend replacing "make %{?_smp_mflags}" with the cleaner "%make_build", as it is the build counterpart to the "%make_install" macro you use now. If you target anything older than EL7, you're going to need to rip out the %make_install macro and replace it with "make DESTDIR=%{buildroot} install". * Your two sed operations should have a comment explaining why you are doing this. Even better, if you produce a patch that could be upstreamed, then you could maintain the changed behavior as a patchset that could be dropped in future updates of the software. -- 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