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

[azure-core-cpp] Fix gcc13 compilation error #34393

Merged
merged 32 commits into from
Oct 31, 2023

Conversation

jimwang118
Copy link
Contributor

@jimwang118 jimwang118 commented Oct 10, 2023

Fixes #34272
/vcpkg/buildtrees/azure-core-cpp/src/ore_1.10.2-d563a045e8.clean/_/_/_/inc/azure/core/base64.hpp:42:55: error: ‘uint8_t’ was not declared in this scope
Add stdint.h header file, fixed via PR 5031 upstream.

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

Usage test pass with following triplets:

x64-linux

@jimwang118 jimwang118 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Oct 10, 2023
@BillyONeal
Copy link
Member

@LarryOsterman FYI

@jimwang118 jimwang118 marked this pull request as ready for review October 11, 2023 02:22
@LarryOsterman
Copy link
Member

Due to the way that the Azure SDK for C++ publishes its packages, all changes made to the vcpkg repository will be discarded when the Azure SDK for C++ is next released. As such, making changes to fix issues in the vcpkg repository are always ephemeral and should probably be avoided.

The Azure SDK is located at https://github.com/Azure/azure-sdk-for-cpp and we welcome community contributions like this one.

FWIW, we currently have an outstanding issue regarding this specific change: Azure/azure-sdk-for-cpp#5007.

@jimwang118
Copy link
Contributor Author

Due to the way that the Azure SDK for C++ publishes its packages, all changes made to the vcpkg repository will be discarded when the Azure SDK for C++ is next released. As such, making changes to fix issues in the vcpkg repository are always ephemeral and should probably be avoided.

The Azure SDK is located at https://github.com/Azure/azure-sdk-for-cpp and we welcome community contributions like this one.

FWIW, we currently have an outstanding issue regarding this specific change: Azure/azure-sdk-for-cpp#5007.

Okay, then I will mark this PR as draft, wait for you to fix it, and then update it.

@jimwang118 jimwang118 marked this pull request as draft October 12, 2023 01:45
@jimwang118 jimwang118 added the depends:upstream-changes Waiting on a change to the upstream project label Oct 12, 2023
@LarryOsterman
Copy link
Member

LarryOsterman commented Oct 12, 2023

Due to the way that the Azure SDK for C++ publishes its packages, all changes made to the vcpkg repository will be discarded when the Azure SDK for C++ is next released. As such, making changes to fix issues in the vcpkg repository are always ephemeral and should probably be avoided.
The Azure SDK is located at https://github.com/Azure/azure-sdk-for-cpp and we welcome community contributions like this one.
FWIW, we currently have an outstanding issue regarding this specific change: Azure/azure-sdk-for-cpp#5007.

Okay, then I will mark this PR as draft, wait for you to fix it, and then update it.

So I took a look at the issue. The base64.hpp file already includes cstdint which is supposed to include stdint.h.

The version of cstdint on my machine (in /usr/include/x86_64-linux-gnu/c++/11) does include stdint.h. Are you using a different C++ runtime library?

EDIT: I think I figured it out. cstdint only defines the std::uint8_t type (and friends), stdint.h defines the uint8_t types in the global namespace. most of the time, cstdint also includes stdint.h, but It appears that the version of the runtime library that you are using does not (possibly because of C++ module support). I've added the header to the source code (and updated our CI pipelines to add clang-13 and clang-15: Azure/azure-sdk-for-cpp#5031.

@BillyONeal
Copy link
Member

EDIT: I think I figured it out. cstdint only defines the std::uint8_t type (and friends), stdint.h defines the uint8_t types in the global namespace.

Right, <cfoo> means "use std::foo", <foo.h> means "use ::foo".

@LarryOsterman
Copy link
Member

EDIT: I think I figured it out. cstdint only defines the std::uint8_t type (and friends), stdint.h defines the uint8_t types in the global namespace.

Right, <cfoo> means "use std::foo", <foo.h> means "use ::foo".

What complicates this is that many (most?) implementations of cstdint include stdint.h (including the version of the CRT used in our CI pipelines). The version of cstdint that the reporters are using do not. That is why even after selecting clang-13 (and clang-15) we didn't see a problem.

@jimwang118 jimwang118 removed the depends:upstream-changes Waiting on a change to the upstream project label Oct 24, 2023
@jimwang118 jimwang118 marked this pull request as ready for review October 24, 2023 07:57
LilyWangLL
LilyWangLL previously approved these changes Oct 25, 2023
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Oct 25, 2023
@data-queue
Copy link
Contributor

Can you fix merge conflicts? And I think we can use the upstream patch since it merged

@data-queue data-queue removed the info:reviewed Pull Request changes follow basic guidelines label Oct 28, 2023
@data-queue data-queue marked this pull request as draft October 28, 2023 01:28
@jimwang118
Copy link
Contributor Author

Can you fix merge conflicts? And I think we can use the upstream patch since it merged

OK, I'll resolve the merge conflict and fix it with the upstream patch.

@jimwang118 jimwang118 marked this pull request as ready for review October 31, 2023 05:52
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Oct 31, 2023
@JavierMatosD JavierMatosD merged commit ec19a4a into microsoft:master Oct 31, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[azure-core] build failure with gcc 13
6 participants