Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve CMake install setup #1325

Merged
merged 3 commits into from
Apr 28, 2023
Merged

Improve CMake install setup #1325

merged 3 commits into from
Apr 28, 2023

Conversation

upsj
Copy link
Member

@upsj upsj commented Apr 19, 2023

Our CMake install setup is not following a handful of best practices related to GNUInstallDirs and relocatability:

  • GinkgoConfig.cmake should not contain absolute paths to be relocatable, we can use PATH_VARS in configure_package_config_file to handle this. This also means removing all references to GINKGO_INSTALL_PREFIX
  • GinkgoConfig.cmake should not contain the project source path, see GinkgoConfig.cmake leaks project source path on installation #1188
  • We should not manually construct installation dirs using CMAKE_INSTALL_PREFIX, but instead rely on the _FULL equivalents provided by GNUInstallDirs

Fixes #1188
Fixes #1152

@upsj upsj self-assigned this Apr 19, 2023
@ginkgo-bot ginkgo-bot added the reg:build This is related to the build system. label Apr 19, 2023
@upsj upsj requested a review from a team April 19, 2023 09:52
@tcojean tcojean self-requested a review April 19, 2023 11:28
tcojean
tcojean approved these changes Apr 19, 2023
View reviewed changes
Copy link
Member

@tcojean tcojean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

yhmtsai
yhmtsai reviewed Apr 20, 2023
View reviewed changes
cmake/GinkgoConfig.cmake.in
set(GINKGO_INSTALL_RPATH_FOR_HIP "-Wl,-rpath,${GINKGO_INSTALL_LIBRARY_DIR}")
set(GINKGO_INSTALL_RPATH ${GINKGO_INSTALL_RPATH})
set(GINKGO_INSTALL_RPATH_USE_ORIGIN ${GINKGO_INSTALL_RPATH_USE_ORIGIN})
set(GINKGO_INSTALL_RPATH_DEPENDENCIES ${GINKGO_INSTALL_RPATH_DEPENDENCIES})

# Forward Ginkgo's MODULE PATH and the PREFIX PATH for HIP and more
list(APPEND CMAKE_MODULE_PATH "@CMAKE_MODULE_PATH@" "${GINKGO_INSTALL_MODULE_DIR}")
list(APPEND CMAKE_PREFIX_PATH "@CMAKE_PREFIX_PATH@")
list(APPEND CMAKE_MODULE_PATH "@PACKAGE_GINKGO_INSTALL_MODULE_DIR@")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after this, ginkgo only includes the ginkgo module in cmake without those cmake dependence when build, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, true. This might be an issue at some point if we include transitive dependencies via CMake (e.g. Ginkgo includes Kokkos, Kokkos adds its own CMake modules that are not installed in the global module path), but I think that should be handled on the package manager level instead of by us, and is also a pretty obscure setting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the CMAKE_MODULE_PATH for HIP, which we set manually, to the GinkgoConfig file. I guess a good guideline is to forward only the module paths we add ourselves?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about the HIP because HIP cmake stuff does not work consistently

cmake/GinkgoConfig.cmake.in
Comment on lines +95 to +97
set(GINKGO_INSTALL_PREFIX "@PACKAGE_CMAKE_INSTALL_PREFIX@")
set(GINKGO_INSTALL_INCLUDE_DIR "@PACKAGE_CMAKE_INSTALL_FULL_INCLUDEDIR@")
set(GINKGO_INSTALL_LIBRARY_DIR "@PACKAGE_CMAKE_INSTALL_FULL_LIBDIR@")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when build the ginkgo without installation, the GINKGO_INSTALL_INCLUDE_DIR and GINKGO_INSTALL_LIBRARY_DIR will be wrong.

Copy link
Member Author

@upsj upsj Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case, either Ginkgo was built via add_subdirectory, when the config file doesn't matter, or somebody could be trying to use Ginkgo through the CMake package registry, then there is no valid include dir, so I think we can ignore this edge case for now - the provided target should still work. Related: CMP0090

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually use this way by giving the cmake location to the integration. It can ensure I use the ginkgo library is the latest one without make install when developing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still works, as long as you use the installed target - most of the GinkgoConfig.cmake variables don't really provide correct paths for the exported setting anyways, so I think we can ignore that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see

@upsj upsj force-pushed the cmake_install_path branch from 5ab7b78 to 3816e4d Compare April 27, 2023 07:23
@upsj upsj requested a review from yhmtsai April 27, 2023 07:34
@upsj upsj added 1:ST:ready-for-review This PR is ready for review 1:ST:run-full-test labels Apr 27, 2023
@pratikvn pratikvn added this to the Release 1.6.0 milestone Apr 27, 2023
yhmtsai
yhmtsai approved these changes Apr 28, 2023
View reviewed changes
cmake/GinkgoConfig.cmake.in
set(GINKGO_INSTALL_RPATH_FOR_HIP "-Wl,-rpath,${GINKGO_INSTALL_LIBRARY_DIR}")
set(GINKGO_INSTALL_RPATH ${GINKGO_INSTALL_RPATH})
set(GINKGO_INSTALL_RPATH_USE_ORIGIN ${GINKGO_INSTALL_RPATH_USE_ORIGIN})
set(GINKGO_INSTALL_RPATH_DEPENDENCIES ${GINKGO_INSTALL_RPATH_DEPENDENCIES})

