-
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
C++20 r-build #246
base: main
Are you sure you want to change the base?
C++20 r-build #246
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome @XanthosXanthopoulos! It's amazing to finally see some green CI for the C++20 builds
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 | ||
export CXX20=$RECIPE_DIR/cxx_wrap.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to define CXX20
here when it is overwritten below?
@@ -1,4 +1,4 @@ | |||
#!/bin/sh | |||
|
|||
args="${@##-mmacosx-version-min=10.9*}" | |||
$NN_CC_ORIG $args -mmacosx-version-min=11.0 | |||
args="${@##-mmacosx-version-min=13.3*}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the version in these 2 lines shouldn't match. My understanding of the purpose of the wrapper script is to remove the original value of -mmacosx-version-min
and replace it with a new one. If the script is replacing the exact same string, we can probably remove the wrapper script all-together.
MACOSX_SDK_VERSION: # [osx and x86_64] | ||
- 11.0 # [osx and x86_64] | ||
MACOSX_SDK_VERSION: # [osx and x86_64] | ||
- 13.3 # [osx and x86_64] | ||
c_stdlib_version: # [osx and x86_64] | ||
- 11.0 # [osx and x86_64] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was concerned that these didn't match, but I think this is correct. From the conda-forge docs, we don't need to bump c_stdlib_version
as long as we're not getting additional errors.
…nda-forge-pinning 2025.01.10.08.48.29
…nda-forge-pinning 2025.01.10.08.48.29
@XanthosXanthopoulos my commits 7,8,9 are as noted in Slack |
After switching to C++20 the R build failed with an cryptic memory error message during build (see #230). The culprit seems to be
-fvisibility-inlines-hidden
, where the build succeeds once the flag is removed. With C++17 there is no issue with that flag and I haven't succeeded in a local repro of the issue outside of a docker container.