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

Build r-tiledbsoma with C++ 20 #231

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Build r-tiledbsoma with C++ 20 #231

wants to merge 2 commits into from

Conversation

johnkerl
Copy link
Collaborator

@jdblischak
Copy link
Collaborator

For some reason the osx-64 build isn't using C++20. The errors are the same as in the nightly build (#230 (comment))

Copy link
Collaborator

@jdblischak jdblischak left a comment

Choose a reason for hiding this comment

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

@johnkerl could you please rebase your PR onto the latest from main to include #228?

@jdblischak
Copy link
Collaborator

For some reason the osx-64 build isn't using C++20

Well I guess this makes sense. The osx-64 build is the most complicated since it is the only one that uses the wrapper functions:

# https://github.com/conda-forge/r-tiledb-feedstock/commit/29cb6816636e7b5b58545e1407a8f0c29ff9dc39
if [[ $target_platform == osx-64 ]]; then
export NN_CXX_ORIG=$CXX
export NN_CC_ORIG=$CC
export CXX=$RECIPE_DIR/cxx_wrap.sh
export CC=$RECIPE_DIR/cc_wrap.sh
mkdir -p ~/.R
echo CC=$RECIPE_DIR/cc_wrap.sh > ~/.R/Makevars
echo CXX=$RECIPE_DIR/cxx_wrap.sh >> ~/.R/Makevars
echo CXX17=$RECIPE_DIR/cxx_wrap.sh >> ~/.R/Makevars
fi

These wrapper scripts were initially added to handle C++17 requirements (conda-forge/r-tiledb-feedstock@29cb681, conda-forge/r-tiledb-feedstock#27). Maybe C++20 has different requirements?

@jdblischak
Copy link
Collaborator

Ah, looking earlier in the build log, I see we have a nice diagnostic signal. R explicitly tells us it is using C++17 on osx-64.

* installing *source* package ‘tiledbsoma’ ...
** using staged installation
** updated src/Makevars for system library via pkg-config
** libs
using C++ compiler: ‘clang version 18.1.8’
using C++17
using SDK: ‘NA’‘NA’‘NA’‘’

Bizarrely though, the osx-arm64 build also reports using C++17, so unclear why it is able to compile successfully.

@johnkerl
Copy link
Collaborator Author

Commit 3:

mamba upgrade -c conda-forge conda-smithy -n base
conda smithy rerender --commit auto

@jdblischak
Copy link
Collaborator

I think it is going to be tough to build r-tiledbsoma with C++20 without the upstream changes in single-cell-data/TileDB-SOMA#3331 that updated configure and src/Makevars.in.

If we want to fix the nightly build, I think we'll need to 1) fix the build on the branch nightly-build, and then 2) modify nightly/update-recipe.sh to incorporate these changes.

@johnkerl
Copy link
Collaborator Author

@jdblischak I was confused

single-cell-data/TileDB-SOMA#3331 is merged to main of TileDB-SOMA & I was confusing this PR with the centralized-nightlies which should be testing the day's main.

Let me take a look at nightly-build and nightly/update-recipe.sh which, embarrassingly, I'm not at all familiar with ...

@jdblischak
Copy link
Collaborator

I was confusing this PR with the centralized-nightlies which should be testing the day's main.

Good news! The TileDB-SOMA-R build in centralized-tiledb-nightlies is using C++20 and building without error. Thus if we can get the nightly feedstock build to also use C++20, it should just work.

@johnkerl
Copy link
Collaborator Author

Awesome, thanks @jdblischak ! I'm still working on what I need to type to get us there.

@johnkerl johnkerl requested a review from jdblischak November 22, 2024 01:58
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.

2 participants