The link is broken.

Also, could you update this to 1.7? I might be willing to review this since psi4 can link to CheMPS2.

Hi Susi,

Here's the srpm on copr. Ignore the el5/6 builds please. We will try to figure out what happened. It's not immediately obvious to me but there's something going with the old versions of cmake/hdf5?

https://copr.fedorainfracloud.org/coprs/talcite/CheMPS2/build/341404/

Thanks!
Matt

Next time, please post separate links to the spec and the srpm, so that the reviewer doesn't have to download the potentially huge srpm and decompress it to have a look at the spec file.

Please clean up the spec file from commented out junk. E.g.

#BuildRequires:  atlas-devel atlas texlive-braket cmake hdf5-devel coreutils gsl gsl-devel doxygen python python-devel numpy Cython
BuildRequires:  atlas-devel atlas cmake hdf5-devel python python-devel numpy Cython

and

#python build

Also, the buildrequires should be split up one per line.

**

Please note that it is not necessary to BR the -devel and the base package.
https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

**

Please note that explicit requires as in
 Requires:       atlas
are forbidden
 https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

**

Please don't replicate hardcoded values in the spec.

Source0:        https://github.com/SebWouters/CheMPS2/archive/v1.7.zip

should be written as 

Source0:        https://github.com/SebWouters/CheMPS2/archive/v%{version}.zip

Also, you should use .tar.gz instead of .zip

**

export CMAKE_INCLUDE_PATH=%{_includedir}/atlas-x86_64-base

is incorrect on most architectures, and should be replaced by

export CMAKE_INCLUDE_PATH=%{_includedir}/atlas

**

You're mixing macro styles, ${RPM_BUILD_ROOT} vs %{buildroot}. Please pick one and stick with it. [Personally, I prefer the latter.]

https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

**

gzip %{_builddir}/%{name}-%{version}/chemps2.1
mkdir -p %{buildroot}%{_mandir}/man1/
cp -p %{_builddir}/%{name}-%{version}/chemps2.1.gz %{buildroot}%{_mandir}/man1/

You don't need to compress the man pages, rpmbuild will do it for you.

Also, referring to %{_builddir} is somewhat bad style.

You could rewrite the %install section as

make -C build install DESTDIR=$RPM_BUILD_ROOT
install -D -p -m 644 chemps2.1 %{buildroot}%{_mandir}/man1/chemps2.1
cd PyCheMPS2
%{__python2} setup.py install -O1 --skip-build --root %{buildroot}
find %{buildroot} -name '*.la' -exec rm -f {} ';'
find %{buildroot} -name '*.a' -exec rm -f {} ';'


Also, instead of deleting the static libraries after install you might consider fixing the cmake files to not build them at all, after which you could send the patch upstream. I'd recommend adding a ENABLE_SHARED flag toggling build of a shared library, and linking everything against it if it's enabled. Correspondingly, a ENABLE_STATIC flag would just toggle if a static library is built or not.

**

for i in test*[0-9].py; do LD_LIBRARY_PATH=../../build/CheMPS2 PYTHONPATH=`find .. -name lib.*`  %{__python2} $i || { echo 'tests failed' ; exit 1; }; done

You're allowed to use indentation in spec files. This would be much more readable as

for i in test*[0-9].py; do
  export LD_LIBRARY_PATH=../../build/CheMPS2
  export PYTHONPATH=`find .. -name lib.*`
  %{__python2} $i || { echo 'tests failed' ; exit 1; }
done

Also, the tests take too much time. Please reduce the set of tests, or otherwise building the package will take ages on the ARM architecture.

**

Please don't use wildcards so blatantly in %files

