Skip to content

Improve packageability of install and install gdb pretty-printer#1502

Merged
upsj merged 5 commits into
developfrom
install_pretty_printer
Dec 11, 2023
Merged

Improve packageability of install and install gdb pretty-printer#1502
upsj merged 5 commits into
developfrom
install_pretty_printer

Conversation

@upsj

@upsj upsj commented Dec 8, 2023

Copy link
Copy Markdown
Member

To make packaging Ginkgo easier, we can separate the installable components into Developement and Runtime (imagine libginkgo and libginkgo-dev packages). By default all packages get installed (we can control this using EXCLUDE_FROM_ALL), but you can choose individual components separately.

For some more context, look at https://www.youtube.com/watch?v=m0DwB4OvDXk around 30:00

Related content: https://www.youtube.com/watch?v=sBP17HQAQjk https://www.youtube.com/watch?v=_5weX5mx8hc

Also I noticed that the compiler version check doesn't check whether a language is enabled, so if CUDA is not enabled, it complains about us using the wrong CUDA compiler.

Finally, every time a shared library gets loaded, gdb looks for libsomething.so.x.x.x-gdb.py files in its auto-load safe-path. We can use this to make our pretty-printer available to user debug builds.

@upsj upsj added the 1:ST:ready-for-review This PR is ready for review label Dec 8, 2023
@upsj upsj requested a review from a team December 8, 2023 18:03
@upsj upsj self-assigned this Dec 8, 2023
@ginkgo-bot ginkgo-bot added the reg:build This is related to the build system. label Dec 8, 2023
Comment thread CMakeLists.txt Outdated
@upsj upsj changed the title Allow installing gdb pretty-printer to be auto-loaded Improve packageability of install and install gdb pretty-printer Dec 9, 2023
@upsj upsj requested review from a team and pratikvn December 9, 2023 12:21

@pratikvn pratikvn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought you also wanted to install the benchmark related files ?

Comment thread cmake/install_helpers.cmake Outdated
Comment thread cmake/install_helpers.cmake Outdated
Comment thread cmake/install_helpers.cmake Outdated

@yhmtsai yhmtsai left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Runtime is only used in runtime linking, right?
Users will only use them with some pre-built packages.

Comment thread cmake/install_helpers.cmake Outdated

@MarcelKoch MarcelKoch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would this be a good opportunity to also fix the cmake --install . --prefix some/prefix/path behavior? Right now, the --prefix path is just ignored.

@upsj

upsj commented Dec 11, 2023

Copy link
Copy Markdown
Member Author

@MarcelKoch I think this is a bigger can of works I'd like to avoid opening right now. There are a few places where we contain absolute paths instead of ones relative to CMAKE_INSTALL_PREFIX, and the pkg-config files would not be regenerated correctly, we probably need to use a install(CODE ...) block to do that correctly.

At the very least, we'd need to revert parts of #1325

@MarcelKoch

Copy link
Copy Markdown
Member

@upsj Ok, I just wanted to check that. Since I'm the only one complaining about it, I will look into it.

- install symlinks only for development (NAMELINK_COMPONENT)
- better formatting

Co-authored-by: Pratik Nayak <pratik.nayak@kit.edu>
@upsj upsj force-pushed the install_pretty_printer branch from 909999c to fda7259 Compare December 11, 2023 10:55
@upsj

upsj commented Dec 11, 2023

Copy link
Copy Markdown
Member Author

@pratikvn Installing benchmarks is much more complicated, since it requires installing things like gflags and our timer/linop wrapper libraries. I'll leave that for a subsequent PR

@tcojean

tcojean commented Dec 11, 2023

Copy link
Copy Markdown
Member

@pratikvn Installing benchmarks is much more complicated, since it requires installing things like gflags and our timer/linop wrapper libraries. I'll leave that for a subsequent PR

Couldn't we just depend on a package manager (spack, conan, pacman, ...) to first install gflags as most other packages do? And if that was not the case, fail loudly. I don't think we need to change that much, and I don't think it's unreasonable to ask people to install that package if they really want the benchmarks to be installed.

It does require an option to separately install dev tools/support compared to the core.

@upsj

upsj commented Dec 11, 2023

Copy link
Copy Markdown
Member Author

@tcojean We can avoid separate options by marking the Benchmarks component EXCLUDE_FROM_ALL

@pratikvn pratikvn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@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 labels Dec 11, 2023
@upsj upsj merged commit 310bb4c into develop Dec 11, 2023
@upsj upsj deleted the install_pretty_printer branch December 11, 2023 15:12
@lahwaacz

Copy link
Copy Markdown
Contributor

@upsj You mentioned "auto-load safe-path" in #1502 (comment) but the $prefix/lib directory, such as /usr/lib, where the gdb script is installed, is not a safe-path by default (at least on Arch Linux):

(gdb) show auto-load safe-path
List of directories from which it is safe to auto-load files is $debugdir:$datadir/auto-load.
(gdb) show data-directory
GDB's data directory is "/usr/share/gdb".
(gdb) show debug-file-directory
The directory where separate debug symbols are searched for is "/usr/lib/debug".

Indeed, most Arch Linux packages install gdb scripts in /usr/share/gdb/auto-load/usr/lib/:

$ pacman -Fx 'usr/lib/lib.*-gdb.py' | sort
usr/lib/libginkgo.so.1.9.0-gdb.py is owned by extra/ginkgo-hpc 1.9.0-1
usr/lib/libginkgo.so.1.9.0-gdb.py is owned by extra/ginkgo-hpc-cuda 1.9.0-1
usr/lib/libginkgo.so.1.9.0-gdb.py is owned by extra/ginkgo-hpc-hip 1.9.0-1
usr/share/gdb/auto-load/usr/lib/libarrow.so.1900.0.0-gdb.py is owned by extra/arrow 19.0.0-1
usr/share/gdb/auto-load/usr/lib/libeo.so.1.28.0-gdb.py is owned by extra/efl 1.28.0-1
usr/share/gdb/auto-load/usr/lib/libglib-2.0.so.0.8200.4-gdb.py is owned by core/glib2-devel 2.82.4-2
usr/share/gdb/auto-load/usr/lib/libgobject-2.0.so.0.8200.4-gdb.py is owned by core/glib2-devel 2.82.4-2
usr/share/gdb/auto-load/usr/lib/libgstreamer-1.0.so.0.2412.0-gdb.py is owned by extra/gstreamer 1.24.12-1
usr/share/gdb/auto-load/usr/lib/libisl.so.23.4.0-gdb.py is owned by core/libisl 0.27-1
usr/share/gdb/auto-load/usr/lib/libstdc++.so.6.0.33-gdb.py is owned by core/gcc 14.2.1+r753+g1cd744a6828f-1

Would you consider changing this or making it configurable in ginkgo?

@upsj

upsj commented Feb 23, 2025 via email

Copy link
Copy Markdown
Member Author

@lahwaacz

Copy link
Copy Markdown
Contributor

Hmm does cmake allow to easily install all except one specific components? AFAIK, it can either install everything or one component at a time. This would require me to loop over all components during install...

@upsj

upsj commented Feb 23, 2025 via email

Copy link
Copy Markdown
Member Author

@lahwaacz

Copy link
Copy Markdown
Contributor

It is manageable atm, but I'd have to watch out for the future components 😄 I still like the cmake variable more, it allows to keep the script in the current component and location by default without breaking the expectations of everybody else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1:ST:ready-to-merge This PR is ready to merge. reg:build This is related to the build system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants