[Bug 1350257] Review Request: petsc - Portable Extensible Toolkit for Scientific Computation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1350257



--- Comment #16 from Antonio Trande <anto.trande@xxxxxxxxx> ---
(In reply to Dave Love from comment #12)
> OK, I've now spent some time looking at this and building it; sorry for the
> delay.
> 
> The %() stuff needs removing.  It can only work for things in the
> build root, and it's not necessary -- you can see errors in the build
> output and from rpmlint.

What do you mean with "%() stuff"?

> 
> I'd forgotten that I had a more recent cgns installed; the epel6
> version won't work, and needs to be conditionalized out.  I also found
> it needed a patch I'll attach to avoid a clash with complex.h.
> 
> EPEL needs Require: openmpi%{_isa}, which I think is harmless elsewhere.
> 
> MPI headers should be in $MPI_INCLUDE/include/petsc, similarly to the serial
> ones.
> 
> The other instances of "environment(modules)" need conditionalizing for el6.
> 
> Why is matplotlib required?

It's there by mistake.

> 
> What is the SETOPT_FLAGS=... business about?  It looks wrong and
> everything seems to work without it.

-D_FORTIFY_SOURCE looks not recognized by 'configure'.

> 
> I still think it should use openblas (as below).
> 
> I think it's better to remove use of pkg-config, which isn't
> necessary, and simplify the configure args.  I think the spec would be
> lot more maintainable and easier to follow by simplifying it.  I think
> the constant with_... conditions should be removed to start with --
> add comments about what doesn't work where appropriate.  You could
> abstract the common configure options and simplify them a lot,
> especially if you ensure the with_... macros are always defined.  The
> following works for me (compacted a bit for posting):
> 
>   %global common_args --with-shared-libraries=1 \\\
>    --with-petsc-arch=%{_arch} --with-fortran-interfaces=1 --with-debugging=0
> \\\
>    --with-precision=double --with-clanguage=C \\\
>    COPTFLAGS="$RPM_OPT_FLAGS -O3" CXXOPTFLAGS="$RPM_OPT_FLAGS -O3" \\\
>    FOPTFLAGS="$RPM_OPT_FLAGS -I%{_fmoddir} -O3" \\\
>    --with-gmp=1 --with-openmp=1 --with-libpng=1 --with-hwloc=1
> --with-metis=1 \\\
>   %if %{with_cgns} \
>    --with-cgns=1 \\\
>   %endif \
>    --with-valgrind=1 --with-valgrind-dir=%{_prefix} --with-papi=1 \\\
>   %if 0%{?el6} \
>    --with-papi-include=%{_libdir}/papi-5.1.1/%{_includedir}
> --with-papi-lib=%{_libdir}/papi-5.1.1/%{_prefix}/lib/libpapi.so \\\
>   %else \
>    --with-papi-include=%{_includedir} --with-papi-lib=libpapi.so \\\
>   %endif \
>   %if %{with_suitesparse} \
>    --with-suitesparse=1 \\\
>    --with-suitesparse-include=%{_includedir}/suitesparse \\\
>   
> --with-suitesparse-lib=[libumfpack.so,libklu.so,libcholmod.so,libbtf.so,
> libccolamd.so,libcolamd.so,libcamd.so,libamd.so,libsuitesparseconfig.so] \\\
>   %endif \
>   %if %{with_superlu} && ! %{arch64} \
>    --with-superlu=1 \\\
>    --with-superlu-include=%{_includedir}/SuperLU \\\
>    --with-superlu-lib=libsuperlu.so \\\
>   %endif \
>   %if %{with_opencl} \
>    --with-opencl=1 \\\
>   %endif
> 
> ...
> 
>   %if 0%{?with_openmpi}
>   pushd buildopenmpi_dir
>   %{_openmpi_load}
>   %configure \
>    %{common_args} \
>    --with-blas-lib=libopenblas.so --with-lapack-lib=libopenblas.so \
>    --with-scalapack=1 --with-hdf5=1 \
>   %if %{with_netcdf}
>    --with-netcdf=1 \
>   %endif
>    --with-ptscotch=1 --with-hypre=1 --with-sundials=1 \
>    --with-hypre-include=$MPI_INCLUDE/hypre
> --with-hypre-lib=$MLI_LIB/hypre/libHYPRE.so
> 
>   make \
>    V=1 MAKE_NP=%{?_smp_mflags}
> PETSC_DIR=%{_builddir}/%{name}-%{version}/buildopenmpi_dir
>   popd
>   %{_openmpi_unload}
>   %endif
> 
> I guess it's worth adding the -O3 to restore vectorization over the
> RPM defaults.
> 
> Assuming it's not going to be supported on el5, you could clean up the
> spec according to the packaging guidelines (no Group:, %clean etc.).
> 
> I hope that doesn't seem too picky.  I think it's worthwhile making the
> maintenance changes on something like this.  (Some of the comments
> apply to my version too, as a result of looking at it again.)

I am working on.

-- 
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
https://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]