%{_libdir}/*.so.*
%{_bindir}/chemps2
%{_mandir}/man1/*.gz

%files devel
%{_includedir}/*
%{_libdir}/*.so

%files python
%{python2_sitearch}/*

could be written as

%files
%{_libdir}/libchemps2.so.*
%{_bindir}/chemps2
%{_mandir}/man1/chemps2.1.*

%files devel
%{_includedir}/chemps2/
%{_libdir}/libchemps2.so

%files python
%{python2_sitearch}/CheMPS2-python-%{version}-py*.egg-info
%{python2_sitearch}/PyCheMPS2.so

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

===== MUST items =====

C/C++:
[-]: Provides: bundled(gnulib) in place as required.
     Note: Sources not installed
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[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.
[!]: License field in the package spec file matches the actual license.
- License is GPLv2+, not GPLv2.

[x]: License file installed when any subpackage combination is installed.

[!]: %build honors applicable compiler flags or justifies otherwise.
- Build with 'make %{?_smp_mflags} VERBOSE=1' so that flags become visible.

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

[!]: Package consistently uses macros.
- As noted above, please fix macros.

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

[!]: Requires correct, justified where necessary.
- No explicit requires as noted above.

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

[!]: Package complies to the Packaging Guidelines
- Fix issues above.

[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 must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.

[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
- Removing the buildroot has not been necessary since RHEL 5.

[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 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]: 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.
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Scriptlets must be sane, if used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %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]: Uses parallel make %{?_smp_mflags} macro.
[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).
[x]: Rpmlint is run on all installed packages.
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.


Rpmlint
-------
Checking: CheMPS2-1.7-1.fc23.x86_64.rpm
          CheMPS2-devel-1.7-1.fc23.x86_64.rpm
          CheMPS2-python-1.7-1.fc23.x86_64.rpm
          CheMPS2-debuginfo-1.7-1.fc23.x86_64.rpm
          CheMPS2-1.7-1.fc23.src.rpm
CheMPS2.x86_64: W: spelling-error Summary(en_US) initio -> initiation
CheMPS2.x86_64: W: spelling-error %description -l en_US renormalization -> re normalization, re-normalization, normalization
CheMPS2.x86_64: W: spelling-error %description -l en_US initio -> initiation
CheMPS2-devel.x86_64: W: only-non-binary-in-usr-lib
CheMPS2-devel.x86_64: W: no-documentation
CheMPS2-python.x86_64: W: no-documentation
CheMPS2.src: W: spelling-error Summary(en_US) initio -> initiation
CheMPS2.src: W: spelling-error %description -l en_US renormalization -> re normalization, re-normalization, normalization
CheMPS2.src: W: spelling-error %description -l en_US initio -> initiation
5 packages and 0 specfiles checked; 0 errors, 9 warnings.




Rpmlint (debuginfo)
-------------------
Checking: CheMPS2-debuginfo-1.7-1.fc23.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
CheMPS2.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libchemps2.so.2 /lib64/libhdf5_hl.so.10
CheMPS2.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libchemps2.so.2 /lib64/libz.so.1
CheMPS2.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libchemps2.so.2 /lib64/libdl.so.2
CheMPS2-devel.x86_64: W: only-non-binary-in-usr-lib
CheMPS2-devel.x86_64: W: no-documentation
CheMPS2-python.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 6 warnings.

- You could try fixing the shlib dependency warning with 
https://fedoraproject.org/wiki/Common_Rpmlint_issues#unused-direct-shlib-dependency


Requires
--------
CheMPS2 (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    atlas
    libc.so.6()(64bit)
    libchemps2.so.2()(64bit)
    libdl.so.2()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.3.1)(64bit)
    libgomp.so.1()(64bit)
    libgomp.so.1(GOMP_1.0)(64bit)
    libgomp.so.1(GOMP_4.0)(64bit)
    libgomp.so.1(OMP_1.0)(64bit)
    libhdf5.so.10()(64bit)
    libhdf5_hl.so.10()(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libtatlas.so.3()(64bit)
    libz.so.1()(64bit)
    rtld(GNU_HASH)

CheMPS2-devel (rpmlib, GLIBC filtered):
    CheMPS2(x86-64)
    libchemps2.so.2()(64bit)

CheMPS2-python (rpmlib, GLIBC filtered):
    CheMPS2(x86-64)
    libc.so.6()(64bit)
    libchemps2.so.2()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libpython2.7.so.1.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    python(abi)
    rtld(GNU_HASH)

CheMPS2-debuginfo (rpmlib, GLIBC filtered):



Provides
--------
CheMPS2:
    CheMPS2
    CheMPS2(x86-64)
    libchemps2.so.2()(64bit)

CheMPS2-devel:
    CheMPS2-devel
    CheMPS2-devel(x86-64)

CheMPS2-python:
    CheMPS2-python
    CheMPS2-python(x86-64)

CheMPS2-debuginfo:
    CheMPS2-debuginfo
    CheMPS2-debuginfo(x86-64)



Unversioned so-files
--------------------
CheMPS2-python: /usr/lib64/python2.7/site-packages/PyCheMPS2.so

- I think this should be OK since it's a python module. 


Source checksums
----------------
https://github.com/SebWouters/CheMPS2/archive/v1.7.zip :
  CHECKSUM(SHA256) this package     : 78528aac83f6dc32452e19b7b7905e2b6af1be99e76c8f75e0d15857b8ae8b89
  CHECKSUM(SHA256) upstream package : 979fd459ce5288e169f749e152e2454b49f6cde9d760dadf3f4e32bbc6d8ff85
diff -r also reports differences

- Please double check your source.

Hi Susi,

Sorry about the SRPM. Here's the separate links:

SRPM: https://copr.fedorainfracloud.org/coprs/talcite/CheMPS2/build/342235/
Spec: http://copr-dist-git.fedorainfracloud.org/cgit/talcite/CheMPS2/CheMPS2.git/tree/CheMPS2.spec

I've changed everything you commented on, except for the shared-only build. The author has added the feature to master. It will be updated the next release he makes.

Do you mind taking a look at it again please?

Thanks!

(In reply to Susi Lehtola from comment #3)
> Please note that it is not necessary to BR the -devel and the base package.
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

This has still not been addressed. You should drop BRs on atlas, python and zlib, because you already require their devel packages.

**

The spec file looks *way* cleaner now.

Now, there is only one remaining issue: the build flags, which have two problems. First, there's an -O3 flag that may override the -O2 in %{optflags}. The default is -O2 for a reason: some of the optimizations in -O3 not only make the code bigger, but also can inhibit the performance. You should patch out the -O3 flag.

Also, the build scripts have added a -march=native flag. Obviously this is not acceptable in Fedora: the binaries must run on all computers of a given architecture, while with -march=native it is only guaranteed to run on the exact same processor model as on the build system.

So basically you'll just need to add -DENABLE_XHOST=OFF to the %cmake line and comment out

set (CMAKE_CXX_FLAGS "-g -O3 ${CMAKE_CXX_FLAGS}")

in CMakeLists.txt

Oh and also instead of python-devel you should BuildRequires python2-devel because you use python2 instead of python in the spec file.
https://fedoraproject.org/wiki/Packaging:Python

Ahh that's what you meant by the BR comment! Okay, I've changed them now.

SPEC: http://copr-dist-git.fedorainfracloud.org/cgit/talcite/CheMPS2/CheMPS2.git/tree/CheMPS2.spec
SRPM: https://copr.fedorainfracloud.org/coprs/talcite/CheMPS2/build/343012/

I used sed to modify CMakeLists.txt from the spec. I'm not sure if that was the right way to go or if I should have done the whole patch thing.

(In reply to Matt Chan from comment #9)
> I used sed to modify CMakeLists.txt from the spec. I'm not sure if that was
> the right way to go or if I should have done the whole patch thing.

You should really use patches, because sed'ing is not safe. If the section changes, sed will still run, but the text doesn't get modified.

(Also, the sed command should be run in %prep, not %build.)

**

You can still drop the one commented line
#for i in build/tests/test*[0-9]; do $i; done

**

The package has been APPROVED for inclusion in Fedora.

Please make the two above changes before git import.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/CheMPS2

CheMPS2-1.7-4.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-8939093259

CheMPS2-1.7-4.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-c81486dde4

CheMPS2-1.7-4.fc23 has been pushed to the Fedora 23 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-2016-8939093259

CheMPS2-1.7-4.fc24 has been pushed to the Fedora 24 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-2016-c81486dde4

CheMPS2-1.7-4.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

CheMPS2-1.7-4.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.