[c++] Remove `spdlog` requirement #1852

nguyenv · 2023-11-03T14:48:39Z

Issue and/or context:

libtiledbsoma still erroneously required spdlog as a dependency due to exposing logger.h in the public API headers. This was reported in a channel internal to TileDB here.

Changes:

Public headers using logger.h have been replaced by logger_public.h
The fmt library is a part of spdlog which means it cannot be used or accessed except internally
When possible, code that was using fmt in the public headers have now been moved into the cc source code which may continue to use the internal logger.h
For fmt usage in templated functions, unit tests, and pytiledbsoma.cc, string formatting has been refactored to use either std::ostringstream or basic std::string appending with + for compatibility with C++17. In C++20, we can switch to using std::format. TODO comments have been left in these areas

Notes for Reviewer:

It is not necessary to include these changes in the 1.5.0 release
These code changes have been tested and confirmed fixed by @jdblischak

johnkerl · 2023-11-03T14:51:37Z

It is not necessary to include these changes in the 1.5.0 release

@nguyenv I've been keeping release-1.5 in sync with main (modulo #1733) and I want to keep it that way until we get 1.5.0. I don't want to be omitting just a PR or two and then get confused.

teo-tsirpanis · 2023-11-03T14:53:38Z

libtiledbsoma/src/cli/cli.cc

+#include "utils/carrow.h"
+#include "utils/logger.h"


Why were these headers added?

logger.h is used for the LOG_* macros and fmt. carrow.h is used for ArrowSchema. soma_array.h use to include logger.h but that has now been removed.

But soma_array.h includes logger_public.h. Isn't it enough?

No, logger_public.h does not expose fmt

FWIW one can (somewhat poorly) drop in tinyfmt which is a less powerful than fmt but a single-header C++11 library. The key is, as @nguyenv points out, that the public logger only takes a string (or const char*, I forgot) to 'report' and does no formatting whatsoever itself.

teo-tsirpanis · 2023-11-03T14:54:41Z

libtiledbsoma/src/soma/soma_array.h

Can we move the generic method definitions to the .cc file and add explicit instantiations?

Are you talking about separating implemention of the templated functions in the cc? I considered doing that, but I feel very ambivalent about having a long list of explicit instantiations for multiple functions.

Is this something that we do in core?

I've seen it a couple of times¹ ², but have no problem either way.

Footnotes

https://github.com/TileDB-Inc/TileDB/blob/1f4ed0a69eca76a01183cca45a9d48fa0be5ecc6/tiledb/sm/query/legacy/cell_slab_iter.cc#L319-L328 ↩

https://github.com/TileDB-Inc/TileDB/blob/1f4ed0a69eca76a01183cca45a9d48fa0be5ecc6/tiledb/sm/query/readers/reader_base.cc#L1255-L1358 ↩

nguyenv · 2023-11-03T14:56:06Z

@eddelbuettel, if you don't mind, could you take a look at the R failures and push the necessary changes to this branch? Today I'd like to focus on getting the necessary Python stuff working for 1.5.0.

jdblischak

Thanks!

johnkerl · 2023-11-03T15:06:28Z

It is not necessary to include these changes in the 1.5.0 release

@nguyenv I've been keeping release-1.5 in sync with main (modulo #1733) and I want to keep it that way until we get 1.5.0. I don't want to be omitting just a PR or two and then get confused.

Of course if this doesn't land in main in time for 1.5.0 that's fine -- this is not a 1.5.0 blocker.

eddelbuettel · 2023-11-03T15:09:05Z

These are compilation errors coming from the header re-arrangement:

using C++17
g++ -std=gnu++17 -I"/usr/share/R/include" -DNDEBUG -I. -I../inst/include/ -I../inst/tiledb/include -I../inst/tiledbsoma/include   -I'/usr/lib/R/site-library/Rcpp/include' -I'/usr/lib/R/site-library/RcppSpdlog/include' -I'/usr/local/lib/R/site-library/RcppInt64/include'     -fpic  -g -O2 -ffile-prefix-map=/build/r-base-MHXHhT/r-base-4.3.1=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2  -c RcppExports.cpp -o RcppExports.o
In file included from ../inst/tiledbsoma/include/tiledbsoma/utils/arrow_adapter.h:10,
                 from ../inst/tiledbsoma/include/tiledbsoma/tiledbsoma:45,
                 from ../inst/include/tiledbsoma_types.h:21,
                 from RcppExports.cpp:4:
../inst/tiledbsoma/include/tiledbsoma/utils/carrow.h:16:8: error: redefinition of ‘struct ArrowSchema’
   16 | struct ArrowSchema {
      |        ^~~~~~~~~~~
In file included from ../inst/include/tiledbsoma_types.h:18,
                 from RcppExports.cpp:4:
./nanoarrow.h:83:8: note: previous definition of ‘struct ArrowSchema’
   83 | struct ArrowSchema {
      |        ^~~~~~~~~~~
In file included from ../inst/tiledbsoma/include/tiledbsoma/utils/arrow_adapter.h:10,
                 from ../inst/tiledbsoma/include/tiledbsoma/tiledbsoma:45,
                 from ../inst/include/tiledbsoma_types.h:21,
                 from RcppExports.cpp:4:
../inst/tiledbsoma/include/tiledbsoma/utils/carrow.h:32:8: error: redefinition of ‘struct ArrowArray’
   32 | struct ArrowArray {
      |        ^~~~~~~~~~
In file included from ../inst/include/tiledbsoma_types.h:18,
                 from RcppExports.cpp:4:
./nanoarrow.h:99:8: note: previous definition of ‘struct ArrowArray’
   99 | struct ArrowArray {
      |        ^~~~~~~~~~
make: *** [/usr/lib/R/etc/Makeconf:200: RcppExports.o] Error 1
ERROR: compilation failed for package ‘tiledbsoma’
* removing ‘/tmp/RtmpPq5dYB/Rinst[192](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/6746709009/job/18341227889?pr=1852#step:10:193)9110b964d/tiledbsoma’

Any chance we can keep changess local? The idea was less spill not more?

nguyenv · 2023-11-03T15:16:47Z

Oh opps... I think that's happening because I removed these lines https://github.com/single-cell-data/TileDB-SOMA/pull/1852/files#diff-e419a62446641fa84b7b42a6ab37141a624cb49a0784add4f49823cd1abef3d5L7-L9. I didn't know what that macro was for or where it was being set, and my tests were passing without it so just kept the change in🤦🏻‍♀️

eddelbuettel · 2023-11-03T16:11:12Z

@nguyenv Per your last comment making that #include conditional again lets me compile the R package. Running tests now, not expecting anything so will commit these two lines in a moment.

codecov-commenter · 2023-11-03T16:20:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

see 40 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

eddelbuettel · 2023-11-03T16:44:27Z

libtiledbsoma/src/soma/column_buffer.h

-    ~ColumnBuffer() {
-        LOG_TRACE(fmt::format("[ColumnBuffer] release '{}'", name_));
-    }
+    ~ColumnBuffer();


Why? While trace is super-voluminous it can be helpful to see that dtors have been reached.

https://github.com/single-cell-data/TileDB-SOMA/pull/1852/files#diff-e563ff857d3a241fa155b2c78fdd243bbe474e8bf28a7782d03f56f00d00f6b5R156-R158

It's because of fmt::format. I've moved it into column_buffer.cc, so we still do a get a log trace.

eddelbuettel

There are a lot of changes here and my head is a little dizzy but I take your word for it that things will work with fmt only in src/ and not the public interface. Thanks for cleaning this up.

A fix was applied upstream to libtiledbsoma, so this is no longer required. It also fixed the runtime error related to a missing symbol from `fmt` single-cell-data/TileDB-SOMA#1852

github-actions · 2023-11-03T19:50:19Z

The backport to release-1.5 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-1.5 release-1.5
# Navigate to the new working tree
cd .worktrees/backport-release-1.5
# Create a new branch
git switch --create backport-1852-to-release-1.5
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 98d30c7380a47b3a2c597630293638c78d0fc7dd
# Push it to GitHub
git push --set-upstream origin backport-1852-to-release-1.5
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-1.5

Then, create a pull request where the base branch is release-1.5 and the compare/head branch is backport-1852-to-release-1.5.

github-actions · 2023-11-03T19:59:31Z

The backport to release-1.5 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-1.5 release-1.5
# Navigate to the new working tree
cd .worktrees/backport-release-1.5
# Create a new branch
git switch --create backport-1852-to-release-1.5
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 98d30c7380a47b3a2c597630293638c78d0fc7dd
# Push it to GitHub
git push --set-upstream origin backport-1852-to-release-1.5
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-1.5

Then, create a pull request where the base branch is release-1.5 and the compare/head branch is backport-1852-to-release-1.5.

* Keep logger.h internal; do not install * Restore conditional include of carrow.h --------- Co-authored-by: Dirk Eddelbuettel <[email protected]>

* Keep logger.h internal; do not install * Restore conditional include of carrow.h --------- Co-authored-by: nguyenv <[email protected]> Co-authored-by: Dirk Eddelbuettel <[email protected]>

Keep logger.h internal; do not install

d423a86

johnkerl added the backport release-1.5 label Nov 3, 2023

johnkerl approved these changes Nov 3, 2023

View reviewed changes

nguyenv requested review from eddelbuettel, johnkerl, beroy, gspowley and teo-tsirpanis November 3, 2023 14:50

teo-tsirpanis approved these changes Nov 3, 2023

View reviewed changes

jdblischak approved these changes Nov 3, 2023

View reviewed changes

Restore conditional include of carrow.h

da9af0a

johnkerl approved these changes Nov 3, 2023

View reviewed changes

eddelbuettel reviewed Nov 3, 2023

View reviewed changes

eddelbuettel approved these changes Nov 3, 2023

View reviewed changes

nguyenv merged commit 98d30c7 into main Nov 3, 2023
12 checks passed

nguyenv deleted the viviannguyen/keep-logger.h-internal branch November 3, 2023 18:50

johnkerl pushed a commit that referenced this pull request Nov 3, 2023

[c++] Remove spdlog requirement (#1852)

af7f1a0

* Keep logger.h internal; do not install * Restore conditional include of carrow.h --------- Co-authored-by: Dirk Eddelbuettel <[email protected]>

johnkerl mentioned this pull request Nov 3, 2023

[Manually backport to release 1.5] [c++] Remove spdlog requirement #1857

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[c++] Remove `spdlog` requirement #1852

[c++] Remove `spdlog` requirement #1852

nguyenv commented Nov 3, 2023 •

edited

Loading

johnkerl commented Nov 3, 2023

teo-tsirpanis Nov 3, 2023

nguyenv Nov 3, 2023

teo-tsirpanis Nov 3, 2023

nguyenv Nov 3, 2023

eddelbuettel Nov 3, 2023

teo-tsirpanis Nov 3, 2023

nguyenv Nov 3, 2023

nguyenv Nov 3, 2023

teo-tsirpanis Nov 3, 2023

nguyenv commented Nov 3, 2023

jdblischak left a comment

johnkerl commented Nov 3, 2023

eddelbuettel commented Nov 3, 2023

nguyenv commented Nov 3, 2023

eddelbuettel commented Nov 3, 2023

codecov-commenter commented Nov 3, 2023 •

edited

Loading

eddelbuettel Nov 3, 2023

nguyenv Nov 3, 2023

eddelbuettel left a comment

github-actions bot commented Nov 3, 2023

github-actions bot commented Nov 3, 2023

		#include "utils/carrow.h"
		#include "utils/logger.h"

[c++] Remove spdlog requirement #1852

[c++] Remove spdlog requirement #1852

Conversation

nguyenv commented Nov 3, 2023 • edited Loading

johnkerl commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

nguyenv commented Nov 3, 2023

jdblischak left a comment

Choose a reason for hiding this comment

johnkerl commented Nov 3, 2023

eddelbuettel commented Nov 3, 2023

nguyenv commented Nov 3, 2023

eddelbuettel commented Nov 3, 2023

codecov-commenter commented Nov 3, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddelbuettel left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2023

github-actions bot commented Nov 3, 2023

[c++] Remove `spdlog` requirement #1852

[c++] Remove `spdlog` requirement #1852

nguyenv commented Nov 3, 2023 •

edited

Loading

codecov-commenter commented Nov 3, 2023 •

edited

Loading