-
Notifications
You must be signed in to change notification settings - Fork 25
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] Update minimum macOS version to 13.3 #3361
base: main
Are you sure you want to change the base?
Conversation
@XanthosXanthopoulos there are subtleties with MacOS with "10" and "11" which aren't as simple as what one might think. Looping in @jdblischak for extra eyes. Re
please share here the link to which draft PR, and which failed CI. (I'm curious to know why that draft PR failed but our |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3361 +/- ##
==========================================
+ Coverage 85.64% 85.74% +0.09%
==========================================
Files 57 57
Lines 6201 6201
==========================================
+ Hits 5311 5317 +6
+ Misses 890 884 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This failed #3329 with error |
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.
My only concern is from a conda-forge feedstock maintenance perspective:
- We can require a newer macOS SDK in the feedstock builds, but it should be done with caution:
While the default version 10.13 can be overridden using the following changes to the recipe, it should be done as a last resort.
- I checked, and there are already 50 conda-forge feedstocks that set
MACOSX_SDK_VERSION
to 13.3 - The nightly feedstock builds can currently successfully build the Python clients using C++20 and a macOS SDK version of 11 (Nightly feedstock build failed TileDB-Inc/tiledbsoma-feedstock#230 (comment)). And the code in
main
already usesstd::format
, so what is different about the use ofstd::format
in [c++]SOMAColumn
part 1 #3329? - If we decide to bump to 13.3, we need to update
MACOSX_SDK_VERSION
(and possiblyc_stdlib_version
, though we should first try without bumping it) inconda_build_config.yaml
and rerendering
Excellent feedback @jdblischak -- @XanthosXanthopoulos I think we should hold off merging this PR until we do a health-check PR on Context is we're close to a release, and all releases go through Conda constraints, and there are tricky shoals to navigate on the Conda seas ..... |
I don't know how to do the conda feedstock. |
@XanthosXanthopoulos #3329 is now passing CI -- ? |
With the changes of this PR yes. I will try to remove the changes and test again |
The failure is #3329 was triggered by including these 4 lines in libtiledbsoma
|
Quick update. We discovered that we will already have to bump the macOS SDK in the conda feedstock builds to 13.3 in order to accommodate the migration to C++20 (#3154) xref: TileDB-Inc/tiledbsoma-feedstock#230 (comment) In other words, we are going to have to migrate the feedstock to 13.3 regardless of the status of the PR #3329, so I'm fine with this PR. |
Issue and/or context:
#3154
Notes for Reviewer:
CI broke on a draft PR the above changes fixed it.