# Forward Ginkgo's MODULE PATH and the PREFIX PATH for HIP and more
list(APPEND CMAKE_MODULE_PATH "@CMAKE_MODULE_PATH@" "${GINKGO_INSTALL_MODULE_DIR}")
list(APPEND CMAKE_PREFIX_PATH "@CMAKE_PREFIX_PATH@")
list(APPEND CMAKE_MODULE_PATH "@PACKAGE_GINKGO_INSTALL_MODULE_DIR@")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about the HIP because HIP cmake stuff does not work consistently

@upsj upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review 1:ST:run-full-test labels Apr 28, 2023
upsj and others added 3 commits April 28, 2023 14:08
@upsj
improve CMake install setup
ede9f22
@upsj
propagate HIP CMAKE_MODULE_PATH
84771aa
@upsj @tcojean @yhmtsai
review updates
f302a59 
- correctly forward RPATH config options
- remove unused variables

Co-authored-by: Terry Cojean <terry.cojean@kit.edu>
Co-authored-by: Yuhsiang M. Tsai <yhmtsai@gmail.com>
@upsj upsj force-pushed the cmake_install_path branch from d0ef08e to f302a59 Compare April 28, 2023 12:09
@upsj upsj merged commit 174184e into develop Apr 28, 2023
@upsj upsj deleted the cmake_install_path branch April 28, 2023 17:13
tcojean added a commit that referenced this pull request Jun 16, 2023
@tcojean
Merge Release 1.6.0
1f1ed46 
Release 1.6.0 of Ginkgo.

The Ginkgo team is proud to announce the new Ginkgo minor release 1.6.0. This release brings new features such as:
- Several building blocks for GPU-resident sparse direct solvers like symbolic
  and numerical LU and Cholesky factorization, ...,
- A distributed Schwarz preconditioner,
- New FGMRES and GCR solvers,
- Distributed benchmarks for the SpMV operation, solvers, ...
- Support for non-default streams in the CUDA and HIP backends,
- Mixed precision support for the CSR SpMV,
- A new profiling logger which integrates with NVTX, ROCTX, TAU and VTune to
  provide internal Ginkgo knowledge to most HPC profilers!

and much more.

