homebrew-core icon indicating copy to clipboard operation
homebrew-core copied to clipboard

mumps 5.6.2 (new formula)

Open johnwparent opened this issue 1 year ago • 10 comments

Currently the only tap that I could find vendoring MUMPS is no longer maintained and is out of date. Packages currently in brew/core actually depend on MUMPS and vendor the package as a resource rather than a package in its own right, which has resulted in some users installing things like ipopt simply to get the mumps headers/etc. MUMPS should be a first class package.

  • [x] Have you followed the guidelines for contributing?
  • [ ] Have you ensured that your commits follow the commit style guide?
  • [x] Have you checked that there aren't other open pull requests for the same formula update/change?
  • [x] Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • [x] Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • [x] Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

johnwparent avatar Nov 15 '23 17:11 johnwparent

Thanks for contributing to Homebrew! :tada: It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

github-actions[bot] avatar Nov 15 '23 17:11 github-actions[bot]

@SMillerDev I have addressed most comments.

The remaining point of uncertainty from the previous review is the "magic". I've removed all that I can while still allowing MUMPS to build for me. I've reached out to the MUMPS folks to see about upstreaming the install target, but AFAICT they have no public VCS so I just have the patch locally. I don't foresee the quick installation logic I have here presenting much of a maintenance issue, and on the off chance MUMPS modifies their installation tree in a way the recipe doesn't handle, it's a quick update. Maintaining the patch arbitrarily seems much harder and I'd rather not hold off this MR on a mailing list response.

All other build system intervention is fairly site-specific (or are specific instructions from the MUMPS build docs) and as such can't really be upstreamed in a generic fashion.

johnwparent avatar Dec 05 '23 22:12 johnwparent

The errors in the compilation might be due to the fact that the present formula compiles with clang. Given the fact that it is a (mixture) of fortran and c, compiled with gfortran, the gcc compiler might be more appropriate.

fahasch avatar Dec 18 '23 13:12 fahasch

The errors in the compilation might be due to the fact that the present formula compiles with clang. Given the fact that it is a (mixture) of fortran and c, compiled with gfortran, the gcc compiler might be more appropriate.

@fahasch Thanks for the insight, I was under the impression that this stanza: depends_on "gcc" would enforce that gcc is the compiler, rather than clang. AFAIK mumps cannot be compiled with the clang compiler (even with flang) so this is likely the cause of the issue as you said.

johnwparent avatar Dec 18 '23 20:12 johnwparent

I'm not completely sure: it is true that ENV.cc should be gcc. To be absolutely sure you can add fails_with :clang after depends_on gcc.

I just tried your formula locally (on MacOS). It turns out that the problem is with the linker. The option -soname is not supported by ld. It should be replaced by -install_name. The relevant parameter might be SONAME. You find some ideas how to organize the linker in the formula for scotch.

fahasch avatar Dec 18 '23 20:12 fahasch

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Jan 09 '24 00:01 github-actions[bot]

@fahasch I believe I have made all requested edits and the package seems to be working now.

johnwparent avatar Jan 16 '24 17:01 johnwparent

Agreed! Let us see what @SMillerDev thinks about the formula.

fahasch avatar Jan 16 '24 17:01 fahasch

@SMillerDev I believe I have either responded to or addressed your concerns/comments and this PR is ready for another round of review feedback.

johnwparent avatar Feb 07 '24 15:02 johnwparent

@SMillerDev @fahasch Just wanted to circle back on this to keep things moving forward, please let me know what I can do to promote the progressions of this MR.

johnwparent avatar Feb 20 '24 16:02 johnwparent

@SMillerDev Apologies for the delay, I've been OOO for the past week. I have addressed the requested changes unrelated to upstreaming the customizations I make in this MR and believe this is ready for another round of review. Regarding the upstreaming of these build system modifications, the MUMPS developers are interested in changes (within reason + good practice) that help them better package MUMPS for brew and to have outright build support for MacOS. I am iterating with them on how best to go about this and what these changes will look like now. However, this still leaves the issue of the lack of upstream issue/MR tracking. My hope is that we can merge this MR as is (assuming no more code review is necessary) without needing to rely on an upstream issue to link to. In lieu of an upstream link, I propose a detailed docstring as part of this package recipe with a procedure for updating the MUMPS version that includes verifying the inclusion of my customizations and removing the salient components of the ruby code. I am however, open to really any solution that moves this along and sidesteps the issue of an upstream link, as that does not seem feasible at this time, so please let me know what the brew folks would feel most comfortable with, and we can iterate from there. Thanks for all the review so far! FWIW I'm also willing to be on the hook to update the progress of the upstream patches in the MUMPS brew recipe as I go.

johnwparent avatar Mar 04 '24 23:03 johnwparent

If there is a mailing list archive link, I think that would be good to add as reference. Otherwise this conversation will have to do.

SMillerDev avatar Mar 05 '24 08:03 SMillerDev

If there is a mailing list archive link, I think that would be good to add as reference. Otherwise this conversation will have to do.

