Copr builds here:

https://copr.fedoraproject.org/coprs/ankursinha/neuroscience-research/package/nest/

Updated srpm with improvements:
https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest-2.8.0-2.gitdc21fdc.fc23.src.rpm

Spec: https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest.spec

New srpm: https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest-2.8.0-3.gitdc21fdc.fc23.src.rpm

Spec: https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest.spec

New srpm: https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest-2.8.0-4.gitdc21fdc.fc23.src.rpm

Spec: https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest.spec

Are you interested in a review swap? Maybe mdp (rhbz#1246790) or polyglot (rhbz#1197333) are something for you to look at.

Review swap maybe with mdp (bug #1246790) or polyglot (bug #1197333)?

Taken them up. These are the latest spec/srp links:

https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest.spec

https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest-2.8.0-5.gitdc21fdc.fc23.src.rpm

I'm using the packages with openmpi now, and they seem to work fine. Some work may be needed to make the package better, though :)

Thanks!
Ankur

For a first step, I found some general advices to your spec file.

MUST:
- Add full URL to Source0, it's not sufficient to mention in comment only.
  I guess you use the snapshot tarball of GitHub, then please follow the 
  official recommendation to do so. Alternatively, you have to specify in a
  comment how to create the tarball (with an included script in SRPM?).
  https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Hosting_Services
- Did you send the patches to upstream? Please make notes in comments or
  use the direct URLs.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Patch_Guidelines
  https://fedoraproject.org/wiki/Staying_close_to_upstream_projects#Why_push_changes_upstream.3F
- Some lines in %description seem to be too long. Please restructure this block.
  The same counts also for the subpackages analogously.
- Usage of %global is preferred over %define, please explain why this does not
  apply here.
  https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define
- The common subpackage does not contain the license file, please add it to
  satisfy all other subpackages with that license too through the specified 
  dependencies. The doc subpackage needs also the license file.
- Use the new %python_provide placeholder, the Provides are somehow wrong when
  we follow the new guidelines.
  https://fedoraproject.org/wiki/Packaging:Python#Provides
- The subpackages have to own also the individually created sub folders, 
  besides the files itself.
  https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_wholly_contained_in_your_package.2C_or_involves_core_functionality_of_your_package
- Specification of doc subpackage is not allowed as such.
  It's needed to list all folders and files individually, of course wildcards
  are possible. Remove %doc when you directly use installed pathes.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

SHOULD:
- You can use pushd/popd instead of cd commands to get the path changes logged.
  http://www.eriwen.com/bash/pushd-and-popd/
- Use "cp -p" to preserve timestamps.
- Please try harder to disable the check of automake, it's better with a 
  special patch (maybe applied in epel7 only but you do not need this build 
  conditional here at all), instead of this ugly sed command that is going to 
  break in future when the line numbers change.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Applying_patches
- You can use the new macros %make_build and %make_install that set default 
  parameters.
- Please rename the subpackages as suggested in the example template:
  python2-nest, python2-nest-openmpi and python2-nest-mpich
  https://fedoraproject.org/wiki/Packaging:Python#Example_common_spec_file
- Please ask upstream about future plans to support Python3.
  https://fedoraproject.org/wiki/Packaging:Python#Python_Version_Support

That's it for now. When the above hints are fixed, I can take a deeper look.

Heya,

So, upstream confirmed that nest supports py3, but they've also said that they intend to redo the build system in the future. If the build system is redone, I'll have to pretty much redo the package - do you think it makes sense to hold off the review until they've completed the change? (I'll keep up with upstream and maintain the COPR until then)

Cheers,
Ankur

> do you think it makes sense to hold off the review until they've completed the change?

I don't think that waiting for some hypothetical future upstream changes makes much sense. At most one of the issues in comment #8 touches the build system (the configure sed, and I don't think Raphael is right here, I think sed is as good as a patch, and less work), they are all about package naming and descriptions and other stuff which is pretty much orthogonal to the build system. F24 will be a very attractive release for neuroscience with various neew packages added, and I'd love to see nest on that list.

It isn't actually a hypothetical upstream change. The nest user mailing list has received clear emails that detail the move to NEST3, which they say is a complete overhaul of the code base - while maintaining compatibility, of course - but it is quite possible that the build system changes too. 

I'm happy to continue the review if another reviewer comes along, but I do think a re-review will be in order (even if unofficially) if the build system and change. 

Is the plan to get all the neuroscience packages into Fedora by F24, though - or can we have a "group copr" thing running and transition packages over as they pass review?

