-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
Default build settings for RAPIDS symbol visibility handling #10
Comments
After further consideration and some continued experimentation, I've come to the conclusion that the best approach here is to move away from the current design of rapids-logger and to instead aim to ship it as a library. The primary reason that I avoided this in the original design was because we intend to use rapids-logger in some header-only libraries (rmm and raft), with rmm in particular having endeavored to this point to also avoid having any pre-compiled library dependencies so that it could be easily used as a header-only dependency. My experiments in rapidsai/rmm#1740 indicate that it would be possible to accomplish this goal, but it would be quite complex and require propagating changes all the way down dependency trees. Moreover, it would make it quite challenging for dependencies to properly manage their spdlog dependencies from a packaging (not just build) perspective. Based on some offline discussions, I've opened rapidsai/rmm#1779 to propose that we convert rmm to a precompiled library, and since feedback was generally positive I think we can move forward with that approach. It should allow us to solve the core problem addressed by this issue without much of the build-time complexity that the previously proposed solutions would entail. |
See discussion in rapidsai/rapids-logger#10 (comment) and #1779. spdlog will remain a requirement for 25.02, but we will remove it in favor of a precompiled rapids-logger library in 25.04 (and that library will completely hide everything related to spdlog: APIs, package requirements, symbols, etc). Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) URL: #1780
This PR is the first step to addressing #10.
This PR is the first step to addressing #10.
If there is a plan to support the Windows platform in the future, current namespace export will not work:
If Windows support is planned, there are two solutions:
Then only define |
Thanks for the note! You're right about needing those kinds of changes if we intended to support Windows. That's currently not on the roadmap since none of the projects that rapids-logger is designed to support (rmm, cudf, cuml, raft) support Windows. If that changes, we can always update the code here. |
The changes in #7 and the tasks in #9 point to a complex interplay in build-time spdlog usage that we need to address. The idea behind rapidsai/rmm#1740 was that projects using rapids-logger could address the symbol visibility issues and conda package issues by forcing a rebuild of spdlog by their consumers when building the logger (the conda package issues stem from the fact that whether spdlog is using a header-only or compiled fmt is determined when the spdlog package is built, which means that the conda package for spdlog will always trigger dynamic linking to libfmt.so even if we use the header-only spdlog target). However, this choice leads us afoul of the last point in #9: we do not want to require consumers of packages using rapids-logger to need to have spdlog on their system, except in the case of header-only packages, and for header-only packages that requirement should only propagate up one level in the dependency stack.
Stepping back, what we ultimately need is to treat all of our package builds (as opposed to users building from source) as special cases with the following requirements, none of which hold for users building our packages from source or building their packages against our packages:
EXCLUDE_FROM_ALL
so that our packages don't ship any spdlog/fmt.To accomplish all of these goals, the best option we have is to default to using
CPM_DOWNLOAD_spdlog
and a static spdlog library, and it should setEXCLUDE_FROM_ALL
. Presumably we will need to add options to disable these settings so that end-users can still build how they want. These options will need to be available both in our lists files and in our config files so that the same optionality is available whether consumers are building against pre-built packages of ours or using something like CPM to build the whole tree from source.The outstanding question remains how to deal with the header-only propagation problem. I'm not sure that there is a good tradeoff here. On one hand, rapidsai/rmm#1740 seems too magical a solution and it would be ideal to be explicit and require consumers to find spdlog a priori, i.e. we move to a BYO spdlog model. Concretely, if a package like cugraph that requires rmm but does not itself have a direct dependency on spdlog (because it does not have its own logger) uses a pre-built rmm package, the cugraph build must call
rapids_cpm_spdlog
with the appropriate settings in place. To facilitate this, the defaulting functionality that I suggested above will probably need to be inrapids_cpm_spdlog
or an intermediate helper function that can be called byrapids_cpm_rapids_logger
before callingrapids_cpm_spdlog
(or other consumers like cugraph). While this is fine for packages that care about the symbol visibility enough to do this kind of thing manually, though, others would probably want spdlog found for them by rmm's config without needing to insert afind_package
themselves. I'm not sure of the right tradeoff here. The two choices we have are:What would really be ideal would be if we could trigger a
rapids_cpm_spdlog
call upon linking to thelogger_impl
target, but I don't know if such arbitrary code can be attached to a target when linked to. I can investigate that a bit further and see. Perhaps there is an approach with custom targets that could work. I suspect that any possible solution would be more convoluted than it's worth, though.The text was updated successfully, but these errors were encountered: