Spec URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc-0.4.0-1.fc17.spec SRPM URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc-0.4.0-1.fc17.src.rpm Description: Orthanc aims at providing a simple, yet powerful standalone DICOM server. Orthanc can turn any computer running Windows or Linux into a DICOM store (in other words, a mini-PACS system). Its architecture is lightweight, meaning that no complex database administration is required, nor the installation of third-party dependencies. What makes Orthanc unique is the fact that it provides a RESTful API. Thanks to this major feature, it is possible to drive Orthanc from any computer language. The DICOM tags of the stored medical images can be downloaded in the JSON file format. Furthermore, standard PNG images can be generated on-the-fly from the DICOM instances by Orthanc. Orthanc lets its users focus on the content of the DICOM files, hiding the complexity of the DICOM format and of the DICOM protocol. Fedora Account System Username: s.jodogne Review Description: I am the upstream maintainer of Orthanc, and this is my first package for Fedora. I am therefore looking for a sponsor. The mock tool has successfully run on the package.
Hi Sebastien. I'm not an official packager/reviewer; I'm just doing few comments. Although this is your first package, it is already good in my opinion. - You should pick a style and use it consistently throughout your packaging so install -m 755 -d %{buildroot}%{_mandir}/man1 becomes install -m 755 -d $RPM_BUILD_ROOT%{_mandir}/man1 and so on. https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS - BuildRequires: systemd-units On Fedora 18 and newer, the systemd-units subpackage has been merged into the base systemd package, so your package for those targets can instead include. Fedora 18 is almost among us, maybe you could use DisTag to mark this requires entry depending on Fedora release. https://fedoraproject.org/wiki/Packaging:DistTag?rd=DistTag https://fedoraproject.org/wiki/Packaging:Systemd?rd=Packaging:Guidelines:Systemd#Filesystem_locations
Hi Antonio, Thank you much for your feedback! I have just uploaded an updated version of my package that takes your comments into account: Spec URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc-0.4.0-2.fc17.spec SRPM URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc-0.4.0-2.fc17.src.rpm
Note: this is an 'unofficial package review'; it could contain errors and is not officially valid. :) Package Review ============== Key: [x] = Pass [!] = Fail [-] = Not applicable [?] = Not evaluated [ ] = Manual review needed Issues: ======= [!]: Spec file name must match the spec package %{name}, in the format %{name}.spec. Note: orthanc-0.4.0-2.fc17.spec should be orthanc.spec See: http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Spec_file_name Rename .spec file: orthanc.spec ===== MUST items ===== C/C++: [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Rpath absent or only used for internal libs. Generic: [!]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. There are two components differently licensed (according to the README file included on package). The 'License' tag for the rpm so should be 'GPLv3+ and BSD and MIT'. However README file reports a 'GPLv3 license with the OpenSSL exception' that seems not compatible with GPLv3 (see http://fedoraproject.org/wiki/Licensing:Main) In my opinion, you should contact Fedora Project Legal (http://fedoraproject.org/wiki/Legal:Main) or wait other comments. [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package contains no bundled libraries. [x]: Changelog in prescribed format. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. Note: rm -rf %{buildroot} present but not required [x]: Sources contain only permissible code or content. [x]: %config files are marked noreplace or the reason is justified. [x]: Each %files section contains %defattr if rpm < 4.4 [x]: Macros in Summary, %description expandable at SRPM build time. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [-]: Package requires other packages for directories it uses. [-]: Package uses nothing in %doc for runtime. [x]: Package is not known to require ExcludeArch. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package complies to the Packaging Guidelines [x]: Spec file lacks Packager, Vendor, PreReq tags. [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [!]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "GPL (v3 or later)", "Unknown or generated", "MIT/X11 (BSD like)", "zlib/libpng", "BSD (3 clause)", "BSD (2 clause)". 6 files have unknown license. Detailed output of licensecheck in /home/sagitter/888301-orthanc-0.4.0-2.fc17/licensecheck.txt [x]: Package consistently uses macro is (instead of hard-coded directory names). [x]: Package is named using only allowed ASCII characters. [x]: Package is named according to the Package Naming Guidelines. [x]: No %config files under /usr. [x]: Package does not generate any conflict. Note: Package contains no Conflicts: tag(s) [x]: Package do not use a name that already exist [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package installs properly. [x]: Package is not relocatable. [x]: Requires correct, justified where necessary. [x]: CheckResultdir [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file is legible and written in American English. [!]: Spec file name must match the spec package %{name}, in the format %{name}.spec. Note: orthanc-0.4.0-2.fc17.spec should be orthanc.spec Rename .spec file: orthanc.spec [x]: Package contains systemd file(s) if in need. [x]: File names are valid UTF-8. [x]: Useful -debuginfo package or justification otherwise. [-]: Large documentation must go in a -doc subpackage. Note: Documentation size is 51200 bytes in 4 files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: Reviewer should test that the package builds in mock. orthanc-0.4.0-2.fc17.i686.rpm successfully builded by using mock. orthanc-0.4.0-2.fc18.x86_64.rpm successfully builded by using rpmbuild. In both cases, rpmlint shows this warning: W: only-non-binary-in-usr-lib. I think it is related to https://bugzilla.redhat.com/show_bug.cgi?id=794777 [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Dist tag is present. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [?]: Package functions as described. rpm correctly installed, orthanc.service created, orthanc group created but user must be added manually to it. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [!]: Patches link to upstream bugs/comments/lists or are otherwise justified. If exists, add the link to every patch otherwise at least include a comment right above. https://fedoraproject.org/wiki/Packaging:PatchUpstreamStatus?rd=Packaging/PatchUpstreamStatus [x]: The placement of pkgconfig(.pc) files are correct. [-]: Scriptlets must be sane, if used. [x]: SourceX tarball generation or download is documented. [x]: SourceX / PatchY prefixed with %{name}. Note: Source2 (jsoncpp-src-0.5.0.tar.gz) Source0 (Orthanc-0.4.0.tar.gz) Source1 (mongoose-3.1.tgz) [x]: SourceX is a working URL. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Spec use %global instead of %define. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Rpmlint ------- Checking: orthanc-debuginfo-0.4.0-2.fc16.x86_64.rpm orthanc-0.4.0-2.fc16.x86_64.rpm orthanc-0.4.0-2.fc16.src.rpm orthanc.x86_64: W: spelling-error Summary(en_US) healthcare -> health care, health-care, ethereal orthanc.x86_64: W: non-standard-uid /var/lib/orthanc orthanc orthanc.x86_64: W: non-standard-gid /var/lib/orthanc orthanc orthanc.x86_64: W: non-standard-uid /var/lib/orthanc/db-v3 orthanc orthanc.x86_64: W: non-standard-gid /var/lib/orthanc/db-v3 orthanc orthanc.src: W: spelling-error Summary(en_US) healthcare -> health care, health-care, ethereal 3 packages and 0 specfiles checked; 0 errors, 6 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint orthanc orthanc-debuginfo orthanc.x86_64: I: enchant-dictionary-not-found en_US orthanc.x86_64: W: non-standard-uid /var/lib/orthanc orthanc orthanc.x86_64: W: non-standard-gid /var/lib/orthanc orthanc orthanc.x86_64: W: non-standard-uid /var/lib/orthanc/db-v3 orthanc orthanc.x86_64: W: non-standard-gid /var/lib/orthanc/db-v3 orthanc 2 packages and 0 specfiles checked; 0 errors, 4 warnings. # echo 'rpmlint-done:' Requires -------- orthanc-debuginfo-0.4.0-2.fc16.x86_64.rpm (rpmlib, GLIBC filtered): orthanc-0.4.0-2.fc16.x86_64.rpm (rpmlib, GLIBC filtered): /bin/sh config(orthanc) = 0.4.0-2.fc16 libboost_filesystem-mt.so.1.47.0()(64bit) libboost_system-mt.so.1.47.0()(64bit) libboost_thread-mt.so.1.47.0()(64bit) libc.so.6()(64bit) libcrypto.so.10()(64bit) libcurl.so.4()(64bit) libdcmdata.so.3.6()(64bit) libdcmnet.so.3.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libglog.so.0()(64bit) libm.so.6()(64bit) liboflog.so.3.6()(64bit) libofstd.so.3.6()(64bit) libpng12.so.0()(64bit) libpng12.so.0(PNG12_0)(64bit) libpthread.so.0()(64bit) libsqlite3.so.0()(64bit) libssl.so.10()(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(CXXABI_1.3.1)(64bit) libuuid.so.1()(64bit) libuuid.so.1(UUID_1.0)(64bit) libz.so.1()(64bit) libz.so.1(ZLIB_1.2.0)(64bit) rtld(GNU_HASH) shadow-utils systemd-units Provides -------- orthanc-debuginfo-0.4.0-2.fc16.x86_64.rpm: orthanc-debuginfo = 0.4.0-2.fc16 orthanc-debuginfo(x86-64) = 0.4.0-2.fc16 orthanc-0.4.0-2.fc16.x86_64.rpm: config(orthanc) = 0.4.0-2.fc16 orthanc = 0.4.0-2.fc16 orthanc(x86-64) = 0.4.0-2.fc16 MD5-sum check ------------- http://downloads.sourceforge.net/project/jsoncpp/jsoncpp/0.5.0/jsoncpp-src-0.5.0.tar.gz : CHECKSUM(SHA256) this package : 22b14ecd0de8cdad2b6b6839f6d0804d3b84e91f42861ebd843832a26a927433 CHECKSUM(SHA256) upstream package : 22b14ecd0de8cdad2b6b6839f6d0804d3b84e91f42861ebd843832a26a927433 https://orthanc.googlecode.com/files/Orthanc-0.4.0.tar.gz : CHECKSUM(SHA256) this package : ca2bf9b176a307141b9f72c7483431efc2d7aa48116c64ba4e8a80ac55548e3a CHECKSUM(SHA256) upstream package : ca2bf9b176a307141b9f72c7483431efc2d7aa48116c64ba4e8a80ac55548e3a https://mongoose.googlecode.com/files/mongoose-3.1.tgz : CHECKSUM(SHA256) this package : fd003ff722d8b654a6ceaaadeffb1806d2d513afe888ba00ecfb4a115897844c CHECKSUM(SHA256) upstream package : fd003ff722d8b654a6ceaaadeffb1806d2d513afe888ba00ecfb4a115897844c Generated by fedora-review 0.3.1 (b71abc1) last change: 2012-10-16 Buildroot used: fedora-16-x86_64 Command line :/usr/bin/fedora-review -b 888301 In %pre # http://fedoraproject.org/wiki/Packaging%3aUsersAndGroups # "We never remove users or groups created by packages" getent group orthanc >/dev/null || groupadd -r orthanc getent passwd orthanc >/dev/null || \ useradd -r -g orthanc -d %{_sharedstatedir}/orthanc -s /sbin/nologin \ -c "User account that holds information for Orthanc" orthanc 'exit 0' is omitted at the end. Why ? http://fedoraproject.org/wiki/Packaging%3aUsersAndGroups
Spec URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc.spec SRPM URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc-0.4.0-3.fc17.src.rpm Dear Antonio, Once again, thanks for your help ! :) I have just uploaded a new version of the package that should meet your concerns. > [!]: Spec file name must match the spec package %{name}, in the format > %{name}.spec. > Note: orthanc-0.4.0-2.fc17.spec should be orthanc.spec Fixed. > [!]: Package is licensed with an open-source compatible license and meets > other legal requirements as defined in the legal section of Packaging > Guidelines. > > There are two components differently licensed (according to the README file > included on package). > The 'License' tag for the rpm so should be 'GPLv3+ and BSD and MIT'. However > README file reports a > 'GPLv3 license with the OpenSSL exception' that seems not compatible with > GPLv3 (see http://fedoraproject.org/wiki/Licensing:Main) > In my opinion, you should contact Fedora Project Legal > (http://fedoraproject.org/wiki/Legal:Main) or wait other comments. Regarding the OpenSSL exception, I have sent a mail to the Legal mailing list: http://lists.fedoraproject.org/pipermail/legal/2012-December/002051.html Regarding the BSD/MIT/... components, I understand that it is sufficient to write "GPLv3" since this corresponds to the licence to the Orthanc binaries: https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#How_should_I_handle_multiple_licensing_situations.3F Please also note that the complete list of the third-party components inside Orthanc can be found in the Debian package: http://packages.debian.org/changelogs/pool/main/o/orthanc/orthanc_0.4.0-1/orthanc.copyright > [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the > beginning of %install. > Note: rm -rf %{buildroot} present but not required This line has been removed. > [!]: Spec file name must match the spec package %{name}, in the format > %{name}.spec. > Note: orthanc-0.4.0-2.fc17.spec should be orthanc.spec > Rename .spec file: orthanc.spec Fixed. > Generic: > [x]: Reviewer should test that the package builds in mock. > > orthanc-0.4.0-2.fc17.i686.rpm successfully builded by using mock. > orthanc-0.4.0-2.fc18.x86_64.rpm successfully builded by using rpmbuild. > In both cases, rpmlint shows this warning: W: only-non-binary-in-usr-lib. > I think it is related to https://bugzilla.redhat.com/show_bug.cgi?id=794777 Indeed, this problem stems from the installation of the Systemd service for Orthanc. I have spotted another package that has been accepted with the same warning, so I think it should be considered as a false positive: https://bugzilla.redhat.com/show_bug.cgi?id=871605 > [?]: Package functions as described. > rpm correctly installed, orthanc.service created, orthanc group created but > user must be added manually to it. Nice catch! Fixed. > [!]: Patches link to upstream bugs/comments/lists or are otherwise justified. > If exists, add the link to every patch otherwise at least include a comment > right above. Fixed. > Rpmlint > ------- > Checking: orthanc-debuginfo-0.4.0-2.fc16.x86_64.rpm > orthanc-0.4.0-2.fc16.x86_64.rpm > orthanc-0.4.0-2.fc16.src.rpm > orthanc.x86_64: W: spelling-error Summary(en_US) healthcare -> health care, > health-care, ethereal > orthanc.x86_64: W: non-standard-uid /var/lib/orthanc orthanc > orthanc.x86_64: W: non-standard-gid /var/lib/orthanc orthanc > orthanc.x86_64: W: non-standard-uid /var/lib/orthanc/db-v3 orthanc > orthanc.x86_64: W: non-standard-gid /var/lib/orthanc/db-v3 orthanc > orthanc.src: W: spelling-error Summary(en_US) healthcare -> health care, > health-care, ethereal > 3 packages and 0 specfiles checked; 0 errors, 6 warnings. In my humble opinion, these warnings are false positives. Indeed, according to an online dictionary, "healthcare" does exist: http://dictionary.reference.com/browse/healthcare For the UID/GID problem, I think it is a good practice to introduce a user and a group that hold the data of the server, with the appropriate permissions. > In > %pre [...] 'exit 0' is omitted at the end. Why ? > http://fedoraproject.org/wiki/Packaging%3aUsersAndGroups Cut and paste problem... Fixed.
I'm concerned about bundled 3rd party library - mongoose v3.1 (available in Fedora, ver. 3.1). You should a) strip it off and build against system-wide copy b) explain why it's impossible / impractical (heavy changed for example) and ask FESCo for the exception.
Peter, > I'm concerned about bundled 3rd party library - mongoose v3.1 (available in > Fedora, ver. 3.1). You should > a) strip it off and build against system-wide copy Thank you for pointing this issue: I was not aware of the existence of the mongoose-lib package (it is not present in the Debian distribution). A new version of the package is available that dynamically links against the system-wide mongoose library: Spec URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc.spec SRPM URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc-0.4.0-4.fc17.src.rpm
Wow: a fine package and so many changes in only a couple of days! Sebastian, I'll review it!
Mario, in FAS you're listed as "Fedora Packager GIT Commit Group (user)" but the FE-NEEDSPONSOR flag set on this ticket means that "it is the first package of a Contributor, the Reviewer must be a Sponsor": https://fedoraproject.org/wiki/Package_Review_Process#Reviewer [...] A very brief look at just the spec file after it has seen a few reviews already: > Source1: http://downloads.sourceforge.net/project/jsoncpp/jsoncpp/0.5.0/jsoncpp-src-0.5.0.tar.gz What's that? Looks like a bundled jsoncpp 0.5.0: https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries Find a review request for jsoncpp 0.6.0 here: bug 882617 > %doc %{_mandir}/man1/*.1* Hint: %_mandir is marked %doc automatically. > %dir %attr(0755, root, root) %{_sysconfdir}/orthanc > %dir %attr(0755, orthanc, orthanc) %{_sharedstatedir}/orthanc > %dir %attr(0755, orthanc, orthanc) %{_sharedstatedir}/orthanc/db-v3 > %config(noreplace) %attr(0644, root, root) %{_sysconfdir}/orthanc/orthanc.json What happens if you limit usage of %attr to just the two directoris that need an orthanc:orthanc ownership change? Will the other two be incorrect then? I ask that, because preferably %attr is used only for overriding ownership/permissions where the default %install is insufficient. For ordinary files and directories, prefer the %install section as well as fixes to the upstream source. Then %attr would be used only for special situations, i.e. where the default is not enough. * An automated build attempt in Plague/Mock here failed: ... make[2]: python: Command not found make[2]: Leaving directory `/builddir/build/BUILD/Orthanc-0.4.0' Note that you could submit a scratch build in the Fedora Build System: https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Install_the_client_tools_.28Koji.29_and_set_up_your_certificate
Oops mid-air collision!! :) Hi again, some miscellaneous findings before a more through review: * There are at least two licenses: MIT (OrthancCppClient) and GPLv3. This should be reflected in the SPEC file as in: https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios * Core/SQlite contains an sqlite wrapper from chrome. Is it under BSD license. I see there are some modifications specifically made for Orthanc, which could qualify as a fork. Sebastian, in order to understand if this is the case, would you mind to comment (briefly) on these questions (https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Standard_questions)? * No need to bundle jsoncpp, it is currently in review here: https://bugzilla.redhat.com/show_bug.cgi?id=882617 * I see a lot of cmake files that deal with 3rd part libraries. If you don't use them, why don't you "rm -rf" them in the %prep section? Just to be sure we are not using them? Please comment on these issues and I'll continue with the review.
(In reply to comment #8) > Mario, in FAS you're listed as "Fedora Packager GIT Commit Group (user)" > but the FE-NEEDSPONSOR flag set on this ticket means that "it is the first > package of a Contributor, the Reviewer must be a Sponsor": > https://fedoraproject.org/wiki/Package_Review_Process#Reviewer Yes Micheal you are right, Peter agreed to sponsor Sebastian as soon as the package is approved. If you prefer to take the review, I'm okay with that :) > > Find a review request for jsoncpp 0.6.0 here: bug 882617 > Indeed :) > > > * An automated build attempt in Plague/Mock here failed: > ... > make[2]: python: Command not found > make[2]: Leaving directory `/builddir/build/BUILD/Orthanc-0.4.0' > > Note that you could submit a scratch build in the Fedora Build System: > > https://fedoraproject.org/wiki/ > Join_the_package_collection_maintainers#Install_the_client_tools_.28Koji. > 29_and_set_up_your_certificate Mmmm... strange, I just mocked it and it worked... let's try with a scratch build!
"BuildRequires: python" is missing indeed. At least for F18, because neither is python on the list of BuildRequires exceptions, https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2 nor is it listed in root.log for Koji/Plague based in Mock.
s/based/based builds/
(In reply to comment #9) Hi Mario. > Oops mid-air collision!! :) > > Hi again, some miscellaneous findings before a more through review: > > * There are at least two licenses: MIT (OrthancCppClient) and GPLv3. This > should be reflected in the SPEC file as in: > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/ > LicensingGuidelines#Multiple_Licensing_Scenarios > The licenses issue have already been bring up (see comment #3). :) Hi Michael >> * An automated build attempt in Plague/Mock here failed: >> ... >> make[2]: python: Command not found >> make[2]: Leaving directory `/builddir/build/BUILD/Orthanc-0.4.0' >> >> Note that you could submit a scratch build in the Fedora Build System: >> >> https://fedoraproject.org/wiki/ >> Join_the_package_collection_maintainers#Install_the_client_tools_.28Koji. >> 29_and_set_up_your_certificate >Mmmm... strange, I just mocked it and it worked... let's try with a scratch >build! For me too ... >> %doc %{_mandir}/man1/*.1* >Hint: %_mandir is marked %doc automatically. I was uncertain about this line. Is it harmful for the package's building ?
Created attachment 666287 [details] fedora-18-x86_64 mock build.log Build failure with "mock --rebuild orthanc-0.4.0-4.fc17.src.rpm". No "python" installed in the buildroot: $ grep python root.log DEBUG util.py:264: python-libs-2.7.3-13.fc18.x86_64 DEBUG util.py:264: boost-python x86_64 1.50.0-4.fc18 fedora 123 k DEBUG util.py:264: boost-python3 x86_64 1.50.0-4.fc18 fedora 126 k DEBUG util.py:264: boost-python.x86_64 0:1.50.0-4.fc18 DEBUG util.py:264: boost-python3.x86_64 0:1.50.0-4.fc18 >> %doc %{_mandir}/man1/*.1* >> >> Hint: %_mandir is marked %doc automatically. > > I was uncertain about this line. Is it harmful for the package's building ? Not harmful. Only redundant. Many reviewers point it out.
Hi again, > * There are at least two licenses: MIT (OrthancCppClient) and GPLv3. This > should be reflected in the SPEC file as in: > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/ > LicensingGuidelines#Multiple_Licensing_Scenarios I am not comfortable enough with legal stuff, but as I understand on the following page, I should just specify "GPLv3" because it corresponds to the license of the Orthanc binaries that are shipped inside the package: https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#How_should_I_handle_multiple_licensing_situations.3F "Here are some common cases: [...] The source code contains some .c files which are GPLv2+ and some other .c files which are BSD. They're compiled together to form an executable. Since some of the files are licensed as GPL, the resulting executable is also GPL. The License tag should read: License: GPLv2+" Let me know if I'm wrong, but I think that the different licensing schemes for the OrthancCppClient and the Core/SQLite folders are upstream-related stuff, and do not interfere with the Fedora package. > * Core/SQlite contains an sqlite wrapper from chrome. Is it under BSD > license. I see there are some modifications specifically made for Orthanc, > which could qualify as a fork. Sebastian, in order to understand if this is > the case, would you mind to comment (briefly) on these questions > (https://fedoraproject.org/wiki/Packaging: > No_Bundled_Libraries#Standard_questions)? I think that this work should not be considered as a fork of Chrome, since it is just an excerpt of a few files that happen to compile well outside the Chrome framework: Chrome does not provide a standalone SQLite wrapper by itself, and thus this folder is not a fork. > * I see a lot of cmake files that deal with 3rd part libraries. If you don't > use them, why don't you "rm -rf" them in the %prep section? Just to be sure > we are not using them? Well, the CMake framework of Orthanc is rather complex, as Orthanc is built so as to compile as well on Windows and on several flavors of Linux: This notably implies that it is possible to statically link Orthanc against all the third-party dependencies. Even if only 5% of the Resources/CMake folder is used for dynamic linking in Linux, the upstream package is maintained for more general linking scenarios. I have the feeling that it would not be a good thing to modify the CMake scripts just for the Fedora package, because the same work would have to be done again and again with each release of Orthanc, which would be error-prone and time-consuming. Cheers, Sébastien-
Spec URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc.spec SRPM URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc-0.4.0-5.fc17.src.rpm Hi all, Thank you much for your massive feedback on this package! I have just uploaded another version of the package with your latest comments (cf. above). Let me quickly summarize the status of the package. **LEGAL STUFF** According to the Legal mailing list, the license for Orthanc is "GPLv3 with exceptions": http://lists.fedoraproject.org/pipermail/legal/2012-December/002052.html This is fixed in the latest version of the package. **JSONCPP** The actual release of the Orthanc package depends on the packaging of JsonCpp: https://bugzilla.redhat.com/show_bug.cgi?id=882617 Even though the Orthanc package does work right now because it statically links against JsonCpp, it is probably a Good Thing to wait for JsonCpp to be officially packaged. **OTHER CHANGES** A few other changes have been spotted by several people and are fixed in the last version of the package. Here is a summary of these changes: * "%doc %{_mandir}/man1/*.1*" => "%{_mandir}/man1/*.1*" * "BuildRequires: Python" * "%attr" removed for "/etc/orthanc" and "/etc/orthanc/orthanc.json" Please let me know if I have forgotten to answer some concern. Regards, Sébastien-
FYI, the Koji build has succeeded: http://koji.fedoraproject.org/koji/taskinfo?taskID=4806182
Yes, but that's a build for F17 where "python" seems to be pulled in into the buildroot as a dependency: http://kojipkgs.fedoraproject.org//work/tasks/6183/4806183/root.log The F18 build also succeeds now with the added BuildRequires, however.
Hello everybody! During vacations I'll have limited connectivity, but this should not be a big problem because, as I understand, we will wait till the approval of jsoncpp, right? Michael, you didn't tell me if you want to take up the review because of the sponsorship problem, if this is the case, just tell me and I'll reset the flag. I wish you all a nice and peaceful Christmas time! Mario
Re: comment 19 No objections. :) I've only mentioned the process/guidelines because the "deal" between Peter and you had not been documented in this ticket. Btw, every other Fedora packager could reset the flags or reassign the ticket (see "Preferences > Permissions" of your bugzilla account). ;-)
To build with the new shared jsoncpp from bug 882617 -DUSE_DYNAMIC_JSONCPP:BOOL=ON \ can be added to the CMake flags.
For your interest, a new major version of Orthanc (0.5.0) was released some days ago. As the jsoncpp package is not finished yet, would it not be interesting to release the Orthanc package as published about 7 weeks ago, even it does not dynamically link against jsoncpp? Thanks for your feedback!
Thanks Sabastien and sorry for not answering you before: I somehow managed to miss the email with your last comment. I agree that it would indeed be nice to approve orthanc as soon as possible, but doing so will require to ask for an exception of the guidelines: http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries and I think it won't be granted, being jsoncpp under review right now. Anyway, it seems that the jsoncpp review is in good state. I'll inquire to see which is their timeframe and get back in touch!
Hi Sebastien, jsoncpp has just been approved. That means that we will be able to complete orthanc soon! Nevertheless, it seems that the authors of jsoncpp will not provide many information on future changes in the interface and that might cause breakages in orthanc in turn. Please see Michael's last comment for further information on this. I was wondering if you, as one of the main users of jcpp, are interested to help Sebastien Willmann to detect those changes. If this is the case, why don't you get in touch with him? Once jcpp hits the repositories, I'll complete the review! Best, Mario
Hi Mario, The approval of jsoncpp is a great new! I will of course notify Sebastien Willmann of any problem I can detect with the jsoncpp package. Unfortunately, as the upstream developer of Orthanc as well as one of the package maintainers of Orthanc inside Debian (and maybe Fedora?), I must admit that I will not humanly be able to complete continuous, in-depth review of the jsoncpp Fedora package... Please let me know if you notice any problem with the Orthanc package. Regards, Sébastien- PS: FYI, my goal would be to put the Orthanc package inside EPEL in order to bring Orthanc to RHEL-based and CentOS-based servers. Maybe could you give some advice on this point after completing the review?
Hi Sebastien, thanks for you mail. I think that just promptly communicating with jsoncpp maintainers will be sufficient. Also Andrey posted a nice link which helps a lot in detecting ABI changes (https://bugzilla.redhat.com/show_bug.cgi?id=882617#c18). I will help you with great pleasure to push Orthanc to EPEL and (as far as I can) CentOs too, once the package is approved. Let's wait a bit more to see if jscpp hits the repositories soon and then let's complete the review. Best, Mario
Hello Sebastien, with jsoncpp inside the repositories, it is time to continue the review. Do I review http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc-0.4.0-5.fc17.src.rpm or is there any new version? Thanks Mario
Spec URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc.spec SRPM URL: http://www.montefiore.ulg.ac.be/~jodogne/Orthanc/Fedora/orthanc-0.5.0-1.fc18.src.rpm Hello Mario, As jsoncpp is now available and as a new upstream version of Orthanc (0.5.0) has been released in the meantime, I have just uploaded a new version of the package that is ready for further review. Please note that the package is now suitable for Fedora 18 and dynamically links against jsoncpp. The Orthanc systemd service can be started through the following command: # sudo service orthanc start Best, Sébastien-
Thanks Sebastien, here it goes the formal review: Package Review ============== Key: [x] = Pass [!] = Fail [-] = Not applicable [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [-]: Package requires other packages for directories it uses. [x]: Package uses nothing in %doc for runtime. [x]: Package is not known to require ExcludeArch. [x]: Package complies to the Packaging Guidelines [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "GPL (v3 or later)", "Unknown or generated", "MIT/X11 (BSD like)", "zlib/libpng", "BSD (3 clause)", "BSD (2 clause)". 6 files have unknown license. Detailed output of licensecheck in /home/mario/fedora/888301-orthanc/licensecheck.txt ----> This was already addressed in comment 16 [x]: Package consistently uses macro is (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [-]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [x]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [-]: Large documentation must go in a -doc subpackage. Note: Documentation size is 51200 bytes in 4 files. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: %config files are marked noreplace or the reason is justified. [x]: Each %files section contains %defattr if rpm < 4.4 [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Fully versioned dependency in subpackages, if present. [x]: Spec file lacks Packager, Vendor, PreReq tags. [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: No %config files under /usr. [x]: Package do not use a name that already exist [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). ===== SHOULD items ===== Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [x]: Package functions as described. [x]: Latest version is packaged. [-]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [ ]: Packages should try to preserve timestamps of original installed files. [x]: Sources can be downloaded from URI in Source: tag [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: Dist tag is present. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Uses parallel make. [x]: SourceX tarball generation or download is documented. [x]: SourceX is a working URL. [x]: Spec use %global instead of %define. ===== EXTRA items ===== Generic: [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: orthanc-0.5.0-1.fc18.x86_64.rpm orthanc.x86_64: W: spelling-error Summary(en_US) healthcare -> health care, health-care, ethereal orthanc.x86_64: W: only-non-binary-in-usr-lib orthanc.x86_64: W: non-standard-uid /var/lib/orthanc/db-v3 orthanc orthanc.x86_64: W: non-standard-gid /var/lib/orthanc/db-v3 orthanc orthanc.x86_64: W: non-standard-uid /var/lib/orthanc orthanc orthanc.x86_64: W: non-standard-gid /var/lib/orthanc orthanc 1 packages and 0 specfiles checked; 0 errors, 6 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint orthanc orthanc.x86_64: W: spelling-error Summary(en_US) healthcare -> health care, health-care, ethereal orthanc.x86_64: W: only-non-binary-in-usr-lib orthanc.x86_64: W: non-standard-uid /var/lib/orthanc/db-v3 orthanc orthanc.x86_64: W: non-standard-gid /var/lib/orthanc/db-v3 orthanc orthanc.x86_64: W: non-standard-uid /var/lib/orthanc orthanc orthanc.x86_64: W: non-standard-gid /var/lib/orthanc orthanc 1 packages and 0 specfiles checked; 0 errors, 6 warnings. # echo 'rpmlint-done:' Requires -------- orthanc (rpmlib, GLIBC filtered): /bin/sh config(orthanc) libboost_filesystem-mt.so.1.50.0()(64bit) libboost_system-mt.so.1.50.0()(64bit) libboost_thread-mt.so.1.50.0()(64bit) libc.so.6()(64bit) libcurl.so.4()(64bit) libdcmdata.so.3.6()(64bit) libdcmnet.so.3.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_4.0.0)(64bit) libglog.so.0()(64bit) libjsoncpp.so.0()(64bit) libm.so.6()(64bit) libmongoose.so.3()(64bit) liboflog.so.3.6()(64bit) libofstd.so.3.6()(64bit) libpng15.so.15()(64bit) libpng15.so.15(PNG15_0)(64bit) libpthread.so.0()(64bit) libsqlite3.so.0()(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(CXXABI_1.3.1)(64bit) libuuid.so.1()(64bit) libuuid.so.1(UUID_1.0)(64bit) libz.so.1()(64bit) libz.so.1(ZLIB_1.2.0)(64bit) rtld(GNU_HASH) shadow-utils systemd Provides -------- orthanc: config(orthanc) orthanc orthanc(x86-64) MD5-sum check ------------- https://orthanc.googlecode.com/files/Orthanc-0.5.0.tar.gz : CHECKSUM(SHA256) this package : da0423c6f5dfea642c41722b594c6752f357818d348423e155921187941d0741 CHECKSUM(SHA256) upstream package : da0423c6f5dfea642c41722b594c6752f357818d348423e155921187941d0741 Generated by fedora-review 0.4.0 (660ce56) last change: 2013-01-29 Buildroot used: fedora-18-x86_64 Command line :/usr/bin/fedora-review -b 888301 I see no additional issues, so the package is APPROVED
@Peter: could you please sponsor Sebastian, so he can ask git access? @Sebastian: once sponsored, please do: * make a SCM admin request as detailed here: https://fedoraproject.org/wiki/Package_SCM_admin_requests * import the package into the repositories as detailed here: https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner * read about maintaining for EPEL here: http://fedoraproject.org/wiki/EPEL/FAQ#I.27m_a_Fedora_contributor_and_want_to_maintain_my_packages_in_EPEL.2C_too._What_do_I_have_to_do_and_what_do_you_expect_from_me.3F Congratulations for a beautiful software and a nicely crafted package!!! Feel free to contact me for any doubt/question Mario
(In reply to comment #30) > @Peter: could you please sponsor Sebastian, so he can ask git access? Sure! Unblocking FE-NEEDSPONSOR - I've just sponsored Sebastian.
New Package SCM Request ======================= Package Name: orthanc Short Description: RESTful DICOM server for healthcare and medical research Owners: sjodogne Branches: f18 InitialCC:
Git done (by process-git-requests).
Dear Mario and Peter, Thank you much for your help! The Orthanc package is now available in the repositories: http://koji.fedoraproject.org/koji/packageinfo?packageID=15874 I will now focus on EPEL. Cheers, Sébastien-
orthanc-0.5.0-1.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/orthanc-0.5.0-1.fc18
orthanc-0.5.0-1.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/orthanc-0.5.0-1.fc19
orthanc-0.5.0-1.fc19 has been pushed to the Fedora 19 testing repository.
orthanc-0.5.0-1.fc18 has been pushed to the Fedora 18 stable repository.
orthanc-0.5.0-1.fc19 has been pushed to the Fedora 19 stable repository.
Package Change Request ====================== Package Name: orthanc New Branches: el6 Owners: sjodogne mrceresa