Spec URL: https://alciregi.fedorapeople.org/alizams/alizams.spec SRPM URL: https://alciregi.fedorapeople.org/alizams/alizams-1.7.0-1.fc35.src.rpm Description: A DICOM viewer with many features. DICOMDIR, 2D and 3D views with many tools, de-identify DICOM, compressed images, structured reports, contours, etc. Fedora Account System Username: alciregi
> %global debug_package %{nil} We want debuginfo in Fedora. https://docs.fedoraproject.org/en-US/packaging-guidelines/Debuginfo/ > -DLIBRARY_OUTPUT_PATH=../../../../../%{_vpath_builddir}/lib You can use "$(pwd)/%{_vpath_builddir}" here. > cp -pr %{_vpath_builddir}/lib/libmdcmjpeg8.so.%{so_ver}* %{buildroot}%{_libdir} Those aren't directories, so -r is not needed. > appstream-util validate-relax --nonet \ > %{buildroot}%{_datadir}/metainfo/*.appdata.xml This should be done in %check.
Ok. Thanks. Spec URL: https://alciregi.fedorapeople.org/alizams/alizams.spec SRPM URL: https://alciregi.fedorapeople.org/alizams/alizams-1.7.1-1.fc35.src.rpm
There is a point. In the meanwhile, upstream released 1.7.1 In this release, they added "set(BUILD_SHARED_LIBS OFF)" to mdcm/Utilities/mdcmjpeg/CMakeLists.txt These libraries are specific to this software, and they are still not released (upstream call them a WIP), so there is no way (or at least, it is pretty difficult for me) to build a separate package, and point cmake to use these shared libraries. (As far as I know, libmdcmjpeg8 libmdcmjpeg12 and libmdcmjpeg16 aren't used by any other software). I'm supposed to patch the cmakelists file in order to unbundle them? (Please consider that I'm not a developer, so my understanding of some programming concepts is not so strong).
(In reply to Alessio from comment #3) > There is a point. In the meanwhile, upstream released 1.7.1 > In this release, they added "set(BUILD_SHARED_LIBS OFF)" to > mdcm/Utilities/mdcmjpeg/CMakeLists.txt > These libraries are specific to this software, and they are still not > released (upstream call them a WIP), so there is no way (or at least, it is > pretty difficult for me) to build a separate package, and point cmake to use > these shared libraries. (As far as I know, libmdcmjpeg8 libmdcmjpeg12 and > libmdcmjpeg16 aren't used by any other software). > I'm supposed to patch the cmakelists file in order to unbundle them? (Please > consider that I'm not a developer, so my understanding of some programming > concepts is not so strong). No, if they are not released as libraries that are meant for public consumption by other tools, we treat them as "internal libraries" that are just part of the source code as any other modules would be, and they can be bundled here. If upstream does release them as independent libraries later, we can look at un-bundling them.
(In reply to Ankur Sinha (FranciscoD) from comment #4) > No, if they are not released as libraries that are meant for public > consumption by other tools, we treat them as "internal libraries" that are > just part of the source code as any other modules would be, and they can be > bundled here. If upstream does release them as independent libraries later, > we can look at un-bundling them. Clear. Thanks.
Excuse me @fedora, do you have the time to continue with the review?
The sources used to build the linked SRPM do not match the upstream tarball that's specified as Source0. Please rebuild the package and then I'll review it. Some quick notes in the meantime: In the spec: > %files > %{_datadir}/%{name}/* This will make the package own files inside /usr/share/alizams, but not the directory itself. https://docs.fedoraproject.org/en-US/packaging-guidelines/UnownedDirectories/#_wildcarding_files_inside_a_created_directory rpmlint messages: > alizams.src:13: W: mixed-use-of-spaces-and-tabs (spaces: line 5, tab: line 13) There's a tab between the "URL:" tag and its value. > E: specfile-error warning: bogus date in %changelog: Sun Oct 20 2021 Oct 20 was a Wednesday. In alizams.appdata.xml: > <screenshot type="default"> > <image>https://lh6.googleusercontent.com/gIyaJlPe0i6ZnjKCt4SlshvHG1b9KItl_4DEShBjXx2d5dHXO7f_vQw-UeobzMysC-2iBAqKf03H1ooobNbTfouXoLAHE2GU6mr3hz55W62NqTGt5mmYnSSG2Cltfr9zVA=w1280</image> > </screenshot> This link gives me a 403.
(In reply to Artur Frenszek-Iwicki from comment #7) > The sources used to build the linked SRPM do not match the upstream tarball > that's specified as Source0. Correct. Upstream made some minor changes to the code, releasing a new tag but leaving the version to 1.7.1 > > Some quick notes in the meantime: > > In the spec: > > %files > > %{_datadir}/%{name}/* > This will make the package own files inside /usr/share/alizams, but not the > directory itself. > https://docs.fedoraproject.org/en-US/packaging-guidelines/UnownedDirectories/ > #_wildcarding_files_inside_a_created_directory Yep. > rpmlint messages: > > alizams.src:13: W: mixed-use-of-spaces-and-tabs (spaces: line 5, tab: line 13) > There's a tab between the "URL:" tag and its value. Whoops. > > > E: specfile-error warning: bogus date in %changelog: Sun Oct 20 2021 > Oct 20 was a Wednesday. Whoops. > > In alizams.appdata.xml: > > <screenshot type="default"> > > <image>https://lh6.googleusercontent.com/gIyaJlPe0i6ZnjKCt4SlshvHG1b9KItl_4DEShBjXx2d5dHXO7f_vQw-UeobzMysC-2iBAqKf03H1ooobNbTfouXoLAHE2GU6mr3hz55W62NqTGt5mmYnSSG2Cltfr9zVA=w1280</image> > > </screenshot> > This link gives me a 403. Yes. Upstream had now provided "stable" pictures links: https://github.com/AlizaMedicalImaging/AlizaMS/issues/2#issuecomment-950189749
Spec URL: https://alciregi.fedorapeople.org/alizams/alizams.spec SRPM URL: https://alciregi.fedorapeople.org/alizams/alizams-1.7.1-1.fc35.src.rpm
While the spec is relatively simple, the upstream source is quite complicated and comprised of many parts with different licences. Sorry, but lately I can't find the time to dig into properly reviewing this and figuring out which parts of the source are used and how they relate to each other. I'll keep an eye on this ticket, so if no one else volunteers to review it, I'll give it another chance when I have some more free time.
(In reply to Artur Frenszek-Iwicki from comment #10) > While the spec is relatively simple, the upstream source is quite > complicated and comprised of many parts with different licences. Sorry, but > lately I can't find the time to dig into properly reviewing this and > figuring out which parts of the source are used and how they relate to each > other. > > I'll keep an eye on this ticket, so if no one else volunteers to review it, > I'll give it another chance when I have some more free time. Thank you anyway.
Upstream say [1] that 3rd party licenses can be found here [2] [1] https://github.com/AlizaMedicalImaging/AlizaMS/issues/2#issuecomment-972655826 [2] https://github.com/AlizaMedicalImaging/AlizaMS/blob/master/debian-10/copyright
About 3rd party code. 3rd party code in AlizaMS is the subset of Bullet Physics for collision detection and several other files listed in 'copyright' file in Debian format. https://raw.githubusercontent.com/AlizaMedicalImaging/AlizaMS/development/debian-10/copyright Many of them are not used, because build for the distribution uses system's libraries, e.g. CharLS, OpenJPEG, Zlib, etc. GLEW is used with only with Qt4. BTW, using shared Insight Toolkit (ITK) libraries causes a lot of unused dependencies. In fact AlizaMS uses only a very small part of core ITK, but distribution's ITK depends on huge amount of other libraries, e.g. whole VTK with all dependencies, VNL, GDCM and many other. In my own builds i always link ITK statically. Probably it is not possible for distribution release (policy). AlizaMS works with ITK shared libraries too, of course, without any problems, the only side effect is rather big download of dependencies, several hundreds MBs. IMHO, ITK and VTK are huge frameworks with countless cross-dependencies and frequent updates, it is difficult to maintain them, the distribution packages are mostly old versions, so most users use own builds, AFAIK. BTW, any ITK version from 4.12 to current 5.3 will work for AlizaMS. For testing with Valgrind, if required, i would recommend to disable accelerated 3D in 'Settings' or simply start "alizams -nogl", otherwise suppression file specific to a driver may be required. Some false positives may come from system libraries, but not much. e.g. Qt may also require special suppression file. If you have questions please don't hesitate to open an issue (Github) or comment. Thank you very much! Kind regards, Mikhail
> %global github_name AlizaMS The usual comment: if you really want to, this is allowed. But to me it makes no sense to use such pointless macros. They make the spec file harder to read, longer (!), and also break select-and-paste-and-use and/or ctrl-click-to-open. > %description > A DICOM viewer. Very fast directory scanner, DICOMDIR. 2D and 3D views with > many tools. View uniform and non-uniform series in physical space. > Consistently de-identify DICOM. View DICOM metadata. Ultrasound with proper > measurement in regions, cine. Scout (localizer) lines. Grayscale softcopy > presentation. Structured report. Compressed images. RTSTRUCT contours. > Siemens mosaic format. United Imaging Healthcare (UIH) Grid / VFrame format. > Elscint ELSCINT1 PMSCT_RLE1 and PMSCT_RGB1 That reads like a bullet list compressed into paragraph format. Maybe try to make it a bit more like a normal text? Also there's a list of formats at the end, I'm not sure what it means. > # Install appdata > install -d %{buildroot}%{_datadir}/metainfo > install -p -m 0644 %{SOURCE1} \ > %{buildroot}%{_datadir}/metainfo install -Dpt %{buildroot}%{_datadir}/metainfo/ -m 0644 %{SOURCE1} In %check, it'd be nice to at least call the installed binaries to see if the execute correctly. -- Mikhail, thank you for the comment about licensing. > Many of them are not used, because build for the distribution uses system's libraries, e.g. CharLS, OpenJPEG, Zlib, etc. GLEW is used with only with Qt4. It'd be great to remove as much of the unused sources as possible in %prep. This way it'll be clearer what is used (and what we need to look at).
vectormath: we don't have this packaged mdcm: not packaged colorspace: not packaged I think that for those three it would be reasonable to add Provides: bundled(vectormath), etc. bullet: there is a package. I think it should be possible to BR:bullet-devel and use the system lib.
Thank you very much! > bullet: there is a package. I think it should be possible to BR:bullet-devel and use the system lib. Yes, i have looked at the package. It is possible to use system Bullet, requires only a little work with CMakeLists.txt file. I shall do it and let you know, it can take several days.
(In reply to Zbigniew Jędrzejewski-Szmek from comment #14) > In %check, it'd be nice to at least call the installed binaries to see if > the execute > correctly. Excuse me, in what sense? If I call the binary, rpmbuild fails.
Usually we'd do something like '%buildroot/%_bindir/foobar --help >/dev/null' or call the tool with some sample input and check if exists successfully. Such a "test" is quite useful to check for trivial errors, like forgetting to link to some library. But in this case it indeed some that it'd be hard to do this. It seems that /usr/bin/alizams ignores all command-line options, and only works in graphical mode.
> bullet: there is a package. I think it should be possible to BR:bullet-devel and use the system lib. Done. There is the option ALIZA_USE_SYSTEM_BULLET. S. https://raw.githubusercontent.com/AlizaMedicalImaging/AlizaMS/master/fedora-34/alizams.spec for SPEC template. > It'd be great to remove as much of the unused sources as possible in %prep. It works. rm -fr mdcm/Utilities/mdcmzlib/ rm -fr mdcm/Utilities/mdcmopenjpeg/ rm -fr mdcm/Utilities/mdcmcharls/ rm -fr mdcm/Utilities/mdcmuuid/ rm -fr mdcm/Utilities/pvrg/ rm -fr b/ rm -fr CG/glew/ S. %prep section (link above) > think that for those three it would be reasonable to add Provides: bundled(vectormath), etc. Vectormath is header-only. MDCM is purposely part of AlizaMS. Maybe it will be released as library later, not yet sure. Colorspace is slightly modified, turned into C++ pure static class. So even if somebody will package https://getreuer.info/posts/colorspace/index.html it will not work. It is one .h and one .cpp file, easier to add to source. Version is 1.7.2 now. Thank you very much! P.S. There was a very minor issue in alizams.appdata.xml in screenshot section in latest rpm. My suggestion would be <screenshots> <screenshot type="default"> <image>https://raw.githubusercontent.com/AlizaMedicalImaging/AlizaMS/master/package/art/alizams_scr1.jpg</image> </screenshot> <screenshot> <image>https://raw.githubusercontent.com/AlizaMedicalImaging/AlizaMS/master/package/art/alizams_scr2.jpg</image> </screenshot> </screenshots>
(In reply to issakomi from comment #19) > > bullet: there is a package. I think it should be possible to BR:bullet-devel and use the system lib. > > Done. There is the option ALIZA_USE_SYSTEM_BULLET. Great. > Version is 1.7.2 now. > Are you providing a v1.7.2 tag? > P.S. There was a very minor issue in alizams.appdata.xml in screenshot > section in latest rpm. My suggestion would be Yes. Thanks
> Are you providing a v1.7.2 tag? I shall add the tag.
The purpose of 'Provides:bundled(…)' is to allow listing all packages that have an embedded copy of some library. For example, if we figure out that that library has some bug, we do 'dnf repoquery --whatprovides "bundled(foo)"' and have a list of packages to patch. So the fact that the library is header-only doesn't matter too much. What is relevant is whether the library is a separate project. "Internal libraries" that are not separate projects don't need the tag. > MDCM is purposely part of AlizaMS. Maybe it will be released as library later, not yet sure. OK, so no tag then. > Vectormath is header-only. > Colorspace is slightly modified, turned into C++ pure static class. It sounds like the tags should be added.
(In reply to Zbigniew Jędrzejewski-Szmek from comment #22) > It sounds like the tags should be added. Provides: bundled(vectormath) Provides: bundled(colorspace) Sounds good?
> Provides: bundled(vectormath) > Provides: bundled(colorspace) +1
The new SPEC and SRPM files: https://alciregi.fedorapeople.org/alizams/alizams.spec https://alciregi.fedorapeople.org/alizams/alizams-1.7.2-1.fc35.src.rpm
Thank you! > The new SPEC and SRPM files: %changelog * Mon Nov 22 2021 Alessio <alessio> - 1.7.1-1 1.7.2-1
(In reply to issakomi from comment #26) > Thank you! > > > The new SPEC and SRPM files: > > %changelog > * Mon Nov 22 2021 Alessio <alessio> - 1.7.1-1 > > 1.7.2-1 Argh.
> > 1.7.2-1 > Argh. Thank you for update! I have build the package from source rpm and tested, LGTM. BTW, here is the collection of challenging DICOM data sets, could be used for tests. https://www.aliza-medical-imaging.de/Aliza_tests.7z
(In reply to issakomi from comment #28) > Thank you for update! I have build the package from source rpm and tested, > LGTM. > BTW, here is the collection of challenging DICOM data sets, could be used > for tests. > https://www.aliza-medical-imaging.de/Aliza_tests.7z Thank you.
> https://www.aliza-medical-imaging.de/Aliza_tests.7z There is a folder "huge_dicom_16GB_RAM_recommended", a dialog with confirmation may appear, better to cancel load on machine with less than 16GB RAM to avoid paging.
%description → wrap to <=80 columns. Maybe after "de-identify" add "(remove personal information)" or something. The term might not be obvious to most people. + package name is OK + latest version + license is acceptable for Fedora (GPLv3) + license is specified correctly + BR/R/P look OK + %check is present and passes (though very minimalistic…) + build and installs and runs OK rpmlint: alizams.src:34: W: unversioned-explicit-provides bundled(vectormath) alizams.src:35: W: unversioned-explicit-provides bundled(colorspace) This is OK: in principle the guidelines say that a version should be provided, but figuring this out is onerous and often the exact version cannot be determined. And when the bundled code has been modified, as is the case here, no number would make sense. alizams.src: W: spelling-error Summary(en_US) Aliza -> Alissa, Alisa, Eliza alizams.src: W: spelling-error %description -l en_US de -> DE, ed, d alizams.x86_64: W: spelling-error Summary(en_US) Aliza -> Alissa, Alisa, Eliza alizams.x86_64: W: spelling-error %description -l en_US de -> DE, ed, d 4 packages and 0 specfiles checked; 0 errors, 6 warnings. All good. Package is APPROVED.
(In reply to Zbigniew Jędrzejewski-Szmek from comment #31) > %description > → wrap to <=80 columns. > Maybe after "de-identify" add "(remove personal information)" or something. > The term might not > be obvious to most people. Ok. > > Package is APPROVED. Great. Thanks!
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/alizams
FEDORA-2021-a31990aef3 has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2021-a31990aef3
FEDORA-2021-18285a5507 has been submitted as an update to Fedora 34. https://bodhi.fedoraproject.org/updates/FEDORA-2021-18285a5507
FEDORA-2021-18285a5507 has been pushed to the Fedora 34 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-18285a5507 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-18285a5507 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2021-a31990aef3 has been pushed to the Fedora 35 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-a31990aef3 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-a31990aef3 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2021-a31990aef3 has been pushed to the Fedora 35 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2021-18285a5507 has been pushed to the Fedora 34 stable repository. If problem still persists, please make note of it in this bug report.