(In reply to Ankur Sinha (FranciscoD) from comment #11)
> I'm happy to continue the review if another reviewer comes along,
I don't understand this sentence. Raphael said that he can review the package in comment #8. If he cannot do it, I'm sure that somebody else (e.g. me) would step up.

> but I do
> think a re-review will be in order (even if unofficially) if the build
> system and change. 
Nah, the build system is not visible in the binary package. As long as it builds nobody cares (except the maintainer of course).

> Is the plan to get all the neuroscience packages into Fedora by F24, though
> - or can we have a "group copr" thing running and transition packages over
> as they pass review?

I don't think there's a plan to get "all" neuroscience packages into Fedora, there's probably too many to even consider that. https://fedoraproject.org/wiki/Changes/NeuroFedora doesn't have a specific list. New packages can be added at any time, and there's quite a bit of time before Fedora 24 release (2016-05-17). I don't know about any plans to have a group copr. I see coprs as a good mechanism to provide alternative versions of already packaged software, or often updated software, or things which are inappropriate for main Fedora for other reasons. I don't think nest or other science related software falls into any of those categories, and making a "detour" through copr would be mostly a waste of time.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> (In reply to Ankur Sinha (FranciscoD) from comment #11)
> > I'm happy to continue the review if another reviewer comes along,
> I don't understand this sentence. Raphael said that he can review the
> package in comment #8. If he cannot do it, I'm sure that somebody else (e.g.
> me) would step up.

Oh, he dropped the review after I asked his opinion about the build system change in comment 9 which I took as an indication that he agreed with me putting off the package (He isn't the assignee any more, and he isn't in the CC list either) :)

> 
> > but I do
> > think a re-review will be in order (even if unofficially) if the build
> > system and change. 
> Nah, the build system is not visible in the binary package. As long as it
> builds nobody cares (except the maintainer of course).

That is what I meant - when a new build system comes, the entire package has to be effectively redone, and nest, as you'll see from the spec, isn't the simplest of packages. Totally a maintainers burden, yes, but I'm going to be the maintainer ;)

> 
> > Is the plan to get all the neuroscience packages into Fedora by F24, though
> > - or can we have a "group copr" thing running and transition packages over
> > as they pass review?
> 
> I don't think there's a plan to get "all" neuroscience packages into Fedora,
> there's probably too many to even consider that.
> https://fedoraproject.org/wiki/Changes/NeuroFedora doesn't have a specific
> list. New packages can be added at any time, and there's quite a bit of time
> before Fedora 24 release (2016-05-17). I don't know about any plans to have
> a group copr. I see coprs as a good mechanism to provide alternative
> versions of already packaged software, or often updated software, or things
> which are inappropriate for main Fedora for other reasons. I don't think
> nest or other science related software falls into any of those categories,
> and making a "detour" through copr would be mostly a waste of time.

I think the list is somewhere on a google spreadsheet. I don't have the link handy at the moment. 

Coprs are also a good mechanism for packages that build and are functional but are not packaged well enough to pass a formal review yet (outer ring in fedora.next and all that), which is exactly what the nest package is at the moment.

Anyway, I'm happy to continue the review if someone takes it up. I'll update the spec with Raphael's comments later this week. I already updated the copr package to 2.10 which came out recently. I need to work on the python3 bit etc.

Cheers!
Ankur

(In reply to Ankur Sinha (FranciscoD) from comment #13)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> > (In reply to Ankur Sinha (FranciscoD) from comment #11)
> > > I'm happy to continue the review if another reviewer comes along,
> > I don't understand this sentence. Raphael said that he can review the
> > package in comment #8. If he cannot do it, I'm sure that somebody else (e.g.
> > me) would step up.
> 
> Oh, he dropped the review after I asked his opinion about the build system
> change in comment 9 which I took as an indication that he agreed with me
> putting off the package (He isn't the assignee any more, and he isn't in the
> CC list either) :)
Was he ever? I only see comment #8 from him.
Anyway, like I said, I can review the package.

> > > but I do
> > > think a re-review will be in order (even if unofficially) if the build
> > > system and change. 
> > Nah, the build system is not visible in the binary package. As long as it
> > builds nobody cares (except the maintainer of course).
> 
> That is what I meant - when a new build system comes, the entire package has
> to be effectively redone, and nest, as you'll see from the spec, isn't the
> simplest of packages. Totally a maintainers burden, yes, but I'm going to be
> the maintainer ;)
True. I doesn't seem so bad to me (about normal for an mpi package in fact :)),
but of course I can't and don't want to push you.

