Skip to content

Fix MacOS build for Xcode 16.3 #956

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

Merged
merged 5 commits into from
Apr 25, 2025
Merged

Fix MacOS build for Xcode 16.3 #956

merged 5 commits into from
Apr 25, 2025

Conversation

figueroa1395
Copy link
Contributor

As mentioned in Xcode 16.3 release notes: The base template for std::char_traits has been removed. If you are using std::char_traits with types other than char, wchar_t, char8_t, char16_t, char32_t or a custom character type for which you specialized std::char_traits, your code will stop working. The Standard does not mandate that a base template is provided, and such a base template is bound to be incorrect for some types, which could previously cause unexpected behavior while going undetected.

This affects us indirectly as we use nlohmann/json and they still use std::char_traits, resulting in failed compilation attempt (only for MacOS devs with Xcode16.3) due to the usage of std::vector<std::byte> as an argument of nlohmann::ordered_json::from_msgpack() in our tests.

In this PR, we modify our tests such that this is not an issue anymore.

Note: There was a recent release of the library, but it didn't address this issue.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 added dependencies Pull requests that update a dependency file improvement Improvement on internal implementation labels Apr 16, 2025
@figueroa1395 figueroa1395 self-assigned this Apr 16, 2025
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 marked this pull request as ready for review April 16, 2025 09:20
mgovers
mgovers previously approved these changes Apr 16, 2025
@TonyXiang8787
Copy link
Member

@figueroa1395 did you raise issue at nlohmann/json board?

@figueroa1395
Copy link
Contributor Author

@figueroa1395 did you raise issue at nlohmann/json board?

I haven't, but I will. I will post here when I do.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@mgovers mgovers marked this pull request as draft April 17, 2025 12:22
@mgovers
Copy link
Member

mgovers commented Apr 17, 2025

Converted back to Draft because not all issues have been resolved yet. Sustainable solution will take a bit longer.

@figueroa1395
Copy link
Contributor Author

Issue created in nlohmann/json#4756

@TonyXiang8787
Copy link
Member

TonyXiang8787 commented Apr 23, 2025

@mgovers @figueroa1395

If I understand correctly, we just need to change from std::vector<std::byte> to std::vector<char> in the following line, the problem is going away right?

std::vector<std::byte> msgpack_data{};

@figueroa1395
Copy link
Contributor Author

@mgovers @figueroa1395

If I understand correctly, we just need to change from std::vector<std::byte> to std::vector<char> in the following line, the problem is going away right?

std::vector<std::byte> msgpack_data{};

@TonyXiang8787 Yes, and that was my first quick and naive solution. However, if we just do that, I think we would be taking a step back. At the CPP API level we should be able to use std::byte when handling raw data, not just go back to char which can also be used for characters or even numbers.

Apparently there is someone interested in implementing a PR to solve my issue in nlohmman/json, so we can just wait a bit more there. In any case, I'll be giving my input as well. I just didn't have the time to do the PR myself.

@TonyXiang8787
Copy link
Member

TonyXiang8787 commented Apr 24, 2025

@mgovers @figueroa1395
If I understand correctly, we just need to change from std::vector<std::byte> to std::vector<char> in the following line, the problem is going away right?

std::vector<std::byte> msgpack_data{};

@TonyXiang8787 Yes, and that was my first quick and naive solution. However, if we just do that, I think we would be taking a step back. At the CPP API level we should be able to use std::byte when handling raw data, not just go back to char which can also be used for characters or even numbers.

Apparently there is someone interested in implementing a PR to solve my issue in nlohmman/json, so we can just wait a bit more there. In any case, I'll be giving my input as well. I just didn't have the time to do the PR myself.

@figueroa1395 @mgovers
We do not have to change the CPP API. But just cast the pointer to char const*. It will then solve the problem. So change the lines:

std::vector<std::byte> msgpack_data{};
msgpack_serializer.get_to_binary_buffer(0, msgpack_data);
auto const json_document = nlohmann::ordered_json::from_msgpack(msgpack_data);

