Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
| dnl Should the MPI preprocessor flags be exported (AC_SUBST) instead | ||
| dnl and only be used in the build rules of PARPACK? | ||
| FFLAGS="$MPI_CPPFLAGS $FFLAGS" | ||
| FCFLAGS="$MPI_CPPFLAGS $FCFLAGS" | ||
| CPPFLAGS="$MPI_CPPFLAGS $CPPFLAGS" |
There was a problem hiding this comment.
These flags have previously been prepended directly to FFLAGS, FCFLAGS, and CPPFLAGS.
For the changes here, it made sense to collect the corresponding flags in MPI_CPPFLAGS (to avoid replicating the same thing in multiple places).
With that, it would be possible to only use these flags for the library that needs them.
Should this be done?
There was a problem hiding this comment.
Likely. Old dead thing... Cleaning/simplifying would be nice...
|
Could you add dedicated tests for this? Like apt-get source openmpi, build-from-source mpi and install it in some /opt-or-tmp/mpi directory and build / run arpack test suite? (mpi is a small lib so building it should be OK) I would understand 101% this work would be above what you intended to do: i.e. adding some kind of ubuntu_latest_autotools_custom_mpi test in the CI. I would understand 201% you would not consider adding a second test for the same purpose but using cmake: i.e. adding some kind of ubuntu_latest_cmake_custom_mpi test in the CI. I understand / agree on the need / PR. But, ideally, I'd like this to be tested!... With both cmake and autotools as we try to get them both in sync and having the same level of functionality. I'd like to avoid blind pushing: we had lots of never ending and re-entrant problems with blind pushing... This is why at some point I created ubuntu_latest_cmake_install / ubuntu_latest_autotools_install and tstCMakeInstall.sh / tstAutotoolsInstall.sh... |
|
I agree that a good CI coverage is useful. I can try to look into that. Would it be ok to split the (currently huge) workflow file into multiple ones (one per platform) first? |
|
For now, my advise would be:
This is the way I did that when I had to... So you can do what you need without opening a PR (split CI) inside another unrelated one (custom mpi)!
Thanks! :)
Yes but in another PR!... Mixing both would be a mess... I let you decide which way you wanna go |
I opened #489 with a proposed change for that. I'll mark this PR as draft until that one has settled. |
|
@mmuetzel: On this PR, could you update the link in the README 
It seems broken as the yaml of the CI changed. |
|
Sorry for the late reply. I'm currently being caught up in other projects. In the meantime, I opened #493 to address your last comment. (Thank you for pointing that out.) |
Currently, there are no dedicated flags that would point the `configure` script to an implementation of MPI that is installed at a non-default prefix. (See opencollab#479.) Add support for the optional configure flags `--with-mpi-includedir` and `--with-mpi-libdir` to select a non-default installation location of MPI. Additionally, support setting the prefix of the MPI installation with `--enable-mpi=PREFIX`.
Add a configuration to the build matrix on Ubuntu that builds OpenMPI and installs it at a non-default prefix where it should picked up by the configure script using the new flags.
|
I added a CI configuration that builds OpenMPI and installs it at a non-default prefix as you suggested. Marking as ready for review again. |
|
Sorry for late reply. I had no time yet to review this PR (and the others): september is busy. #474 (comment): ideally the same thing should be done for BLAS / LAPACK. I'll try to do that when / if possible... |
| install: without | ||
| blas: MKL | ||
| configure-flags: --enable-icb --with-blas=mkl_gf_ilp64 --with-lapack=mkl_gf_ilp64 | ||
| - name: Ubuntu autotools with non-default MPI prefix |
There was a problem hiding this comment.
No time to review. I've just bird-eye the thing.
We fight for years to make autotools/cmake propose the same options/behavior: did you test also cmake with non-default MPI prefix?
There was a problem hiding this comment.
CMake has its FindMPI module which handles this. So, there is no need for any changes in ARPACK-ng when using CMake as the build tool.
I don't think we need to add CI tests to check whether CMake works as they document.
Obviously, the flags will have different names. After all, CMake and autotools are quite different build systems.
There was a problem hiding this comment.
Yes.. And no?!
Yes, on a theoretical point of view, I agree: we should not test CMake.
No, on a practical point of view: it's always good to provide examples of "how to use CMake".
I've seen many people raging "find_package doesn't find anything" and blame CMake. CMake rely on the environment to find packages (there is no magic!). So typically, in an admin (that installed MPI or BLAS or LAPACK) and didn't set environment variable correctly in the module-load file then find_package will never find anything and you can not blame CMake! On supercomputers where you are not root, this may become a nightmare...
I believe providing examples of using CMake in these edge-cases (non-default location of some dependency) in the CI is a good thing as people who want to use arpack in these situations will have an example and you could then reply to @Fabian188 here #474 (comment): "did you set at least CMAKE_PREFIX_PATH (or friends) and check your environment is set properly (by module-load or apt or any other machinery that is supposed to have done that for you) like the CI example?"
I understand your point. But, for me, testing also cmake with non-default MPI prefix is valuable.
I let you decide what you think is best.
There was a problem hiding this comment.
In my opinion, adding a CI runner just for that is overkill.
It would just apply to one specific MPI implementation on one specific operating system. Other MPI implementations on other operating systems might need to be handled differently.
Some documentation might be better. But then, it is already documented by CMake. And any instructions here might depend on the CMake version of the user. (When it comes to the FindMPI module specifically, it hasn't changed in a long time. But that doesn't mean that CMake will never change it.)
So, the best might be to just redirect to the official documentation:
https://cmake.org/cmake/help/latest/module/FindMPI.html
And similarly for FindBLAS.
Maybe, having a collection of links to the upstream documentation of the used CMake modules would be best. The respective section of the README.md file looks like a good place for that.
There was a problem hiding this comment.
did you test also cmake with non-default MPI prefix?
I don't think we need to add CI tests to check whether CMake works as they document.
I let you decide what you think is best.
OK, I can understand both decisions.
There was a problem hiding this comment.
I've opened #496 for that documentation change.
2 participants
Pull request purpose
Currently, there are no dedicated flags that would point the
configurescript to an implementation of MPI that is installed at a non-default prefix. (See #479.)Detailed changes proposed in this pull request
Add support for the optional configure flags
--with-mpi-includedirand--with-mpi-libdirto select a non-default installation location of MPI. Additionally, support setting the prefix of the MPI installation with--enable-mpi=PREFIX.