> > > Is the plan to get all the neuroscience packages into Fedora by F24, though
> > > - or can we have a "group copr" thing running and transition packages over
> > > as they pass review?
> > 
> > I don't think there's a plan to get "all" neuroscience packages into Fedora,
> > there's probably too many to even consider that.
> > https://fedoraproject.org/wiki/Changes/NeuroFedora doesn't have a specific
> > list. New packages can be added at any time, and there's quite a bit of time
> > before Fedora 24 release (2016-05-17). I don't know about any plans to have
> > a group copr. I see coprs as a good mechanism to provide alternative
> > versions of already packaged software, or often updated software, or things
> > which are inappropriate for main Fedora for other reasons. I don't think
> > nest or other science related software falls into any of those categories,
> > and making a "detour" through copr would be mostly a waste of time.
> 
> I think the list is somewhere on a google spreadsheet. I don't have the link
> handy at the moment.
I wasn't aware of that. I know there's
http://taiga.fedorainfracloud.org/project/ignatenkobrain-neurofedora/kanban
but probably not everything is on there.

> Coprs are also a good mechanism for packages that build and are functional
> but are not packaged well enough to pass a formal review yet (outer ring in
> fedora.next and all that), which is exactly what the nest package is at the
> moment.
In my experience, it's quite easy for coprs to get obsolete. If the package
is in Fedora proper, than at least it will be regularly rebuilt, sometimes
a provenpackager will do a drive-by fix of some small issue, and users can
report issues in bugzilla, crashes are colleted. I think coprs are great
for development, or for a small focuses group of users, but after using them
for a while, I'm starting to see downsides of using them as a mechanism for
distributing software to a diverse group of users, over long time.

> Anyway, I'm happy to continue the review if someone takes it up. I'll update
> the spec with Raphael's comments later this week. I already updated the copr
> package to 2.10 which came out recently. I need to work on the python3 bit
> etc.

Great! Let's see if Raphael wants to pick up the review. If not, I'll take it.

Pull request with cmake build system: https://github.com/nest/nest-simulator/pull/213

I suggest we hold off until this is merged?

So, I've started working on the cmake spec. I have a question though - I need to build it without mpi, with mpich, with openmpi, and for each of these 3, with python2 and python3 - so, can I just build the thing SIX times in the spec?? I've been trying to hack around and try to limit it to the minimum three times - without mpi, with mpich and with openmpi, but it's extremely hacky and quite frankly ugly. Thoughts?

I'm don't think you need to build 6 times. The python wrapper might be independent of mpi. Can you post your latest version of the spec file, I'll try to have a look. If the python module (.so files in %{python2_sitearch}/%{name}) aren't linked to libmpi, even if the package is built with mpi, then there's no need to provide separate python2-openmpi, python2-mpich, etc, subpackages.

This is the current version, which doesn't quite work:

https://ankursinha.fedorapeople.org/misc/nest.spec

They use cmake, which builds the c++ bits to build the SLI version. The cmake then calls python setup.py to build pynest and the other python requirements - connplotter, topology. For the python bit, they haven't separated building and installing, so I patched that part out and was doing it myself. The cython part works OK using the cmake build system itself. I don't think thee py parts link to MPI, but I'll have to check. If they don't, we can probably do:

- build serial with python2
- build serial with python3
- build mpich without python
- build openmpi without python

That'll simplify the build quite a bit.

Oh, by the way, this version gets the latest cmake version of the code from this pull request: https://github.com/nest/nest-simulator/pull/318

I've put up the srpm too: https://ankursinha.fedorapeople.org/misc/nest-2.10.0-22.git79b2f01.fc23.src.rpm

Python .so files have rpath set, you'll need to get rid of it [https://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath]. I haven't look here, but sometimes -DCMAKE_SKIP_RPATH:BOOL=ON works with cmake.

I had to modify the spec file to triple the backslashes in dobuild macro for all the cmake args (cmake \\\
                    -DCMAKE_C_FLAGS_RELEASE:STRING="-DNDEBUG" \\\
                    ...)
With that change it builds and installs and crashes in debuginfo generation.

I think you should change all ";" to "&&" in %dobuild, otherwise errors might go unnoticed.

