https://bugzilla.redhat.com/show_bug.cgi?id=1432983 --- Comment #3 from srakitnican <samuel.rakitnican@xxxxxxxxx> --- (In reply to Lubomir Rintel from comment #1) > 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 Well, initially I had it as a separate package, but I've ran into all sort of problems because it is not really intended to be packaged separately, as the author itself told me. For example CAMotics is using configuration files from cbang for scons and build dependencies checking is done as a one unit. Also I have found cbang compiles mostly into static library files. If you still think that should be packaged separately, I can do that. > 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. With debug flag I ran into an issue where compiled binary immediately crashed without starting up. Also compiled binaries file sizes did not look right, e.g. smaller then upstream. So debug_package is here in order for rpmbuild to not error out on missing debuginfo. > 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. > Submited and included, will make a note in spec file. https://github.com/CauldronDevelopmentLLC/CAMotics/issues/211 > 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} cbang does not work with newer version of v8 which Fedora defaults to. I guess conditional is not strictly required, but I though it would be more straightforward to mark what exactly needs it because RHEL still uses 3.14 as default. > 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. Thanks! -- 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