To:

                std::vector<std::byte> msgpack_data{};
                msgpack_serializer.get_to_binary_buffer(0, msgpack_data);
                auto const char_start = reintepret_cast<unsigned char const*>(msgpack_data.data());
                auto const char_end = char_start + msgpack_data.size();
                auto const json_document = nlohmann::ordered_json::from_msgpack(char_start, char_end);

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395
Copy link
Contributor Author

@mgovers @figueroa1395
If I understand correctly, we just need to change from std::vector<std::byte> to std::vector<char> in the following line, the problem is going away right?

std::vector<std::byte> msgpack_data{};

@TonyXiang8787 Yes, and that was my first quick and naive solution. However, if we just do that, I think we would be taking a step back. At the CPP API level we should be able to use std::byte when handling raw data, not just go back to char which can also be used for characters or even numbers.
Apparently there is someone interested in implementing a PR to solve my issue in nlohmman/json, so we can just wait a bit more there. In any case, I'll be giving my input as well. I just didn't have the time to do the PR myself.

@figueroa1395 @mgovers We do not have to change the CPP API. But just cast the pointer to char const*. It will then solve the problem. So change the lines:

std::vector<std::byte> msgpack_data{};
msgpack_serializer.get_to_binary_buffer(0, msgpack_data);
auto const json_document = nlohmann::ordered_json::from_msgpack(msgpack_data);

To:

                std::vector<std::byte> msgpack_data{};
                msgpack_serializer.get_to_binary_buffer(0, msgpack_data);
                auto const char_start = reintepret_cast<unsigned char const*>(msgpack_data.data());
                auto const char_end = char_start + msgpack_data.size();
                auto const json_document = nlohmann::ordered_json::from_msgpack(char_start, char_end);

I implemented it in c3609a8. I'm fine with this local fix here until nlohmann/json#4756 is addressed and released. @mgovers any thoughts?

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@mgovers
Copy link
Member

mgovers commented Apr 25, 2025

@mgovers @figueroa1395
If I understand correctly, we just need to change from std::vector<std::byte> to std::vector<char> in the following line, the problem is going away right?

std::vector<std::byte> msgpack_data{};

@TonyXiang8787 Yes, and that was my first quick and naive solution. However, if we just do that, I think we would be taking a step back. At the CPP API level we should be able to use std::byte when handling raw data, not just go back to char which can also be used for characters or even numbers.
Apparently there is someone interested in implementing a PR to solve my issue in nlohmman/json, so we can just wait a bit more there. In any case, I'll be giving my input as well. I just didn't have the time to do the PR myself.

@figueroa1395 @mgovers We do not have to change the CPP API. But just cast the pointer to char const*. It will then solve the problem. So change the lines:

std::vector<std::byte> msgpack_data{};
msgpack_serializer.get_to_binary_buffer(0, msgpack_data);
auto const json_document = nlohmann::ordered_json::from_msgpack(msgpack_data);

To:

                std::vector<std::byte> msgpack_data{};
                msgpack_serializer.get_to_binary_buffer(0, msgpack_data);
                auto const char_start = reintepret_cast<unsigned char const*>(msgpack_data.data());
                auto const char_end = char_start + msgpack_data.size();
                auto const json_document = nlohmann::ordered_json::from_msgpack(char_start, char_end);

I implemented it in c3609a8. I'm fine with this local fix here until nlohmann/json#4756 is addressed and released. @mgovers any thoughts?

Ok!

@TonyXiang8787 TonyXiang8787 marked this pull request as ready for review April 25, 2025 13:55
@TonyXiang8787 TonyXiang8787 added this pull request to the merge queue Apr 25, 2025
Merged via the queue into main with commit 645dbf9 Apr 25, 2025
29 of 30 checks passed
@TonyXiang8787 TonyXiang8787 deleted the fix-xcode16.3 branch April 25, 2025 14:14
@figueroa1395
Copy link
Contributor Author

@mgovers @TonyXiang8787 FYI: The issue I created relevant to this has already been closed as completed via nlohmann/json#4760. Now we have to wait until they make their next release. Since that may take very long, should I create a simple issue here to keep track of it for once it gets released on nlohmann/json's side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file improvement Improvement on internal implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants