Fedora Account System
Red Hat Associate
Red Hat Customer
Spec URL: https://jskarvad.fedorapeople.org/satdump/satdump.spec SRPM URL: https://jskarvad.fedorapeople.org/satdump/satdump-1.2.2%5e20260825git63a23431-1.fc46.src.rpm Description: A generic satellite data processing software. Fedora Account System Username:jskarvad
Copr build: https://copr.fedorainfracloud.org/coprs/build/10902884 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2523675-satdump/fedora-rawhide-x86_64/10902884-satdump/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Found inside by coincidence bundled src-core/nlohmann directory. It seems it contains the same version I have on F43: json-devel-3.11.3-4.fc43.x86_64 vs bundled . If it could be compiled with system provided instance, that should be used instead. But the core core seem to use relative includes only. Some effort should be tried to test, whether it can work with packaged json package. It must declare bundled version in any case. There seem to be more bundled external projects: - https://github.com/ocornut/imgui in src-core/imgui (osgearth package might contain a copy of it already) It also uses src-core/libs and bundles it into the build. It seems unconditional in src-core/CMakeLists.txt, variable GLOB_RECURSE. Suspicious are: - jpeg - jpeg12 - bzlib (bzip2/libbzip2 version 1.1.0 of 6 September 2010 !) - calceph - supernovas - miniz - muparser - rapidxml.hpp is provided also by boost-devel. Not sure it is compatible. I think some effort should be done to use system packages, because there often are matching ones. If they cannot replaced by the system provided files, they MUST be marked by 'bundled(X)' Provides. I think every lib used from src-core/libs should be mentioned, if not unbundled. Especially if it has some LICENSE file inside, which identifies it as a separate project. if they are unmodified, maybe deleting bundled subdirectories and replacing them with symlinks could work often. It should mention explicitly in spec, when some bundled dependency is necessary and difficult to replace with a system package. A lot of those bundles are missing in Fedora. I have not found any suspicious license among them (have not tried really hard), that is good. Otherwise the package is good. and spec is nice. Appstream XML verification is added already.
it seems to me imgui is large enough to deserve its own separate package, its upstream seem active enough. marking it with bundled(imgui) should be only the minimal variant.
(In reply to Petr Menšík from comment #3) > it seems to me imgui is large enough to deserve its own separate package, > its upstream seem active enough. marking it with bundled(imgui) should be > only the minimal variant. The challenge with packaging imgui is that while it is not header-only, it provides no build system and no support for building it as a library, shared or otherwise; it is designed to be used as a large copylib or by including it as a git submodule. It might be possible to design a scheme where imgui is shipped source-only, packaging both the .hpp and .cpp files, and treated as a static library similar to a C++ header-only library, but the right way to do this is not obvious. In general, though, I strongly agree with your comments on bundled system libraries. The requirements in https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling for using bundled libraries are fairly strict and are nearly all MUST rather than SHOULD.
Upstream comment on it (https://github.com/SatDump/SatDump/issues/1196#issuecomment-5416738425): > I read up on that review request and notice there were talks of not using bundled libraries. Please don't do that. There's a reason I bundle things when I do and don't use/want to let CMake find system libraries. Many bundled libraries are also modified in order to do what I needed. > > It also guarantees that since I often rely on specific versions (with specific behavior) things don't randomly break unexpectedly. > > So my answer there would to declared them as bundled in the package as suggested, and please don't touch this, you'll break things even if it may be hard to notice at first.
I checked the debian, it seems they also don't unbundle. Moreover they have the following (interesting) patches: - AMBE drop, IMHO some AMBE variants are still covered by valid SW patents (but will expire soon), I am not sure whether it's this case, but regarding the SW patents I asked upstream: https://github.com/SatDump/SatDump/issues/1196#issuecomment-5417212890 - icon location fix - this is what I already wanted to patch. I will probably grab the patch from debian and notify upstream - desktop file fix - IMHO it validates OK as it is and IMHO debian uses wrong unpreprocessed desktop file, maybe the keywords could be updated, I will think more about it
(In reply to Jaroslav Škarvada from comment #5) > Upstream comment on it > (https://github.com/SatDump/SatDump/issues/1196#issuecomment-5416738425): > > I read up on that review request and notice there were talks of not using bundled libraries. Please don't do that. There's a reason I bundle things when I do and don't use/want to let CMake find system libraries. Many bundled libraries are also modified in order to do what I needed. This seems like an adequate justification for using bundled libraries, particularly the fact that many bundled libraries have been forked, even if it’s unfortunate (https://fedoraproject.org/wiki/Bundled_Libraries#Why_Bundled_Libraries_are_a_problem). You can also link that comment for the requirement to publicly contact upstream about a path to using system libraries. You still need to make sure you have identified all of the bundled dependencies and documented them with virtual Provides.
A couple of other things from a quick glance at the spec file: ---- > %{_libdir}/lib*.so.* To avoid undetected ABI-breaking updates, don’t glob over the SONAME version. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files ---- You need to enumerate *all* of the licenses in the software, including those for bundled dependencies, not just the “top-level” one, and include them in the License field, ideally with a comment breaking down which licenses apply to what. See https://docs.fedoraproject.org/en-US/legal/license-field/#_no_effective_license_analysis and https://docs.fedoraproject.org/en-US/legal/license-field/#_more_complex_example. You also need to make sure that there aren’t any missing mandatory license texts for bundled dependencies; see https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text. All of this can be quite a bit of work for a package with many bundled dependencies and bits of code copied from here and there (as I well know from experience), but it’s required.
I think upstream should be asked to clearly mention, which versions are modified (and briefly why) and which are not. I completely understand why people hate moving libraries with changing behaviour. But at the same time it is then upstream's author duty to watch for possible CVE fixes, even if they do not need later bug fixes or improvements. I doubt that is a common practice by projects bundling external dependencies. It is much easier to work with, but not without (hidden) additional costs. If possible, helping upstream with using stock unmodified libraries plus maybe some external addition would be better for the future. But not without extra work now.