MacOS Discord Integration Crash Fix

[Ericzklm]

Set locale on MacOS

This resolves a crash that occurs when Discord Integration on MacOS sends a message to Discord when the app is opened via the bundle. The environment when opening the app via bundle differs from when it's opened via the executable and may be missing certain variables that prevent the locale from being automatically set.

Stacktrace:

0   libsystem_kernel.dylib        	       0x193492600 __pthread_kill + 8
1   libsystem_pthread.dylib       	       0x1934caf70 pthread_kill + 288
2   libsystem_c.dylib             	       0x1933d7908 abort + 128
3   libc++abi.dylib               	       0x19348144c abort_message + 132
4   libc++abi.dylib               	       0x19346fa24 demangling_terminate_handler() + 320
5   libobjc.A.dylib               	       0x1931153f4 _objc_terminate() + 172
6   libc++abi.dylib               	       0x193480710 std::__terminate(void (*)()) + 16
7   libc++abi.dylib               	       0x193483f8c __cxa_rethrow + 204
8   libc++.1.dylib                	       0x19342d6d4 std::__1::locale::__imp::__imp(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, unsigned long) + 2320
9   libc++.1.dylib                	       0x19342f148 std::__1::locale::locale(char const*) + 92
10  libdpp.10.0.22.dylib          	       0x10f15f850 crossplatform_strptime(char const*, char const*, tm*) + 128
11  libdpp.10.0.22.dylib          	       0x10f1613e4 dpp::ts_not_null(nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char>>> const*, char const*) + 324
12  libdpp.10.0.22.dylib          	       0x10f19f938 dpp::message::fill_from_json(nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char>>>*, dpp::cache_policy_t) + 5932
13  libdpp.10.0.22.dylib          	       0x10f10ba38 std::__1::__function::__func<dpp::cluster::message_create(dpp::message const&, std::__1::function<void (dpp::confirmation_callback_t const&)>)::$_0, std::__1::allocator<dpp::cluster::message_create(dpp::message const&, std::__1::function<void (dpp::confirmation_callback_t const&)>)::$_0>, void (nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char>>>&, dpp::http_request_completion_t const&)>::operator()(nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char>>>&, dpp::http_request_completion_t const&) + 96
14  libdpp.10.0.22.dylib          	       0x10f0cbec0 std::__1::__function::__func<dpp::cluster::post_rest_multipart(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, dpp::http_method, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::function<void (nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char>>>&, dpp::http_request_completion_t const&)>, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>> const&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>> const&)::$_0, std::__1::allocator<dpp::cluster::post_rest_multipart(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, dpp::http_method, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::function<void (nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char>>>&, dpp::http_request_completion_t const&)>, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>> const&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>> const&)::$_0>, void (dpp::http_request_completion_t const&)>::operator()(dpp::http_request_completion_t const&) + 240
15  libdpp.10.0.22.dylib          	       0x10f1a98c8 dpp::request_queue::out_loop() + 516
16  libdpp.10.0.22.dylib          	       0x10f1aad9c void* std::__1::__thread_proxy[abi:ne180100]<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, void (dpp::request_queue::*)(), dpp::request_queue*>>(void*) + 72
17  libsystem_pthread.dylib       	       0x1934cb2e4 _pthread_start + 136
18  libsystem_pthread.dylib       	       0x1934c60fc thread_start + 8

[Ericzklm]

On the x86 runner, some warnings are present with version 10.0.22 of DPP library so those warnings are waived to allow compilation with Discord Integration enabled.

Shorten-64-to-32 warning is due to this

template <uint32_t> uint32_t from_string(const std::string &s)
{
	return std::stoul(s, 0, 10);
}

Which truncates a 64 bit unsigned long into 32 bit unsigned int

Unused parameter warning is due to trivial base implementations like this

T& fill_from_json(nlohmann::json* j) {
	throw dpp::logic_exception("JSON interface doesn't implement parse_from_json");
}

[Gin890]

Thanks for finding those errors! I feel it's better to fix DPP than to add one more compiler flag variations! Let me discuss with DPP author on what to do with that.

As for the language locale, I don't know much about it. Would this added language setting have any problem on non-English environment for non-English-speaking users?

