https://bugzilla.redhat.com/show_bug.cgi?id=1662526 --- Comment #6 from Ankur Sinha (FranciscoD) <sanjay.ankur@xxxxxxxxx> --- (In reply to Dominik 'Rathann' Mierzejewski from comment #5) > Hi, Ankur. > The new version looks better. Hi Dominik, Thanks for the review. > 1. I read your comment at the top of the spec: > > # This is a serial build of NEURON without Python or other bindings. > > # Both the MPI builds and Python bindings require NEURON to be already > > # installed in the system---they are build as post-installation hooks. So, we > > # first package a serial version of NEURON and then package those separately > > # after using this package as a BR > > Are you sure this is really the case? Looking at src/nrnmpi/Makefile.am, > it seems that specifying the path to the just-built libraries should suffice > to build it after the serial version is built. I did try that, but I wasn't able to get it to work. nrnmpi depends on other bits that are not in subdirs of the nrnmpi directory. So, the dep chain isn't set up correctly. From what I could find, there's a way to force the dependent targets that are not in subdirs. (The other alternative is to drop the subdir method of using Autotools and re-do the project.) So, it seems that this why upstream build nrnmpi in the post-install hook (and not post-build). > > 2. I noticed that many source files under src/ have executable permission > bits. I'd suggest something like this in %prep: > > find src -type f -executable ! -name "*.sh" | xargs chmod -x Done. > > 3. The header file in -devel subpackage: > # should this be here?! > %{_libdir}/nrnconf.h > > It shouldn't. Please put it in %{_includedir} . Moved. > > 4. It looks like there's more bundled code: > src/gnu - libstdc++-devel (?) > src/readline - readline-devel Unfortunately it seems to be a version from 1988(!), and libstdc++ has changed so much since then that I cannot even find the bundled headers. I've filed a ticket upstream. For the time being, though, I've included it. Upstream changed the soname, so it won't conflict with the upstream version we provide in Fedora. > 5. You might want to change the License: tag to GPLv3, because two files > are GPLv3: > ./src/ivoc/nrngsl_hc_radix2.c: GPL (v3 or later) > ./src/ivoc/nrngsl_real_radix2.c: GPL (v3 or later) > > Do check if they're compiled and included in the binaries. Done. They are both included in src/ivoc/fourier.cpp. > 6. README.md isn't very useful for the installed package as it talks only > about installing from source. Removed. Updated spec/srpm: https://ankursinha.fedorapeople.org/neuron/neuron.spec https://ankursinha.fedorapeople.org/neuron/neuron-7.5-3.20181214git5687519.fc29.src.rpm Cheers, Ankur -- 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 Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