@SMillerDev I've been looking for an archive for a while now to no results. There's clearly an option to access the archive in the MUMPS mailing list signup UI, but the option appears non functional/enabled. https://listes.ens-lyon.fr/sympa/subscribe/mumps-users for context, you can see the unhighlighted option for "archive" in the table to the left of the page.

At this point, it seems like this conversation is all we'll have to document this issue, so LGTM?

johnwparent avatar Mar 05 '24 15:03 johnwparent

I'd like to help push this over the finish line, if I can. What work still remains here before this can merge?

jwnimmer-tri avatar Mar 13 '24 02:03 jwnimmer-tri

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Apr 03 '24 07:04 github-actions[bot]

@SMillerDev Last I understood these changes were ready for the upstream a few weeks ago, are there remaining blockers? What can we do to move this PR along and prevent stale-ness? Any help is appreciated, and thanks for your review so far!

johnwparent avatar Apr 03 '24 20:04 johnwparent

It does work with clang, compiling MUMPS on my machine with Clang absolutely works.

Also it seems that all these environment modifications are not necessary - here is my Makefile.inc:

SCOTCHDIR  = $(shell brew --prefix scotch)
ISCOTCH    = -I$(SCOTCHDIR)/include
LSCOTCH    = -L$(SCOTCHDIR)/lib -lptesmumps -lscotch -lptscotch -lptscotcherr -lptscotcherrexit -lscotcherrexit

LPORDDIR = $(topdir)/PORD/lib/
IPORD    = -I$(topdir)/PORD/include/
LPORD    = -L$(LPORDDIR) -lpord

LMETISDIR = $(shell brew --prefix metis)
IMETIS    = -I$(LMETISDIR)/include

LMETIS    = -L$(LMETISDIR)/lib -lmetis

ORDERINGSF = -Dscotch -Dmetis -Dpord -Dptscotch
ORDERINGSC  = $(ORDERINGSF)

LORDERINGS = $(LMETIS) $(LPORD) $(LSCOTCH)
IORDERINGSF = $(ISCOTCH)
IORDERINGSC = $(IMETIS) $(IPORD) $(ISCOTCH)

# PLAT : use it to add a default suffix to the generated libraries
PLAT    = 
# Library extension, + C and Fortran "-o" option
# may be different under Windows
LIBEXT  = .a
OUTC    = -o 
OUTF    = -o 
# RM : remove files
RM      = /bin/rm -f
# CC : C compiler
CC      = cc
# FC : Fortran 90 compiler
FC      = mpif90
# FL : Fortran linker
FL      = mpif90
# AR : Archive object in a library
#      keep a space at the end if options have to be separated from lib name
AR      = ar vr 
# RANLIB : generate index of an archive file
#   (optionnal use "RANLIB = echo" in case of problem)
RANLIB  = ranlib
#RANLIB  = echo

# DEFINE HERE YOUR LAPACK LIBRARY
LAPACK = -llapack

# SCALAP should define the SCALAPACK and  BLACS libraries.
SCALAP  = -L$(shell brew --prefix scalapack)/lib -lscalapack

# INCLUDE DIRECTORY FOR MPI
INCPAR  = -I$(shell brew --prefix open-mpi)/include

# LIBRARIES USED BY THE PARALLEL VERSION OF MUMPS: $(SCALAP) and MPI
LIBPAR  = $(SCALAP) $(LAPACK) -L/usr/lib -lmpi

# The parallel version is not concerned by the next two lines.
# They are related to the sequential library provided by MUMPS,
# to use instead of ScaLAPACK and MPI.
INCSEQ  = -I$(topdir)/libseq
LIBSEQ  = $(LAPACK) -L$(topdir)/libseq -lmpiseq

# DEFINE HERE YOUR BLAS LIBRARY
LIBBLAS = -lblas

# DEFINE HERE YOUR PTHREAD LIBRARY
LIBOTHERS = -lpthread

# FORTRAN/C COMPATIBILITY:
#  Use:
#    -DAdd_ if your Fortran compiler adds an underscore at the end
#              of symbols,
#     -DAdd__ if your Fortran compiler adds 2 underscores,
#
#     -DUPPER if your Fortran compiler uses uppercase symbols
#
#     leave empty if your Fortran compiler does not change the symbols.
#

CDEFS = -DAdd_

#COMPILER OPTIONS
OPTF    = -O -fallow-argument-mismatch
OPTC    = -O -I.
OPTL    = -O

# CHOOSE BETWEEN USING THE SEQUENTIAL OR THE PARALLEL VERSION.

#Sequential:
#INCS = $(INCSEQ)
#LIBS = $(LIBSEQ)
#LIBSEQNEEDED = libseqneeded

#Parallel:
INCS = $(INCPAR)
LIBS = $(LIBPAR)
LIBSEQNEEDED = 

Works within a regular shell and just make all.

ProfFan avatar Apr 16 '24 18:04 ProfFan

The RPATH issue can be solved with install_name_tool after libs are built, btw.

ProfFan avatar Apr 16 '24 18:04 ProfFan