[Ericzklm]

Does SerialPrograms have any support for other languages besides english? I guess there might be a difference between US english and other variants (MMDDYYYY vs DDMMYYYY, and such). I think at the moment, the bundle should be using the US format so this would be consistent with that.

Some of these issues have already been fixed in DPP in later versions so this would likely tie back to #753

[Gin890]

Looking at the files changed, do we still need a different compiler flag for arm and non-arm macOS?

[Ericzklm]

x86 will throw warnings if you have -Wshorten-64-to-32 and if you dont have -Wno-unused-parameter. Both arm and non-arm can have the same compiler flags (dont include -Wshorten-64-to-32 and include -Wno-unused-parameter) but that would make the arm build more permissive with those warnings removed

[Gin890]

Kuro's philosophy is to have as many warnings as errors as possible. What about having both -Wshorten-64-to-32 and -Wunused-parameter? I can fix the warnings on x86. Can you point to how to check for those warnings on Github?

[Ericzklm]

To reproduce the issue, you can checkout this pull request from the deployment CI: https://github.com/PokemonAutomation/AutoBuildRelease/pull/8/files

Then kick off the workflow, the defaults with the main branch and any tag should do.

My question is if there's anything that can be done to resolve the warnings from Arduino-Source? These come from the compilation of the DPP 10.0.22 library source code and my understanding was that the options are to waive the warnings, update DPP (not sure if that will even fix everything), or write our own patches for version 10.0.22 of DPP

[naussika]

theoretically those warnings should have been suppressed already by SYSTEM keyword:

target_include_directories(${target_name} SYSTEM PRIVATE ../3rdParty/)

how about changing one line below:

pkg_search_module(DPP dpp=10.0.22)
    if (DPP_FOUND) # enable dpp integration
        target_compile_definitions(SerialProgramsLib PUBLIC PA_DPP)
        ## to build against headers within this repo and properly set flags ##
        ## don't enable the following line ##
        # target_include_directories(SerialProgramsLib PRIVATE ${DPP_INCLUDE_DIRS})
        target_link_directories(SerialProgramsLib PUBLIC ${DPP_LIBRARY_DIRS})
        target_link_libraries(SerialProgramsLib PUBLIC ${DPP_LIBRARIES})
    endif()

to include homebrew one but with SYSTEM:

target_include_directories(SerialProgramsLib SYSTEM PRIVATE ${DPP_INCLUDE_DIRS})

[Ericzklm]

I've tried this, let me know if I misunderstood your comment.

    if (DPP_FOUND) # enable dpp integration
        target_compile_definitions(SerialProgramsLib PUBLIC PA_DPP)
        ## to build against headers within this repo and properly set flags ##
        target_include_directories(SerialProgramsLib SYSTEM PRIVATE ${DPP_INCLUDE_DIRS})
        target_link_directories(SerialProgramsLib PUBLIC ${DPP_LIBRARY_DIRS})
        target_link_libraries(SerialProgramsLib PUBLIC ${DPP_LIBRARIES})
    endif()

The warnings are still present unfortunately, possibly because it's not apart of the headers and is a template in DPP code

[naussika]

on arm macOS, building would be failed without SYSTEM keyword target_include_directories(SerialProgramsLib PRIVATE ${DPP_INCLUDE_DIRS})

In file included from /opt/homebrew/Cellar/libdpp@10.0.22/10.0.22/include/dpp/dpp.h:37:
/opt/homebrew/Cellar/libdpp@10.0.22/10.0.22/include/dpp/stringops.h:168:9: error: implicit conversion loses integer precision: 'unsigned long' to 'uint32_t' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
  168 |         return std::stoul(s, 0, 10);
      |         ~~~~~~ ^~~~~~~~~~~~~~~~~~~~

and with SYSTEM keyword, cmake emits -isystem /opt/homebrew/Cellar/libdpp@10.0.22/10.0.22/include instead of -I/opt/homebrew/Cellar/libdpp@10.0.22/10.0.22/include.

Can you open compile_commands.json file in build directory to verify that?

[naussika]

but if the compiler on x64 doesn't respect that, we may not have other choices