Bug 1662526 (neuron) - Review Request: neuron - A flexible and powerful simulator of neurons and networks
Summary: Review Request: neuron - A flexible and powerful simulator of neurons and net...
Status: CLOSED ERRATA
Alias: neuron
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dominik 'Rathann' Mierzejewski
QA Contact: Fedora Extras Quality Assurance
neuro-sig
URL:
Whiteboard:
Keywords:
: 1249094 (view as bug list)
Depends On:
Blocks: python-pynn fedora-neuro
TreeView+ depends on / blocked
 
Reported: 2018-12-29 10:29 UTC by Ankur Sinha (FranciscoD)
Modified: 2019-05-15 10:38 UTC (History)
2 users (show)

(edit)
Added to list of tools:

https://pagure.io/neuro-sig/documentation/c/c3829b5405a40490944d5b6fbfb016f6d04a3157?branch=master
Clone Of:
(edit)
Last Closed: 2019-02-27 01:15:40 UTC
dominik: fedora-review+


Attachments (Terms of Use)

Description Ankur Sinha (FranciscoD) 2018-12-29 10:29:58 UTC
Spec URL: https://ankursinha.fedorapeople.org/neuron/neuron.spec
SRPM URL: https://ankursinha.fedorapeople.org/neuron/neuron-7.5-1.20181214git5687519.fc29.src.rpm

Description: 
NEURON is a simulation environment for modeling individual neurons and networks
of neurons. It provides tools for conveniently building, managing, and using
models in a way that is numerically sound and computationally efficient. It is
particularly well-suited to problems that are closely linked to experimental
data, especially those that involve cells with complex anatomical and
biophysical properties.

This package is currently built without GUI (iv) support.
It does not include MPI support.



Fedora Account System Username: ankursinha

Comment 1 Ankur Sinha (FranciscoD) 2019-01-02 11:52:44 UTC
*** Bug 1249094 has been marked as a duplicate of this bug. ***

Comment 2 Dominik 'Rathann' Mierzejewski 2019-01-05 22:53:14 UTC
# Let us tell the system where to find the sundials libraries. It is hard coded.
sed -i 's|SUNDIALS_LIBDIRNAMES="${prefix}/lib"|SUNDIALS_LIBDIRNAMES="$MPI_LIB"|' configure.ac

You're doing the above but not module-loading any MPI implementation configuration, nor is any MPI library added to build dependencies.

Could you sort the BuildRequires and put each one on a separate line?

I would use 'rm' to delete Random123 files instead of doing that with a patch. It'd make the patch much smaller.

Comment 3 Ankur Sinha (FranciscoD) 2019-01-06 16:05:00 UTC
Hi Dominik,

Thanks for taking up the review. I've updated the spec/srpm:

* Sun Jan 06 2019 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 7.5-2.20181214git5687519
- Put each BR on different line
- Remove unneeded comment
- Re-do random123 patch to only modify autotools files
- Remove random123 in prep


https://ankursinha.fedorapeople.org/neuron/neuron.spec
https://ankursinha.fedorapeople.org/neuron/neuron-7.5-2.20181214git5687519.fc29.src.rpm


This is a serial (non MPI) build of NEURON. The MPI builds, and python bindings require NEURON to be installed already. Upstream builds them as post-installation hooks. So, I'll package them up as different packages that will require this one as a BR. I've added this as a comment at the start of the spec.

Cheers,
Ankur

Comment 4 Ankur Sinha (FranciscoD) 2019-01-20 16:43:53 UTC
Hi Dominik,

Thanks for taking up the review. Would you have time to complete this review sometime this week please?

Cheers,
Ankur

Comment 5 Dominik 'Rathann' Mierzejewski 2019-01-23 10:02:36 UTC
Hi, Ankur.
The new version looks better.

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.

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

3. The header file in -devel subpackage:
# should this be here?!
%{_libdir}/nrnconf.h

It shouldn't. Please put it in %{_includedir} .

4. It looks like there's more bundled code:
src/gnu - libstdc++-devel (?)
src/readline - readline-devel

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.

6. README.md isn't very useful for the installed package as it talks only
   about installing from source.

