https://bugzilla.redhat.com/show_bug.cgi?id=1432983 Lubomir Rintel <lkundrak@xxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |lkundrak@xxxxx Assignee|nobody@xxxxxxxxxxxxxxxxx |lkundrak@xxxxx Flags| |fedora-review? --- Comment #1 from Lubomir Rintel <lkundrak@xxxxx> --- This looks rather well done. Just a couple of comments: 1.) Please unbundle cbang > %global commit1 9f80bae739a36727a5f66e5f1b0c55cd9ee1c01a > %global name1 cbang > %global shortcommit1 %(c=%{commit1}; echo ${c:0:7}) ... > Source1: https://github.com/CauldronDevelopmentLLC/cbang/archive/%{commit1}.tar.gz#/%{name1}-%{shortcommit1}.tar.gz ... Looks like this should go into a separate package. See: https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries 2.) Don't disable debuginfo > # With debug flags camotics executable does not run > %global debug_package %{nil} ... > # Trim ~2MB more, manually since not a debug build > %{__strip} %{buildroot}%{_bindir}/* Eeek! Don't do this. What didn't work here? It certainly works all right on my system. 3.) Submit the MIME info upstream > Source2: camotics.xml It's okay to add it in the package, but please ask upstream to include it and add a comment with a link to the upstream ticket. 4.) Why is this conditional on Fedora? > cd %{_builddir}/%{name1}-%{commit1} > scons ccflags="%{?fedora:-I%{_includedir}/v8-3.14/}" %{?_smp_mflags} > > cd %{_builddir}/CAMotics-%{version} > scons ccflags="%{?fedora:-I%{_includedir}/v8-3.14/}" %{?_smp_mflags} 5.) Please add BuildRequires: gcc-c++ The guindelines seem to mandate it now: https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B The rest of the review will follow. I'm a packager sponsor, so I can eventually sponsor you when we sort this review out. -- 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 To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
