https://bugzilla.redhat.com/show_bug.cgi?id=1368855 Lubomir Rintel <lkundrak@xxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Flags| |fedora-review+ --- Comment #17 from Lubomir Rintel <lkundrak@xxxxx> --- This starts to look well. Sorry for the delays. Let's help get this sorted out. Thanks for your work on this, having radare in the distro would be very nice. There's a couple of issues remaining, but I think there's a clear way forward. Feel free to ping me on freenode (I'm "lubko", on #nm and #fedora-devel) if anything I suggested below is wrong or unclear, or you need help. That might improve the chances of a speedier response. Also, feel free to reach me with a direct e-mail if this bug needs attention, because the regular bugzilla traffic seems to drown in the loads of bug-mail :( Are you in touch with upstream? Are they by chance open to making life easier for distro maintainers? I'm asking because they seem to be opinionated about the "right" way to install the software, but that doesn't seem to be too well aligned with what we'd need. 0.) Simple things first: >>> URL: http://radare.org/ >>> #URL: https://github.com/radare/radare2 >>Drop a useless comment please. >No. I consider it useful when updating the package. While the browser friendly is the radare.org, when updating the package it is more handy the link to github. That sounds reasonable. Please don't make it masquerade as a commented out URL tag then though. Something like this would look less messy: # GitHub project: https://github.com/radare/radare2 >>> Source0: https://github.com/%{gituser}/%{gitname}/archive/%{commit}/%{name}-%{version}-%{shortcommit}.tar.gz >>> Source1: https://github.com/%{gituser}/%{sdbgitname}/archive/%{sdbcommit}/%{sdbgitname}-%{version}-%{sdbshort}.tar.gz >>This source is not used at all. >True. Not used for the release packing. Source0 used when packing from git directly (in case of patches preventing build on fedora). This looks like a general issue here -- you seem to include a lot of cruft that's only useful for snapshot builds: > %global gituser radare > %global gitname radare2 > %global commit 4b77cb2c36f8c99d09d14ee411e9c5c14b55c609 > %global shortcommit %(c=%{commit}; echo ${c:0:7}) ... > #Release: 1.git%{shortcommit}%{?dist} ... > #Source0: https://github.com/%{gituser}/%{gitname}/archive/%{commit}/%{name}-%{version}-%{shortcommit}.tar.gz It arguably harms readability of the SPEC file. Snapshot builds are largely irrelevant to Fedora and I'd prefer if you left that out. Nevertheless, this seems not to be an exceptional practice, so I don't consider this a review blocker if you insist on leaving it in place. >>> %build >>> %configure --with-sysmagic --with-syszip --with-syscapstone >>You don't enable openssl. Why? (no idea what is it used for) >Option is in the configure, but it is not working in radare for anything now. >At least that was answer from Pancake last time I asked about that (see sys/*.sh for the recommended build path - doesn't contain this option). Fair enough. I'm wondering if you add a comment explaining how do you determine the right configure arguments? >Radare has custom build which doesn't have 2 links to library, but just one. >For example: >/lib64/libr_core.so -> /lib64/libr_core.so.1.6.0 > >Radare2 is linked against the /lib64/libr_core.so: >ldd `which radare2` >linux-vdso.so.1 (0x00007ffce9dfb000) > libdl.so.2 => /lib64/libdl.so.2 (0x00007f349242f000) > libr_core.so => /lib64/libr_core.so (0x00007f34920e1000) > libr_parse.so => /lib64/libr_parse.so (0x00007f3491e8f000) >... > > So it is needed in the binary package in order to run, not just for linking. This is clearly a bug. Their SONAME is wrong: > [lkundrak@belphegor SPECS]$ readelf -a /usr/lib64/libr_core.so.1.4.0 |grep SONAME > 0x000000000000000e (SONAME) Library soname: [libr_core.so] > [lkundrak@belphegor SPECS]$ It should be libr_core.so.1.4.0, not libr_core.so. This is a review blocker since, apart from the proper slit of the -devel package, the SONAME is used for generating the package dependencies and ensuring packages built against incompatible version won't run (and, presumably, crash encountering undefined behavior) against the wrong library. (I can help with a fix if needed.) 1.) Now onto the difficult stuff, which arguably is the overabundance of bundling: 1.1.) The License tag: >> Changed to GPLv3+ due to its viral nature, although majority of the package is > meant to be licensed LGPL. > >I think you need to list all licenses: >https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios Yes. The safest thing to do here is to just include the list of licences joined with an AND. That said, I believe you don't need to include all variants of GPL and LGPL since in general later versions of them are not compatible with the previous thus the "or later" clauses turn then into the later ones. (It seems to be that wherever you say "GPL" and "LGPL" you actually mean "GPL+" and "LGPL+" otherwise it wouldn't make much sense.) Also, in practical terms, "GPLv2+" in "GPLv2+ and ASL2.0" and "GPLv2+ and LGPLv3+" is in fact equivalent to "GPLv3+", since LGPLv3 and ASL2.0 are not compatible with previous versions of GPL, but it's prehaps better to specify the actual licenses as specified in the source files. Thus: License: GPLv2+ and LGPLv3+ and BSD and MIT and ASL2.0 and MPL2.0 would work. The comment about the details is useful. In any case, the License tag is not legally binding. It's somewhat okay to err here (meaning: if someone finds a problem, then it's just a bug to be fixed), especially on the side of caution, as long as the actual licenses permit redistribution and inclusion in Fedora, which seems to be the case. 1.2.) Bundling Javascript First thing: shipping pre-built "minified" files is a huge no-no. It is no longer free software since it strips the user of the freedom to do their modifications (in what GPL refers to as "preferred form") and thus is not appropriate for Fedora. Here's the list of "minified" components I've found in the source: DataTables 1.10.13 FileSaver.js snapshot JointJS 0.9.7 JointJS 1.0.3 jQuery 2.0.3 jQuery 2.2.4 jquery.layout 1.3.0.rc30.79 jquery.onoff 0.3.6 jquery.scrollTo.min.js 2.1.2 jQuery UI 1.11.4 jQuery UI context menu plugin 1.11.0 jQuery UI v1.10.3 lodash 3.10.1 material-design-lite 1.1.3 mdl-selectfield.min.css unknown backbone-min.js unknown Underscore.js 1.8.3 Needless to say, this is a huge mess. There's old and redundant versions of various libraries present there. It's not clear to me how to fix that. Here's what I find to be a good plan on how to proceed: * If the up-to-date versions of the libraries are good enough, they should be replaced with Fedora packages. This is how it's typically done for jQuery. * If a particular version is required, the minified version should be replaced with a pristine non-minified copy in the source package. If minification is desired, then it should be done as a part of the build process. This then requires the bundling exceptions, but I think they could be justified here. The latter can be done either by you (start with "find -name '*[-.]min.*' -delete" in %prep and then replace the files from %{SOURCEx}s) or, better, by upstream. I highly suggest you talk to the upstream -- if you convince them to improve the track of their third party js libraries so that it's always clear how and where to get their sources from it would be a huge improvement. Until this is done, you can drop the web frontend from the package. 2.2.) Bundling of C libraries Upstream seems to have their opinion about the bundling. Let's assume they understand the implications and we need to proceed with it. The guidelines currently allow that: https://fedoraproject.org/wiki/Bundled_Software_policy The first step here would be to audit the bundled libraries and add the Provides: https://fedoraproject.org/wiki/Bundled_Libraries?rd=Packaging:Bundled_Libraries#Requirement_if_you_bundle If there's a known reason for bundling, it would be nice to have a Provide line accompanied with a comment explaining the rationale for bundling. -- 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
