Spec URL: https://ankursinha.fedorapeople.org/arbor/arbor.spec SRPM URL: https://ankursinha.fedorapeople.org/arbor/arbor-0.2.2-1.fc31.src.rpm Description: Arbor is a high-performance library for computational neuroscience simulations. Some key features include: - Asynchronous spike exchange that overlaps compute and communication. - Efficient sampling of voltage and current on all back ends. - Efficient implementation of all features on GPU. - Reporting of memory and energy consumption (when available on platform). - An API for addition of new cell types, e.g. LIF and Poisson spike generators. - Validation tests against numeric/analytic models and NEURON. Documentation is available at https://arbor.readthedocs.io/en/latest/ Fedora Account System Username: ankursinha
I can take this review. Can you take bug 1780941 in exchange?
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated Issues: ======= - Static libraries in -static or -devel subpackage, providing -devel if present. Note: Package has .a files: arbor-devel, arbor-mpich-devel, arbor- openmpi-devel. Does not provide -static: arbor-devel, arbor-mpich-devel, arbor-openmpi-devel. See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#packaging-static-libraries That is an automatically generated issue. I think it is complaining that the 3 devel packages provide arbor{,-mpich,-openmpi}-static(%{arch}), but not arbor{,-mpich,-openmpi}-static (i.e., without the "(%{arch})" part). - Thank you for filing an upstream bug about static vs. shared libraries. I wonder if we shouldn't build shared libraries for Fedora anyway, even though upstream doesn't support them. The issue is that once this package is introduced, other packages will begin to depend on it by linking the static libraries into their executables or libraries. That will require doing some transitioning in the future to move to shared libraries. It might be better to pay the cost now instead of later. What are your thoughts? - The test suite failed, but %check did not. Please arrange to have %check fail the build if the tests do not pass. - The spec file contains: BuildRequires: python-devel Please change that to python3-devel. See https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_dependencies - Please consider building the sphinx documentation. That will require that you unbundle python3-sphinx_rtd_theme, however. - Not all directories created by these packages are owned. In particular, these directories are not owned by the arbor packages, nor by the openmpi or mpich packages: %{_libdir}/openmpi/lib/cmake, %{_libdir}/mpich/lib/cmake, %{_libdir}/openmpi/include, %{_libdir}/mpich/include, %{python3_sitearch}/mpich, %{python3_sitearch}/openmpi. Those last two look like they SHOULD be owned by python3-mpi4py-mpich and python3-mpi4py-openmpi, but they are NOT. That is probably a bug in those 2 packages. - Can you do something about rpmlint's summary-not-capitalized warnings? For example, "Arbor built with mpich" or "Mpich build of arbor" or "Multi-compartment neural network simulation library (mpich build)"? - This is not an issue. I just have to note that I laughed out loud when I saw the spell checker suggest this: neuroscience -> pseudoscience. mpich -> chimp is pretty funny, too. ===== MUST items ===== C/C++: [-]: Provides: bundled(gnulib) in place as required. [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Development (unversioned) .so files in -devel subpackage, if present. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [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]: License field in the package spec file matches the actual license. [x]: License file installed when any subpackage combination is installed. [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib64/openmpi/lib/cmake, /usr/lib64/mpich/lib/cmake, /usr/lib64/openmpi/include, /usr/lib64/mpich/include, /usr/lib64/python3.8/site-packages/mpich, /usr/lib64/python3.8/site-packages/openmpi [x]: Package does not own files or directories owned by other packages. [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. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (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. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 30720 bytes in 3 files. [!]: Package complies to the Packaging Guidelines [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). [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 %license. [x]: Package requires other packages for directories it uses. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [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 ===== 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]: Fully versioned dependency in subpackages if applicable. [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [x]: 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]: 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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: arbor-0.2.2-1.fc32.x86_64.rpm arbor-devel-0.2.2-1.fc32.x86_64.rpm arbor-mpich-0.2.2-1.fc32.x86_64.rpm arbor-mpich-devel-0.2.2-1.fc32.x86_64.rpm arbor-openmpi-0.2.2-1.fc32.x86_64.rpm arbor-openmpi-devel-0.2.2-1.fc32.x86_64.rpm arbor-debuginfo-0.2.2-1.fc32.x86_64.rpm arbor-debugsource-0.2.2-1.fc32.x86_64.rpm arbor-0.2.2-1.fc32.src.rpm arbor.x86_64: W: spelling-error Summary(en_US) Multi -> Mulch, Mufti arbor.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor.x86_64: W: no-manual-page-for-binary modcc arbor-devel.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor-devel.x86_64: W: no-documentation arbor-mpich.x86_64: W: summary-not-capitalized C arbor built with mpich arbor-mpich.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor-mpich-devel.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor-mpich-devel.x86_64: W: no-documentation arbor-openmpi.x86_64: W: summary-not-capitalized C arbor built with openmpi arbor-openmpi.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor-openmpi-devel.x86_64: W: spelling-error Summary(en_US) mpich -> chimp arbor-openmpi-devel.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor-openmpi-devel.x86_64: W: no-documentation arbor.src: W: spelling-error Summary(en_US) Multi -> Mulch, Mufti arbor.src: W: spelling-error %description -l en_US neuroscience -> pseudoscience 9 packages and 0 specfiles checked; 0 errors, 16 warnings. Rpmlint (debuginfo) ------------------- Checking: arbor-debuginfo-0.2.2-1.fc32.x86_64.rpm arbor-openmpi-debuginfo-0.2.2-1.fc32.x86_64.rpm arbor-mpich-debuginfo-0.2.2-1.fc32.x86_64.rpm 3 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- arbor.x86_64: W: spelling-error Summary(en_US) Multi -> Mulch, Mufti arbor.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor.x86_64: W: no-manual-page-for-binary modcc arbor-devel.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor-devel.x86_64: W: no-documentation arbor-mpich.x86_64: W: summary-not-capitalized C arbor built with mpich arbor-mpich.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor-mpich-devel.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor-mpich-devel.x86_64: W: no-documentation arbor-openmpi.x86_64: W: summary-not-capitalized C arbor built with openmpi arbor-openmpi.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor-openmpi-devel.x86_64: W: spelling-error Summary(en_US) mpich -> chimp arbor-openmpi-devel.x86_64: W: spelling-error %description -l en_US neuroscience -> pseudoscience arbor-openmpi-devel.x86_64: W: no-documentation 10 packages and 0 specfiles checked; 0 errors, 14 warnings. Unversioned so-files -------------------- arbor: /usr/lib64/python3.8/site-packages/arbor.cpython-38-x86_64-linux-gnu.so arbor-mpich: /usr/lib64/python3.8/site-packages/mpich/arbor.cpython-38-x86_64-linux-gnu.so arbor-openmpi: /usr/lib64/python3.8/site-packages/openmpi/arbor.cpython-38-x86_64-linux-gnu.so Source checksums ---------------- https://github.com/arbor-sim/arbor/archive/v0.2.2/arbor-0.2.2.tar.gz : CHECKSUM(SHA256) this package : 5a7845f3b33ec27e326d58e1d16e5d8ad0f5aaae113d2a4a2b639e101073dab6 CHECKSUM(SHA256) upstream package : 5a7845f3b33ec27e326d58e1d16e5d8ad0f5aaae113d2a4a2b639e101073dab6 Requires -------- arbor (rpmlib, GLIBC filtered): ld-linux-x86-64.so.2()(64bit) libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libm.so.6()(64bit) libpthread.so.0()(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(CXXABI_1.3.11)(64bit) libstdc++.so.6(CXXABI_1.3.2)(64bit) libstdc++.so.6(CXXABI_1.3.3)(64bit) libstdc++.so.6(CXXABI_1.3.5)(64bit) libstdc++.so.6(CXXABI_1.3.7)(64bit) libstdc++.so.6(CXXABI_1.3.9)(64bit) python(abi) rtld(GNU_HASH) arbor-devel (rpmlib, GLIBC filtered): arbor(x86-64) cmake-filesystem(x86-64) arbor-mpich (rpmlib, GLIBC filtered): ld-linux-x86-64.so.2()(64bit) libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libm.so.6()(64bit) libmpi.so.12()(64bit)(mpich-x86_64) libpthread.so.0()(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(CXXABI_1.3.11)(64bit) libstdc++.so.6(CXXABI_1.3.2)(64bit) libstdc++.so.6(CXXABI_1.3.3)(64bit) libstdc++.so.6(CXXABI_1.3.5)(64bit) libstdc++.so.6(CXXABI_1.3.7)(64bit) libstdc++.so.6(CXXABI_1.3.9)(64bit) mpich python(abi) python3-mpi4py-mpich rtld(GNU_HASH) arbor-mpich-devel (rpmlib, GLIBC filtered): arbor-mpich(x86-64) arbor-openmpi (rpmlib, GLIBC filtered): ld-linux-x86-64.so.2()(64bit) libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libm.so.6()(64bit) libmpi.so.40()(64bit)(openmpi-x86_64) libpthread.so.0()(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(CXXABI_1.3.11)(64bit) libstdc++.so.6(CXXABI_1.3.2)(64bit) libstdc++.so.6(CXXABI_1.3.3)(64bit) libstdc++.so.6(CXXABI_1.3.5)(64bit) libstdc++.so.6(CXXABI_1.3.7)(64bit) libstdc++.so.6(CXXABI_1.3.9)(64bit) openmpi python(abi) python3-mpi4py-openmpi rtld(GNU_HASH) arbor-openmpi-devel (rpmlib, GLIBC filtered): arbor-openmpi(x86-64) arbor-debuginfo (rpmlib, GLIBC filtered): arbor-debugsource (rpmlib, GLIBC filtered): Provides -------- arbor: arbor arbor(x86-64) arbor-devel: arbor-devel arbor-devel(x86-64) arbor-static(x86-64) cmake(arbor) arbor-mpich: arbor-mpich arbor-mpich(x86-64) arbor-mpich-devel: arbor-mpich-devel arbor-mpich-devel(x86-64) arbor-mpich-static(x86-64) arbor-openmpi: arbor-openmpi arbor-openmpi(x86-64) arbor-openmpi-devel: arbor-openmpi-devel arbor-openmpi-devel(x86-64) arbor-openmpi-static(x86-64) arbor-debuginfo: arbor-debuginfo arbor-debuginfo(x86-64) debuginfo(build-id) arbor-debugsource: arbor-debugsource arbor-debugsource(x86-64) Generated by fedora-review 0.7.3 (44b83c7) last change: 2019-09-18 Command line :/usr/bin/fedora-review -b 1780906 -m fedora-rawhide-x86_64 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: Haskell, Java, R, Python, fonts, SugarActivity, Perl, Ocaml, PHP, Ruby Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Thanks for the review Jerry. I've made the required fixes: (In reply to Jerry James from comment #2) > Package Review > ============== > > Legend: > [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated > > Issues: > ======= > - Static libraries in -static or -devel subpackage, providing -devel if > present. > Note: Package has .a files: arbor-devel, arbor-mpich-devel, arbor- > openmpi-devel. Does not provide -static: arbor-devel, arbor-mpich-devel, > arbor-openmpi-devel. > See: https://docs.fedoraproject.org/en-US/packaging- > guidelines/#packaging-static-libraries > > That is an automatically generated issue. I think it is complaining that > the > 3 devel packages provide arbor{,-mpich,-openmpi}-static(%{arch}), but not > arbor{,-mpich,-openmpi}-static (i.e., without the "(%{arch})" part). I thought the arch specific Provides would be better, but I've removed that bit to please rpmlint. > > - Thank you for filing an upstream bug about static vs. shared libraries. I > wonder if we shouldn't build shared libraries for Fedora anyway, even > though > upstream doesn't support them. The issue is that once this package is > introduced, other packages will begin to depend on it by linking the static > libraries into their executables or libraries. That will require doing > some > transitioning in the future to move to shared libraries. It might be > better > to pay the cost now instead of later. What are your thoughts? Well, this isn't a general purpose library so we won't have tools linking to it as they generally do. Only users that run simulations using Arbor will link against it, and it is extremely unlikely that such simulation code will ever be packaged in Fedora. Since this package is related to science and correctness is paramount, I'm wary of providing any features that upstream does not support. I'm hoping upstream will start supporting shared libraries, and if/when they do, I can make them available. What do you think? > > - The test suite failed, but %check did not. Please arrange to have %check > fail the build if the tests do not pass. I've done this, but now I've had to disable tests completely to get the build to pass. Until upstream fixes the issue we've reported, we can't enable them. > > - The spec file contains: > > BuildRequires: python-devel > > Please change that to python3-devel. See > > https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ > #_dependencies Done. > > - Please consider building the sphinx documentation. That will require that > you unbundle python3-sphinx_rtd_theme, however. Done. > > - Not all directories created by these packages are owned. In particular, > these directories are not owned by the arbor packages, nor by the openmpi > or mpich packages: %{_libdir}/openmpi/lib/cmake, > %{_libdir}/mpich/lib/cmake, > %{_libdir}/openmpi/include, %{_libdir}/mpich/include, > %{python3_sitearch}/mpich, %{python3_sitearch}/openmpi. > > Those last two look like they SHOULD be owned by python3-mpi4py-mpich and > python3-mpi4py-openmpi, but they are NOT. That is probably a bug in those > 2 > packages. They were owned by python3-{mpich,openmpi} so I've added those as Requires where required. > > - Can you do something about rpmlint's summary-not-capitalized warnings? For > example, "Arbor built with mpich" or "Mpich build of arbor" or > "Multi-compartment neural network simulation library (mpich build)"? Fixed. > > - This is not an issue. I just have to note that I laughed out loud when I > saw > the spell checker suggest this: neuroscience -> pseudoscience. > mpich -> chimp is pretty funny, too. > XD I know! Looks like the "enchant" library that rpmlint uses for spell checks does not include these terms. Updated spec/srpm: Spec: https://ankursinha.fedorapeople.org/arbor/arbor.spec SRPM: https://ankursinha.fedorapeople.org/arbor/arbor-0.2.2-2.fc32.src.rpm Changelog: * Mon Dec 09 2019 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 0.2.2-2 - Remove arch info in provides for static - Temporarily disable tests - use python3-devel - Add documentation in separate sub-package - add python3-{mpich,openmpi} as requires that own MPI_PYTHON3_SITEARCH directories - Improve summaries for sub-packages to please rpmlint
(In reply to Ankur Sinha (FranciscoD) from comment #3) > I thought the arch specific Provides would be better, but I've removed that > bit to please rpmlint. If it concerns you, you could include both. > Well, this isn't a general purpose library so we won't have tools linking to > it as they generally do. Only users that run simulations using Arbor will > link against it, and it is extremely unlikely that such simulation code will > ever be packaged in Fedora. Since this package is related to science and > correctness is paramount, I'm wary of providing any features that upstream > does not support. I'm hoping upstream will start supporting shared > libraries, and if/when they do, I can make them available. What do you think? Ah, okay, I did not understand how the libraries would be consumed. In that case, please stick with what upstream supports. > I've done this, but now I've had to disable tests completely to get the > build to pass. Until upstream fixes the issue we've reported, we can't > enable them. Yes, or you could disable just the failing test. Or patch time_event_span as_time_event_span to replace "&v[0]" with "v->data()" as upstream suggested. In any case, the spec file now meets all of the MUST requirements, so this package is APPROVED.
Thanks, I'm carrying the patch upstream has suggested. So the tests are enabled and they all pass without issue: https://kojipkgs.fedoraproject.org//work/tasks/3829/39473829/build.log https://koji.fedoraproject.org/koji/taskinfo?taskID=39473829 Updated spec/srpm: Spec: https://ankursinha.fedorapeople.org/arbor/arbor.spec SRPM: https://ankursinha.fedorapeople.org/arbor/arbor-0.2.2-3.fc32.src.rpm I've requested SCM now. Cheers, Ankur
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/arbor
Built for rawhide now.