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.

Sign up for GitHub

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

Jump to bottom

Fix build errors from date/date.h C++20 compatibility #20139

Merged
merged 4 commits into from
Apr 3, 2024
Merged

Fix build errors from date/date.h C++20 compatibility #20139

merged 4 commits into from
Apr 3, 2024

Conversation

afantino951
Copy link
Contributor

Description

For C++ standards >= 20, use std::chrono::operator<< in place of date::operator<< to fix ambiguous operator compile error.

Motivation and Context

The external dependency HowardHinnant/date has a conflict with std::chrono for >=C++20.
Solves #20137

@afantino951
Copy link
Contributor Author 

@microsoft-github-policy-service agree

@microsoft-github-policy-service agree

@edgchen1
Copy link
Contributor

/azp run MacOS CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@edgchen1
Copy link
Contributor

Thanks for the PR.

Unfortunately, this doesn't work for older Xcode versions. We use Xcode 14.2 in our CI build now.

/Users/runner/work/1/s/onnxruntime/core/common/logging/sinks/ostream_sink.cc:26:23: error: no member named 'operator<<' in namespace 'std::chrono'
  using timestamp_ns::operator<<;
        ~~~~~~~~~~~~~~^

Looks like older implementations of the C++20 standard library in Xcode don't support std::chrono::operator<<.

Maybe add an Xcode version check?

@afantino951
Copy link
Contributor Author

afantino951 commented Mar 29, 2024 •
edited
Loading

/azp run MacOS CI Pipeline

Edit: nvm :)

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 20139 in repo microsoft/onnxruntime

1 similar comment
@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 20139 in repo microsoft/onnxruntime

@tianleiwu
Copy link
Contributor

/azp run MacOS CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tianleiwu
Copy link
Contributor

/azp run Windows ARM64 QNN CI Pipeline,Windows x64 QNN CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,ONNX Runtime Web CI Pipeline,Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline

@tianleiwu
Copy link
Contributor

/azp run Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,onnxruntime-binary-size-checks-ci-pipeline,Big Models,Android CI Pipeline

@tianleiwu
Copy link
Contributor

/azp run iOS CI Pipeline,ONNX Runtime React Native CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 10 pipeline(s).

1 similar comment
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 10 pipeline(s).

@afantino951
Copy link
Contributor Author

@tianleiwu,

Thank you for running the CI tests for my change. Are the React Native CI Pipeline tests correct for this? I checked the logs, but didn't understand how my change could affect the React Native code.

tianleiwu
tianleiwu previously approved these changes Apr 1, 2024
View reviewed changes
@tianleiwu
Copy link
Contributor

/azp run Windows ARM64 QNN CI Pipeline,Windows x64 QNN CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,ONNX Runtime Web CI Pipeline,Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline

@tianleiwu
Copy link
Contributor

/azp run Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,onnxruntime-binary-size-checks-ci-pipeline,Big Models,Android CI Pipeline

@tianleiwu
Copy link
Contributor

/azp run iOS CI Pipeline,ONNX Runtime React Native CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 10 pipeline(s).

1 similar comment
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 10 pipeline(s).

tianleiwu
tianleiwu previously approved these changes Apr 2, 2024
View reviewed changes
@tianleiwu
Copy link
Contributor

/azp run Windows ARM64 QNN CI Pipeline,Windows x64 QNN CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,ONNX Runtime Web CI Pipeline,Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline

@tianleiwu
Copy link
Contributor

/azp run Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,onnxruntime-binary-size-checks-ci-pipeline,Big Models,Android CI Pipeline

@tianleiwu
Copy link
Contributor

/azp run iOS CI Pipeline,ONNX Runtime React Native CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 10 pipeline(s).

@tianleiwu
Copy link
Contributor

/azp run ONNX Runtime Web CI Pipeline,Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,onnxruntime-binary-size-checks-ci-pipeline,Big Models

@tianleiwu
Copy link
Contributor

/azp run Android CI Pipeline,iOS CI Pipeline,ONNX Runtime React Native CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 10 pipeline(s).

@tianleiwu tianleiwu merged commit 7303a90 into microsoft:main Apr 3, 2024
79 of 82 checks passed
@IvanPizhenko IvanPizhenko mentioned this pull request Apr 22, 2024
Closed
rvinluan-sidefx pushed a commit to sideeffects/onnxruntime that referenced this pull request Apr 26, 2024
@afantino951
Fix build errors from date/date.h C++20 compatibility (microsoft#20139)
528b8a5 
### Description
For C++ standards >= 20, use `std::chrono::operator<<` in place of
`date::operator<<` to fix ambiguous operator compile error.

### Motivation and Context
The external dependency HowardHinnant/date has a conflict with
std::chrono for >=C++20.
Solves microsoft#20137
TedThemistokleous pushed a commit to TedThemistokleous/onnxruntime that referenced this pull request May 7, 2024
@afantino951
Fix build errors from date/date.h C++20 compatibility (microsoft#20139)
dc0da69 
### Description
For C++ standards >= 20, use `std::chrono::operator<<` in place of
`date::operator<<` to fix ambiguous operator compile error.

### Motivation and Context
The external dependency HowardHinnant/date has a conflict with
std::chrono for >=C++20.
Solves microsoft#20137
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edgchen1 edgchen1 edgchen1 approved these changes

@tianleiwu tianleiwu tianleiwu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@afantino951 @edgchen1 @tianleiwu