If you face an issue, please first check our [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues) and the [open issues list](https://github.com/ginkgo-project/ginkgo/issues) and if you do not find a solution, feel free to [open a new issue](https://github.com/ginkgo-project/ginkgo/issues/new/choose) or ask a question using the [github discussions](https://github.com/ginkgo-project/ginkgo/discussions).

Supported systems and requirements:
+ For all platforms, CMake 3.13+
+ C++14 compliant compiler
+ Linux and macOS
  + GCC: 5.5+
  + clang: 3.9+
  + Intel compiler: 2018+
  + Apple Clang: 14.0 is tested. Earlier versions might also work.
  + NVHPC: 22.7+
  + Cray Compiler: 14.0.1+
  + CUDA module: CUDA 9.2+ or NVHPC 22.7+
  + HIP module: ROCm 4.5+
  + DPC++ module: Intel OneAPI 2021.3+ with oneMKL and oneDPL. Set the CXX compiler to `dpcpp`.
+ Windows
  + MinGW: GCC 5.5+
  + Microsoft Visual Studio: VS 2019+
  + CUDA module: CUDA 9.2+, Microsoft Visual Studio
  + OpenMP module: MinGW.

### Version Support Changes
+ ROCm 4.0+ -> 4.5+ after [#1303](#1303)
+ Removed Cygwin pipeline and support [#1283](#1283)

### Interface Changes
+ Due to internal changes, `ConcreteExecutor::run` will now always throw if the corresponding module for the `ConcreteExecutor` is not build [#1234](#1234)
+ The constructor of `experimental::distributed::Vector` was changed to only accept local vectors as `std::unique_ptr` [#1284](#1284)
+ The default parameters for the `solver::MultiGrid` were improved. In particular, the smoother defaults to one iteration of `Ir` with `Jacobi` preconditioner, and the coarse grid solver uses the new direct solver with LU factorization. [#1291](#1291) [#1327](#1327)
+ The `iteration_complete` event gained a more expressive overload with additional parameters, the old overloads were deprecated. [#1288](#1288) [#1327](#1327)

### Deprecations
+ Deprecated less expressive `iteration_complete` event. Users are advised to now implement the function `void iteration_complete(const LinOp* solver, const LinOp* b, const LinOp* x, const size_type& it, const LinOp* r, const LinOp* tau, const LinOp* implicit_tau_sq, const array<stopping_status>* status, bool stopped)` [#1288](#1288)

### Added Features
+ A distributed Schwarz preconditioner. [#1248](#1248)
+ A GCR solver [#1239](#1239)
+ Flexible Gmres solver [#1244](#1244)
+ Enable Gmres solver for distributed matrices and vectors [#1201](#1201)
+ An example that uses Kokkos to assemble the system matrix [#1216](#1216)
+ A symbolic LU factorization allowing the `gko::experimental::factorization::Lu` and `gko::experimental::solver::Direct` classes to be used for matrices with non-symmetric sparsity pattern [#1210](#1210)
+ A numerical Cholesky factorization [#1215](#1215)
+ Symbolic factorizations in host-side operations are now wrapped in a host-side `Operation` to make their execution visible to loggers. This means that profiling loggers and benchmarks are no longer missing a separate entry for their runtime [#1232](#1232)
+ Symbolic factorization benchmark [#1302](#1302)
+ The `ProfilerHook` logger allows annotating the Ginkgo execution (apply, operations, ...) for profiling frameworks like NVTX, ROCTX and TAU. [#1055](#1055)
+ `ProfilerHook::created_(nested_)summary` allows the generation of a lightweight runtime profile over all Ginkgo functions written to a user-defined stream [#1270](#1270) for both host and device timing functionality [#1313](#1313)
+ It is now possible to enable host buffers for MPI communications at runtime even if the compile option `GINKGO_FORCE_GPU_AWARE_MPI` is set. [#1228](#1228)
+ A stencil matrices generator (5-pt, 7-pt, 9-pt, and 27-pt) for benchmarks [#1204](#1204)
+ Distributed benchmarks (multi-vector blas, SpMV, solver) [#1204](#1204)
+ Benchmarks for CSR sorting and lookup [#1219](#1219)
+ A timer for MPI benchmarks that reports the longest time [#1217](#1217)
+ A `timer_method=min|max|average|median` flag for benchmark timing summary [#1294](#1294)
+ Support for non-default streams in CUDA and HIP executors [#1236](#1236)
+ METIS integration for nested dissection reordering [#1296](#1296)
+ SuiteSparse AMD integration for fillin-reducing reordering [#1328](#1328)
+ Csr mixed-precision SpMV support [#1319](#1319)
+ A `with_loggers` function for all `Factory` parameters [#1337](#1337)

### Improvements
+ Improve naming of kernel operations for loggers [#1277](#1277)
+ Annotate solver iterations in `ProfilerHook` [#1290](#1290)
+ Allow using the profiler hooks and inline input strings in benchmarks [#1342](#1342)
+ Allow passing smart pointers in place of raw pointers to most matrix functions. This means that things like `vec->compute_norm2(x.get())` or `vec->compute_norm2(lend(x))` can be simplified to `vec->compute_norm2(x)` [#1279](#1279) [#1261](#1261)
+ Catch overflows in prefix sum operations, which makes Ginkgo's operations much less likely to crash. This also improves the performance of the prefix sum kernel [#1303](#1303)
+ Make the installed GinkgoConfig.cmake file relocatable and follow more best practices [#1325](#1325)

### Fixes
+ Fix OpenMPI version check [#1200](#1200)
+ Fix the mpi cxx type binding by c binding [#1306](#1306)
+ Fix runtime failures for one-sided MPI wrapper functions observed on some OpenMPI versions [#1249](#1249)
+ Disable thread pinning with GPU executors due to poor performance [#1230](#1230)
+ Fix hwloc version detection [#1266](#1266)
+ Fix PAPI detection in non-implicit include directories [#1268](#1268)
+ Fix PAPI support for newer PAPI versions: [#1321](#1321)
+ Fix pkg-config file generation for library paths outside prefix [#1271](#1271)
+ Fix various build failures with ROCm 5.4, CUDA 12, and OneAPI 6 [#1214](#1214), [#1235](#1235), [#1251](#1251)
+ Fix incorrect read for skew-symmetric MatrixMarket files with explicit diagonal entries [#1272](#1272)
+ Fix handling of missing diagonal entries in symbolic factorizations [#1263](#1263)
+ Fix segmentation fault in benchmark matrix construction [#1299](#1299)
+ Fix the stencil matrix creation for benchmarking [#1305](#1305)
+ Fix the additional residual check in IR [#1307](#1307)
+ Fix the cuSPARSE CSR SpMM issue on single strided vector when cuda >= 11.6 [#1322](#1322) [#1331](#1331)
+ Fix Isai generation for large sparsity powers [#1327](#1327)
+ Fix Ginkgo compilation and test with NVHPC >= 22.7 [#1331](#1331)
+ Fix Ginkgo compilation of 32 bit binaries with MSVC [#1349](#1349)
@upsj upsj mentioned this pull request Oct 6, 2023
Merged
7 tasks
@upsj upsj mentioned this pull request Dec 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yhmtsai yhmtsai yhmtsai approved these changes

@tcojean tcojean tcojean approved these changes

Assignees

@upsj upsj

Labels
1:ST:ready-to-merge This PR is ready to merge. reg:build This is related to the build system.
Projects
None yet
Milestone
Release 1.6.0
Development

Successfully merging this pull request may close these issues.

GinkgoConfig.cmake leaks project source path on installation CMake modules get installed to a wrong path
5 participants
@upsj @yhmtsai @tcojean @pratikvn @ginkgo-bot