DPP: De-const have_voice option and add coroutines support. #4824
Conversation
Nk125
commented
Aug 2, 2024
Nk125
commented
- Before adding new features and new modules, please go to issues to submit the relevant feature description first. [✔️]
- See DPP: De-const have_voice option and add coroutines support. #4823
- Write good commit messages and use the same coding conventions as the rest of the project. [✔️]
- Across the whole pull request conventional commits are used.
- Please commit code to dev branch and we will merge into master branch in feature [✔️]
- Ensure your edited codes with four spaces instead of TAB. [✔️]
- The EditorConfig plugin for Visual Studio Code managed these based in the public EditorConfig file in this repo.
De-const have_voice option and enables optional download of voice libraries
Adds a configurable option to enable coroutine support
|
Before committing, the changes were tested using the test.lua script in this repo with my machine (Windows 10 21H1 x86_64). |
packages/d/dpp/xmake.lua
Outdated
| @@ -53,7 +53,9 @@ package("dpp") | |||
|
|
|||
| add_deps("nlohmann_json", "openssl", "zlib") | |||
|
|
|||
| add_configs("have_voice", { description = "Enable voice support for the library.", default = true, type = "boolean" , readonly = true}) | |||
| add_configs("have_voice", { description = "Enable voice support for the library.", default = true, type = "boolean" , readonly = false}) | |||
There was a problem hiding this comment.
please rename have_voice to voice, we need not any have_ with_ enable_ preifx.
There was a problem hiding this comment.
This was changed but the main issue is that this change will break the current user code bases, the flag was left unchanged since 2022 (introduced here), the most reliable solution I was considering is showing a deprecation warning to anyone is using have_voice flag and then in later DPP versions erase it from the next versions.
Another solution I propose is doing a temporal alias from have_voice to voice internally in the script, and doing the same as before, show a deprecation warning and then delete the flag.
There was a problem hiding this comment.
you can reserve have_voice and add some deprecated warnings.
and add new voice config , then tell user to use voice instead of have_voice
There was a problem hiding this comment.
Added in the last commit, please review it for other advices, thanks for the support!
As mentioned in the same pull request, prefixes like have_ are discouraged in xmake packages. BREAKING CHANGES: This WILL break xmake scripts that use the have_voice flag, IMO it's better to keep the previous have_voice flag as a deprecated alias of the new voice flag, and in the next version of D++ completely remove it from the package.
If coroutines are enabled, C++20 is required or the compilation will fail even though the test only requires C++17 to work, as dpp automatically includes coroutines if the macro is defined without checking C++ version
The current configuration wasn't proper and when coroutines were enabled, the C++ version would change to C++17 sporadically, ignoring the later C++20 specification that was done later. Now the C++ version will adjust correctly to the coro flag.
…-repo into dpp-feats-coro-voice
This will bring up again have_voice to avoid codebase breakage, but with many deprecation warnings around to prevent new users to define it.
