https://bugzilla.redhat.com/show_bug.cgi?id=1432983 Lubomir Rintel <lrintel@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lrintel@xxxxxxxxxx --- Comment #4 from Lubomir Rintel <lrintel@xxxxxxxxxx> --- (In reply to srakitnican from comment #3) > (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. That is fair enough. However, please add a comment explaining why do you bundle cbang and add a Provides: bundled(cbang) as described in the wiki paragraph linked to above. > > 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. Well, this needs to be fixed first. I could help with that, but am not able to reproduce the issue. Please share as much information as you can -- what version and architecture are you building on? Can you reproduce the bug with a mock build? > > 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 Thank you. > > 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. That is okay. But please add a comment explaining why are you doing that. > > 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're welcome. -- 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