Comment 6 Ankur Sinha (FranciscoD) 2019-01-27 19:09:54 UTC
(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

Comment 7 Dominik 'Rathann' Mierzejewski 2019-01-31 12:52:12 UTC
(In reply to Ankur Sinha (FranciscoD) from comment #6)
> (In reply to Dominik 'Rathann' Mierzejewski from comment #5)
[...]
> > 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).

Alright. Could you open a ticket with upstream about this, too? It would be
better if MPI variants could be built in the same build process.

[...]
> > 3. The header file in -devel subpackage:
> > # should this be here?!
> > %{_libdir}/nrnconf.h
> > 
> > It shouldn't. Please put it in %{_includedir} .
> 
> Moved.

You seem to have left the comment which is no longer relevant.

> > 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.

OK, thanks for opening the upstream ticket.

[...]
> > 6. README.md isn't very useful for the installed package as it talks only
> >    about installing from source.
> 
> Removed.

Good, but the changelog entry doesn't explain why you did it, which might be useful.

> Updated spec/srpm:
> https://ankursinha.fedorapeople.org/neuron/neuron.spec

# Do we need the static libraries?
%files static
%license Copyright
%doc README.md
%{_libdir}/libivoc.la
%{_libdir}/libmemacs.la
%{_libdir}/libmeschach.la
%{_libdir}/libneuron_gnu.la
%{_libdir}/libnrniv.la
%{_libdir}/libnrnmpi.la
%{_libdir}/libnrnoc.la
%{_libdir}/liboc.la
%{_libdir}/libocxt.la
%{_libdir}/libsparse13.la
%{_libdir}/libscopmath.la
%{_libdir}/libivos.la

If it's just .la (libtool archive), then these are not enough, you should have
.a files as well. So either ship those, too or don't ship .la files at all
unless they're dlopened by neuron consumers.

Comment 8 Ankur Sinha (FranciscoD) 2019-01-31 21:14:53 UTC
Updated spec/srpm:

https://ankursinha.fedorapeople.org/neuron/neuron.spec
https://ankursinha.fedorapeople.org/neuron/neuron-7.5-4.20181214git5687519.fc29.src.rpm

* Thu Jan 31 2019 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 7.5-4.20181214git5687519
- Remove libtool archives
- Remove stray comment
- Improve previous changelog


I'm already in conversation with upstream about multiple issues (arch support/sundials/libstdc++) so I'll bring up the build issue there too. I don't want to pepper them with too many issues all at once.

Cheers,
Ankur

Comment 9 Dominik 'Rathann' Mierzejewski 2019-02-09 01:04:41 UTC
Wouldn't it make sense to move
%{_includedir}/nrnconf.h
into
%{_includedir}/%{tarname}

I don't know where packages that will BuildRequires: neuron-devel expect these files to be, so I'll leave this up to you to decide.

Also, I wouldn't define a macro that is longer than what it expands into, i.e. I'd just use "nrn" everywhere instead of %{tarname}.

None of the above are blockers and the package looks good otherwise, so approved.

Comment 10 Ankur Sinha (FranciscoD) 2019-02-16 13:15:02 UTC
(In reply to Dominik 'Rathann' Mierzejewski from comment #9)
> Wouldn't it make sense to move
> %{_includedir}/nrnconf.h
> into
> %{_includedir}/%{tarname}
> 
> I don't know where packages that will BuildRequires: neuron-devel expect
> these files to be, so I'll leave this up to you to decide.

I've not seen too many refer to that header so I'll leave it for the time being and change it later if required.

> 
> Also, I wouldn't define a macro that is longer than what it expands into,
> i.e. I'd just use "nrn" everywhere instead of %{tarname}.

Ah, yes XD

> 
> None of the above are blockers and the package looks good otherwise, so
> approved.

Thanks very much! SCM requested: https://pagure.io/releng/fedora-scm-requests/issue/9737

Cheers,
Ankur

Comment 11 Igor Gnatenko 2019-02-16 16:59:06 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/neuron

Comment 12 Fedora Update System 2019-02-16 17:23:28 UTC
neuron-7.5-4.20181214git5687519.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2019-aef28e1e3d

Comment 13 Fedora Update System 2019-02-16 17:23:31 UTC
neuron-7.5-4.20181214git5687519.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-214c9d5a07

Comment 14 Fedora Update System 2019-02-17 01:56:19 UTC
neuron-7.5-4.20181214git5687519.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-214c9d5a07

Comment 15 Fedora Update System 2019-02-17 03:11:19 UTC
neuron-7.5-4.20181214git5687519.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-aef28e1e3d

Comment 16 Fedora Update System 2019-02-27 01:15:40 UTC
neuron-7.5-4.20181214git5687519.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2019-02-27 03:28:27 UTC
neuron-7.5-4.20181214git5687519.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.

Comment 18 Christophe Fergeau 2019-04-04 08:00:30 UTC
Fwiw, the /usr/bin/oc file this package installs conflicts with a binary from origin-clients, I've filed rhbz#1696118 for that.

Comment 19 Dominik 'Rathann' Mierzejewski 2019-05-15 10:38:00 UTC
(In reply to Christophe Fergeau from comment #18)
> Fwiw, the /usr/bin/oc file this package installs conflicts with a binary
> from origin-clients, I've filed rhbz#1696118 for that.

Indeed, sorry for missing that in review. I've just hit it myself by accident.


Note You need to log in before you can comment on or make changes to this bug.