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.

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

[c++] Support dynamic linking to libspdlog #3585

Open
jdblischak opened this issue Jan 17, 2025 · 5 comments
Open

[c++] Support dynamic linking to libspdlog #3585

jdblischak opened this issue Jan 17, 2025 · 5 comments

Comments

@jdblischak
Copy link
Collaborator

When installing libtiledbsoma in a conda environment, we want to dynamically link to the binaries installed by conda. Currently our conda-build in tiledbsoma-feedstock dynamically links to the conda-installed libfmt but not to the conda-installed libspdlog.

This was discovered in the PR to add tiledbsoma to conda-forge (conda-forge/staged-recipes#28828). The contributor added a patch that allowed libtiledbsoma to dynamically link to the conda-installed libspdlog. We need to evaluate this patch. While we want to enable dynamically linking to an existing libspdlog installed in the system, we also need to ensure that we continue to support a bootstrapped installation where we download libspdlog and statically link it.

For more details, please see the discussion starting at conda-forge/staged-recipes#28828 (comment)

@jdblischak
Copy link
Collaborator Author

Now that I am aware that we aren't dynamically linking against the conda-installed libspdlog, this is allowing me to re-interpret our conversation in the Issue dedicated to troubleshooting the C++20 compilation errors. My mental model was that we were linking against the conda-installed libspdlog, but it appears that we were explicitly not doing that for some reason.

It's a super-long Issue that discusses various things. Below are comments relevant to the discussion of linking against spdlog:

This raises a lot of questions for me. What are our requirements in regards to linking against spdlog that we have to statically link it instead of dynamically linking against a system version (eg installed by conda)? Does the patch used in the conda-forge recipe reveal how we could accomplish linking against a system version?

@johnkerl
Copy link
Member

This raises a lot of questions for me

I defer to @nguyenv and/or @XanthosXanthopoulos (in addition to @jdblischak ).

All I want is green CI, in this repo as well as the tiledbsoma-feedstock repo, using C++20.

I'm happy with whatever it takes to get us there.

@nguyenv
Copy link
Member

nguyenv commented Jan 17, 2025

Sorry for any confusion, but I only suggested building spdlog as an external project because it failed with the conda-forge version but passed when built externally. I assumed that meant there was some incompatibility with using C++20 and the conda-forge version of spdlog. There wasn’t any other reason behind the suggestion.

@jdblischak
Copy link
Collaborator Author

Sorry for any confusion, but I only suggested building spdlog as an external project because it failed with the conda-forge version but passed when built externally. I assumed that meant there was some incompatibility with using C++20 and the conda-forge version of spdlog. There wasn’t any other reason behind the suggestion.

@nguyenv thanks for the clarification! Now I better understand your previous suggestion.

Thus I think we should be able to fix the CMake files to support either static linking an external project or dynamically linking a system library for libspdlog. I am not the CMake expert among us, so if someone better understands the patch I linked above, please feel free to experiment. Otherwise I can attempt to investigate more next week.

@XanthosXanthopoulos
Copy link
Collaborator

XanthosXanthopoulos commented Jan 17, 2025

These finding match what I saw with some experiments when debugging the C++20 builds. There where some linking errors regarding spdlog when I was simulating the local build as a single output recipe. I will experiment with the patch locally and report back any findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants