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] Update minimum macOS version to 13.3 #3361

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

XanthosXanthopoulos
Copy link
Collaborator

Issue and/or context:
#3154

Notes for Reviewer:
CI broke on a draft PR the above changes fixed it.

@johnkerl
Copy link
Member

@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

CI broke on a draft PR the above changes fixed it.

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 main is green with C++ 20 for several days now ...)

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.74%. Comparing base (ca00d5b) to head (90fc44f).

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     
Flag Coverage Δ
python 85.74% <ø> (+0.09%) ⬆️

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

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

@XanthosXanthopoulos
Copy link
Collaborator Author

This failed #3329 with error 'to_chars' has been explicitly marked unavailable here caused by std::format

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.

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.

@johnkerl
Copy link
Member

Excellent feedback @jdblischak -- @XanthosXanthopoulos I think we should hold off merging this PR until we do a health-check PR on tiledbsoma-feedstock using a recipe/meta.yml that points at this PR's commit on TileDB-SOMA. (You can do that if you know how; otherwise I'm happy to.)

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 .....

@XanthosXanthopoulos
Copy link
Collaborator Author

I don't know how to do the conda feedstock.
Regarding the different behavior on the PR vs. the nightly, I haven't been able to find anything. Even on the same PR some macos workflows built successfully which is even more baffling.

@johnkerl
Copy link
Member

@johnkerl
Copy link
Member

@XanthosXanthopoulos #3329 is now passing CI -- ?

@XanthosXanthopoulos
Copy link
Collaborator Author

@XanthosXanthopoulos #3329 is now passing CI -- ?

With the changes of this PR yes. I will try to remove the changes and test again

@XanthosXanthopoulos
Copy link
Collaborator Author

The failure is #3329 was triggered by including these 4 lines in libtiledbsoma

#include "soma/soma_column.h"
#include "soma/soma_attribute.h"
#include "soma/soma_dimension.h"
#include "soma/soma_geometry_column.h"

@jdblischak
Copy link
Collaborator

My only concern is from a conda-forge feedstock maintenance perspective:

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.

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