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

Jsonc #10183

Merged
merged 13 commits into from
May 16, 2024
Merged

Jsonc #10183

merged 13 commits into from
May 16, 2024

Conversation

akallabeth
Copy link
Member

Replaces #10178

@akallabeth akallabeth added this to the next-3.0 milestone May 14, 2024
@akallabeth
Copy link
Member Author

@ondrejholy did take your pr and moved the stuff to WinPR so we don´t require to know which json library was linked.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 34. Check the log or trigger a new build to see more.

winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11953/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/libwinpr/utils/json/json.c Outdated Show resolved Hide resolved
@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11954/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11955/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

winpr/include/winpr/json.h Outdated Show resolved Hide resolved
winpr/include/winpr/json.h Outdated Show resolved Hide resolved
@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11956/

ondrejholy and others added 4 commits May 14, 2024 20:45
* Add module for cJSON
* Add module for json-c
as we now support cJSON and json-c we need to wrap the functions we use.
also allows drop in replacements for older cJSON versions lacking
certain functions.
use the new WinPR JSON wrapper API
@akallabeth
Copy link
Member Author

@ondrejholy ok, did implement the generic JSON wrapper API (mostly only the functions we currently use)
would be nice if you could test this and readd the tests from your pull request (did not take these parts, only renamed the functions we use)

Copy link

clang-tidy review says "All clean, LGTM! 👍"

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11957/

Copy link

clang-tidy review says "All clean, LGTM! 👍"

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11958/

Copy link

clang-tidy review says "All clean, LGTM! 👍"

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11960/

Copy link

clang-tidy review says "All clean, LGTM! 👍"

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11963/

Copy link

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11973/

Copy link

clang-tidy review says "All clean, LGTM! 👍"

@akallabeth
Copy link
Member Author

@ondrejholy did cherry-pick your commits to this pr.

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11974/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

client/SDL/sdl_prefs.cpp Show resolved Hide resolved
client/SDL/sdl_prefs.cpp Show resolved Hide resolved
client/SDL/sdl_prefs.cpp Show resolved Hide resolved
client/SDL/sdl_prefs.cpp Show resolved Hide resolved
client/SDL/sdl_prefs.cpp Show resolved Hide resolved
client/SDL/test/TestSDLPrefs.cpp Outdated Show resolved Hide resolved
client/SDL/test/TestSDLPrefs.cpp Outdated Show resolved Hide resolved
client/SDL/test/TestSDLPrefs.cpp Outdated Show resolved Hide resolved
client/SDL/test/TestSDLPrefs.cpp Outdated Show resolved Hide resolved
client/SDL/test/TestSDLPrefs.cpp Outdated Show resolved Hide resolved
@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11975/

@akallabeth
Copy link
Member Author

@freerdp-bot test please

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11976/

Currently, the `arm_handle_bad_request` function returns `FALSE` when the
`cJSON_ParseWithLength` function fails to parse the message, but only when
the `cJSON_GetErrorPtr` returns a valid pointer. It would be better to
return regardless of the `cJSON_GetErrorPtr` return value.
Although, the `pkg_check_modules` function is used when finding the
json-c library, the results are never used. Let's add the `HINTS` params
for the `find_path` and `find_library` functions.
This is in preparation for the subsequent commit adding test case for
the prefs functionality to avoid building the code twice.
The WINPR_JSON_FOUND macro is never defined currently. Consequently,
the SDL prefs always use the fallback implementation. Let's use the
WITH_WINPR_JSON instead to really parse the sdl-freerdp.json file.
@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11977/

@akallabeth
Copy link
Member Author

@freerdp-bot test please

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11978/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11979/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11980/

* pkg-config sets CJSON_FOUND, check for that instead of the paths, which
  might be unset or set to <var>-NOTFOUND
* move detection to own file. find_package targets are meant to be used
  in same directory or below, but we define the WinPR target one above,
  so include it in parent
@akallabeth
Copy link
Member Author

@ondrejholy ok, did have to remove the SDL tests:

  1. WINPR_ASSERT might not be defined, better use error return
  2. the config file can not be overridden, the default of the user running the test is used.

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/11981/

@ondrejholy
Copy link
Contributor

Hmm, right. It is not a problem to replace WINPR_ASSERT with the if-return statements.

The config path is overridden over the XDG_CONFIG_HOME envar, see: https://github.com/ondrejholy/FreeRDP/blob/af592294cd4fda51468331e1af4e7667890525b5/client/SDL/test/CMakeLists.txt#L25. The ctest --test-dir build -R TestSDLPrefs works fine. It doesn't work when you manually start the test binary without setting XDG_CONFIG_HOME though. Maybe the test can verify that XDG_CONFIG_HOME is properly set, or set that environment variable itself, what do you think?

But since we have the JSON wrapper now, it should not be hard to add some generic tests for the wrapper itself... would you like me to prototype something?

@akallabeth
Copy link
Member Author

Hmm, right. It is not a problem to replace WINPR_ASSERT with the if-return statements.

The config path is overridden over the XDG_CONFIG_HOME envar, see: https://github.com/ondrejholy/FreeRDP/blob/af592294cd4fda51468331e1af4e7667890525b5/client/SDL/test/CMakeLists.txt#L25. The ctest --test-dir build -R TestSDLPrefs works fine. It doesn't work when you manually start the test binary without setting XDG_CONFIG_HOME though. Maybe the test can verify that XDG_CONFIG_HOME is properly set, or set that environment variable itself, what do you think?

But since we have the JSON wrapper now, it should not be hard to add some generic tests for the wrapper itself... would you like me to prototype something?

I think the whole sdl_pref_* must be rewritten.
might be a good idea to create a class and then use an instance() method that uses the default file name can be replaced by some custom stuff while testing?

@akallabeth
Copy link
Member Author

I´ll have a look at the rewrite of the SdlPref

@akallabeth akallabeth merged commit c073c76 into FreeRDP:master May 16, 2024
5 of 7 checks passed
@akallabeth akallabeth deleted the jsonc branch May 16, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants