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++/python/r] Migrate to C++20 #3331

Merged
merged 11 commits into from
Nov 19, 2024
Merged

Conversation

XanthosXanthopoulos
Copy link
Collaborator

@XanthosXanthopoulos XanthosXanthopoulos commented Nov 18, 2024

This PR migrates minimum C++ version to C++20 and replaces fmt::format with the now available std::format

Context: #3154 [sc-57301]

Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.51%. Comparing base (946d3f7) to head (73efdbf).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3331      +/-   ##
==========================================
+ Coverage   85.14%   85.51%   +0.36%     
==========================================
  Files          53       54       +1     
  Lines        5568     5703     +135     
==========================================
+ Hits         4741     4877     +136     
+ Misses        827      826       -1     
Flag Coverage Δ
python 85.51% <ø> (+0.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api 85.51% <ø> (+0.36%) ⬆️
libtiledbsoma ∅ <ø> (∅)
---- 🚨 Try these New Features:

Copy link
Member

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

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

I know this is in draft and you haven't submitted it for review yet. :)

However -- CI is green and this is delightful work. I'm approving it in advance. Especially with regard to timezone alignment.

Feel free to merge this when you're ready.

Thanks again for doing this!!! :)

@johnkerl
Copy link
Member

johnkerl commented Nov 18, 2024

One note and cc @mojaveazure -- given that there are compile errors if RcppSpdlog is on a user's system and they have not updated -- I'd love a lower bound in apis/r/DESCRIPTION like RcppSpdlog (>= 0.0.19) -- however -- this is only a LinkingTo not an Imports

RcppSpdlog,
... is there a good way to get some automation around upgrading RcppSpdlog when users upgrade their tiledbsoma version (once we tag 1.15.0)?

@mojaveazure
Copy link
Member

mojaveazure commented Nov 18, 2024

this is only a LinkingTo not an Imports

I don't think this matters. In R, the "hard" dependencies are Depends, Imports, and LinkingTo; these must be present and at the specified minimum version before the package can be installed. If we bump RcppSpdlog to >= 0.0.19, any user who does install.packages("tiledbsoma") or update.packages() will be required to install the newer version of RcppSpdlog before tiledbsoma 1.15.0 can be installed

Moreover, R and CRAN allow all types of dependencies (Depends, Imports, LinkingTo, Suggests, and Enhances) to have lower bounds (though only the first three have a hard check, manual checks must be carried out for Suggests and Enhances). Case in point, Seurat links to Rcpp >= 0.11.0

@johnkerl
Copy link
Member

OK thank you @mojaveazure !

I did get an error on my laptop until I upgraded RcppSpdlog.

So since @mojaveazure asserts that LinkingTo can have a lower bound, I think let's please put in a lower bound at 0.0.19 for RcppSpglog.

@XanthosXanthopoulos XanthosXanthopoulos marked this pull request as ready for review November 19, 2024 10:18
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

Successfully merging this pull request may close these issues.

3 participants