(In reply to Ankur Sinha (FranciscoD) from comment #18)
> - build serial with python2
> - build serial with python3
> - build mpich without python
> - build openmpi without python

Yeah, that would be the best option. I'm still not sure if that's possible.
./usr/lib64/python{2.7,3.4}/site-packages/nest/pynestkernel.so
links to ./usr/lib64/libnestkernel.so. If the MPI version is built,
it might be possible to link the same pynestkernel.so to
/usr/lib64/openmpi/lib/libnestkernel.so if it has the same symbols.

But I think the spec file looks 80-90% done...

The python subpackages should be called python2-nest and python3-nest
(they can have Provides: pynest). This seems more in line with modern
practice, and the guidelines [https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28python_modules.29].

%python_provide should be used [https://fedoraproject.org/wiki/Packaging:Python#The_.25python_provide_macro, https://fedoraproject.org/wiki/Packaging:Python#Example_common_spec_file has an actual example].

Some more recommendations:
- I wouldn't bother with a seperate -connplotter subpackage. People
  are more likely to install and use it with the pynest anyway, and it
  would simplify packaging a bit.

- Do you really need openmpi-common and openmpi-doc? If they contain
  the same stuff as common and doc, you should use them instead.
  A symlink in the right place might be necessary.

- define a %global _description and reuse it in all %descriptions.
  Though -common and -doc probably don't need a full description.

- python2 and python3 subpackages must Require an exact version of the
  main package, because you don't want the user to mix pynestkernel.so
  with some different version of libnestkernel.so.

- openmpi or mpich python subpackages should not have Provides: python2-nest,
  because they are not interchangeable. (If some other package has a dependency
  on python2-nest, it'll break if python2-nest-openmpi is installed instead.)

OK, now all the versions build, and I think they're putting files in the right place too. We're ready for review!

https://ankursinha.fedorapeople.org/nest/nest.spec
https://ankursinha.fedorapeople.org/nest/nest-2.14.0-2.fc28.src.rpm

Cheers!

> # They run their tests per commit, so we don't need to run them here to check
> # it all again.
That's almost certainly wrong. Upstream does not test the same combination of architectures, system libraries, compiler, compilation flags, and local patches that we do. Testing at build time is not *required*, but is usually a good idea.

> BuildRequires:  ncurses-devel gsl-devel readline-devel
> BuildRequires:  python2-devel Cython
> BuildRequires:  python3-devel python3-Cython
One per line please!

> %global _description    NEST is a simulator for spiking neural network models that \
> focuses on the dynamics, size and structure of neural systems rather than on \
You can instead do:
> %global _description \
> NEST is a ...

The advantage is that the first line is not shorter than the later ones.

> %setup -q -n %{name}-simulator-%{version}
> %patch0 -p1
> %patch1 -p1
→ %autosetup -n %{name}-simulator-%{version} -p1

> - Do not make mpi packages noarch, since MPI_HOME is arch dependent---check
"---check" left over.

Pfff, that's a beast of a package. It's still building here, I might have some more comments after fedora-review is done. So far nothing major.

So luckily there's no conflict over /usr/bin/nest. Nestopia uses /usr/bin/nestopia and nested uses /usr/bin/nested. Similarly for /usr/bin/sli, there are a few similar names, but nothing exactly the same.

+ package name is correct
+ license is acceptable for Fedora (GPLv2+)
+ license is specified correctly
+ builds and installs OK
+ Provides/Requires/BuildRequires appear to be correct
  Please change "Cython" to "python2-Cython"

+ latest version

%define is used, either replace by %global or add an explanatory comment.

> %global do_make_build \
> pushd %{name}-simulator-%{version}$MPI_COMPILE_TYPE  && \
>     make %{?_smp_mflags} \
> popd;
This can be written as simply
> make %{?_smp_mflags} -C %{name}-simulator-%{version}$MPI_COMPILE_TYPE

Hi!

I've made the required changes. It builds, and I've run a scratch build for F28 and tested the packages too. I've also added a README.Fedora file to explain how the package should be used. I'm still working on getting the tests to run, but haven't managed them all just yet.

Updated spec/srpm:

https://ankursinha.fedorapeople.org/nest/nest.spec
https://ankursinha.fedorapeople.org/nest/nest-2.14.0-4.fc28.src.rpm

I was wondering if we need to support all architectures for this particular simulator. It's usually run either on workstations or clusters, but I wouldn't think people would run it on ARM, for instance. Should we exclude some arches?

Cheers!
Ankur

F28 scratch build:

https://koji.fedoraproject.org/koji/taskinfo?taskID=28651669

So I went on a longish roundabout hike after looking an the Provides and ended up filing #1618949 and #1618951. It seems that this package is not at fault, but the automatic dependency generator is too eager in one case and not eager enough in another.

> Should we exclude some arches?

If you *have* to, for example because they aren't supported upstream or tests fail or whatever, then sure. But otherwise, everything should be compiled for all architectures. You're right that arm probably isn't an option, but somebody might be running small stuff on a chromebook. Arm64 is also getting more viable.

Looks all good to me. I gave it some light testing, and everything seems to work fine. I already listed license and other checks above in #c23.
Package is APPROVED.

Thanks for the review!

I'll leave all arches enabled then. It'll take a little longer to build, but that's fine. 

I've noticed that libneurosim is an optional dependency that NEST needs to be built with to get PyNN to work with it properly. I'll package that too and rebuild NEST when that's been approved.

Thanks again, I'll import the package later today.

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/nest

nest-2.14.0-4.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-b95878833b

nest-2.14.0-4.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-2018-b95878833b

nest-2.14.0-4.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.