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

Fix std::chrono/date conflict for mac builds with C++20 #22138

Merged
merged 7 commits into from
Sep 20, 2024

Conversation

skottmckay
Copy link
Contributor

@skottmckay skottmckay commented Sep 18, 2024

Description

Fix usage of c++ std::chrono::operator<< in mac builds for wider range of xcode/targets.

Motivation and Context

Resolve #21033

@snnn
Copy link
Member

snnn commented Sep 19, 2024

We may also consider using absl::date, then we can totally get rid of the third-party date library, regardless C++ standard version.
For example:

// Construct an absl::Time from the system clock.
absl::Time t1 = absl::Now();

// When formatting a time, a time zone should be passed.
absl::TimeZone utc =  absl::UTCTimeZone();
std::cout << absl::FormatTime(t1, utc);

@skottmckay
Copy link
Contributor Author

What's the difference between absl::date and the date library from Howard Hinnant that we use? Both are third-party aren't they?

I would have assumed the latter is closer to what is in the C++20 standard given Howard Hinnant is on the C++ standards committee.

@snnn
Copy link
Member

snnn commented Sep 19, 2024

Howard Hinnant is out of maintenance. On the other hand, absl::date is not deprecated, and it is already in the list of ORT's dependencies. Anyway we only use it in very a few places. The change would be local.

snnn
snnn previously approved these changes Sep 19, 2024
@snnn snnn dismissed their stale review September 19, 2024 01:07

You need to run clang-format on the changed files.

@skottmckay
Copy link
Contributor Author

Howard Hinnant is out of maintenance. On the other hand, absl::date is not deprecated, and it is already in the list of ORT's dependencies. Anyway we only use it in very a few places. The change would be local.

I assume that's because it was added to the C++20 standard: https://github.com/HowardHinnant/date#standardization

Ideally we'd only use one date library. If there's other functionality from absl::date we need I'm fine with that being the choice (assuming no significant binary size impact). But if this is all short term and we expect the C++20 features to remove the need for a third part date library I'd leave things as-is for now.

snnn
snnn previously approved these changes Sep 20, 2024
snnn
snnn previously approved these changes Sep 20, 2024
snnn
snnn previously approved these changes Sep 20, 2024
@mszhanyi
Copy link
Contributor

/azp run Linux GPU CI Pipeline

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@snnn
Copy link
Member

snnn commented Sep 20, 2024

/azp run ONNX Runtime Web CI Pipeline

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@snnn snnn merged commit d469283 into main Sep 20, 2024
87 checks passed
@snnn snnn deleted the skottmckay/FixCxx20DataChronoConflict branch September 20, 2024 18:18
@@ -15,7 +15,9 @@
void AppleLogSink::SendImpl(const Timestamp& timestamp, const std::string& logger_id, const Capture& message) {
Copy link
Member

Choose a reason for hiding this comment

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

The file still includes the third-party "date.h" header file, even when timestamp_ns refers to std::chrono

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.

v1.17.0 + requires macOS deployment target >= than 13.3 due to C++ 20
4 participants