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

Add msgpack obj init for complex types #1135

Closed

Conversation

Athishpranav2003
Copy link

@Athishpranav2003 Athishpranav2003 commented Aug 13, 2024

This PR addresses the feature request to initialise msgpack objects such as string, bin, array and map
Closes #1134

Signed-off-by: Athish Pranav D <[email protected]>
@Athishpranav2003
Copy link
Author

Athishpranav2003 commented Aug 13, 2024

@redboltz Could you please check this PR
Once confirmed i will also raise a PR to add this to the documentation

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 54.26%. Comparing base (9785991) to head (96493f5).
Report is 22 commits behind head on c_master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##           c_master    #1135      +/-   ##
============================================
- Coverage     55.45%   54.26%   -1.20%     
============================================
  Files             8        8              
  Lines          1044     1067      +23     
============================================
  Hits            579      579              
- Misses          465      488      +23     

@Athishpranav2003
Copy link
Author

@redboltz just to give more context, we are trying to initialize the msgpack_obj and since its dynamically typed it needs hardcoded assigning. But since we can see 4 of the types have the same format (size and pointer) it will be more modular to have a function to do the same. I am trying to use this for one of the enhancements in fluentbit.

@redboltz
Copy link
Contributor

@Athishpranav2003
Thank you for sending the PR.
It seems that you only add the definition.
I think that a declaration is required at
https://github.com/msgpack/msgpack-c/blob/c_master/include/msgpack/object.h#L99

In addition, tests for added function should be added the end of
https://github.com/msgpack/msgpack-c/blob/c_master/test/msgpack_c.cpp

@Athishpranav2003
Copy link
Author

I will add them now. I forgot to add them in the hurry

@Athishpranav2003
Copy link
Author

@redboltz i am having issues with executing the tests tho.

 ~/p/msgpack-c   msgpack-object-kv *…  
(° ͜ʖ͡°)╭∩╮=>sudo make install                                                                         Wed 14 Aug 2024 09:44:23 AM IST
[ 25%] Built target msgpack-c
[ 50%] Built target msgpack-c-static
[ 58%] Built target boundary
[ 66%] Built target lib_buffer_unpack
[ 75%] Built target simple_c
[ 83%] Built target speed_test_uint32_array
[ 91%] Built target speed_test_uint64_array
[100%] Built target user_buffer_unpack
Install the project...
-- Install configuration: ""
-- Up-to-date: /usr/local/lib64/libmsgpack-c.so.2.0.0
-- Up-to-date: /usr/local/lib64/libmsgpack-c.so.2
-- Up-to-date: /usr/local/lib64/libmsgpack-c.so
-- Up-to-date: /usr/local/lib64/libmsgpack-c.a
-- Up-to-date: /usr/local/include/msgpack.h
-- Up-to-date: /usr/local/include/msgpack/fbuffer.h
-- Up-to-date: /usr/local/include/msgpack/gcc_atomic.h
-- Up-to-date: /usr/local/include/msgpack/object.h
-- Up-to-date: /usr/local/include/msgpack/pack.h
-- Up-to-date: /usr/local/include/msgpack/pack_define.h
-- Up-to-date: /usr/local/include/msgpack/sbuffer.h
-- Up-to-date: /usr/local/include/msgpack/timestamp.h
-- Up-to-date: /usr/local/include/msgpack/unpack.h
-- Up-to-date: /usr/local/include/msgpack/unpack_define.h
-- Up-to-date: /usr/local/include/msgpack/unpack_template.h
-- Up-to-date: /usr/local/include/msgpack/util.h
-- Up-to-date: /usr/local/include/msgpack/version.h
-- Up-to-date: /usr/local/include/msgpack/version_master.h
-- Up-to-date: /usr/local/include/msgpack/vrefbuffer.h
-- Up-to-date: /usr/local/include/msgpack/zbuffer.h
-- Up-to-date: /usr/local/include/msgpack/zone.h
-- Up-to-date: /usr/local/include/msgpack/pack_template.h
-- Up-to-date: /usr/local/include/msgpack/sysdep.h
-- Up-to-date: /usr/local/lib64/pkgconfig/msgpack-c.pc
-- Up-to-date: /usr/local/lib64/cmake/msgpack-c/msgpack-c-targets.cmake
-- Up-to-date: /usr/local/lib64/cmake/msgpack-c/msgpack-c-targets-noconfig.cmake
-- Up-to-date: /usr/local/lib64/cmake/msgpack-c/msgpack-c-config.cmake
-- Up-to-date: /usr/local/lib64/cmake/msgpack-c/msgpack-c-config-version.cmake
 ~/p/msgpack-c   msgpack-object-kv *…  
(° ͜ʖ͡°)╭∩╮=>cd test/                                                                          203ms  Wed 14 Aug 2024 09:44:27 AM IST
 ~/p/msgpack-c   msgpack-object-kv *…  test  
(° ͜ʖ͡°)╭∩╮=>make                                                                                      Wed 14 Aug 2024 09:44:29 AM IST
[ 20%] Built target buffer_c
[ 40%] Built target fixint_c
[ 60%] Built target msgpack_c
[ 80%] Built target pack_unpack_c
[100%] Built target streaming_c
 ~/p/msgpack-c   msgpack-object-kv *…  test  
(° ͜ʖ͡°)╭∩╮=>ls                                                                                139ms  Wed 14 Aug 2024 09:44:31 AM IST
buffer_c*     CMakeCache.txt  cmake_install.cmake  fixint_c*     Makefile    msgpack_c.cpp   pack_unpack_c.cpp  streaming_c.cpp
buffer_c.cpp  CMakeFiles/     CMakeLists.txt       fixint_c.cpp  msgpack_c*  pack_unpack_c*  streaming_c*
 ~/p/msgpack-c   msgpack-object-kv *…  test  
(° ͜ʖ͡°)╭∩╮=>./msgpack_c                                                                               Wed 14 Aug 2024 09:44:33 AM IST
./msgpack_c: error while loading shared libraries: libmsgpack-c.so.2: cannot open shared object file: No such file or directory

I tried exporting the LD_LIBRARY_PATH but still not working. Can you help me with this

@redboltz
Copy link
Contributor

The path /usr/local is the log is something weird.

Build and test should be at your working directory.

Try this at you working directory,

  1. mkdir build
  2. cd build
  3. cmake ..
  4. make
  5. ctest -V

@Athishpranav2003
Copy link
Author

Athishpranav2003 commented Aug 15, 2024

@redboltz

(° ͜ʖ͡°)╭∩╮=>ctest -V                                                                                  Thu 15 Aug 2024 01:43:42 PM IST
UpdateCTestConfiguration  from :/home/aggressive_racer1/projects/msgpack-c/DartConfiguration.tcl
UpdateCTestConfiguration  from :/home/aggressive_racer1/projects/msgpack-c/DartConfiguration.tcl
Test project /home/aggressive_racer1/projects/msgpack-c
Constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
No tests were found!!!

Not sure whats the problem

NVM. Running the test binary manually works. Thanks for helping me out

@Athishpranav2003
Copy link
Author

@redboltz Added the UTs for the same. Please check if any more tests needs to be added

@Athishpranav2003 Athishpranav2003 force-pushed the msgpack-object-kv branch 2 times, most recently from d5be04a to 9628c08 Compare August 16, 2024 03:31
include/msgpack/object.h Outdated Show resolved Hide resolved
src/objectc.c Outdated Show resolved Hide resolved
src/objectc.c Outdated
Comment on lines 163 to 164
return 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return 0;
return true;

Please remove empty line here.

src/objectc.c Outdated Show resolved Hide resolved
test/msgpack_c.cpp Outdated Show resolved Hide resolved
@Athishpranav2003
Copy link
Author

@redboltz addressed your comments

src/objectc.c Outdated Show resolved Hide resolved
@Athishpranav2003
Copy link
Author

@redboltz addressed your comments

@Athishpranav2003
Copy link
Author

@redboltz sorry for force pushing the changes. I just moved the assigning part to end of function and removed the nil line

@redboltz
Copy link
Contributor

@Athishpranav2003

I noticed another issues about the PR. So I created #1137, which includes your contribution.

Here’s the background:

I considered why msgpack_object_init() returns a boolean and under what conditions it could fail. It seems that the failure occurs when an invalid type parameter is passed.

I also thought about whether msgpack_object_init() could be extended to support all msgpack object types in the future. However, I realized that doing so while maintaining a clean interface would be difficult.

From this, I came to two conclusions. First, the name msgpack_object_init() is too broad, covering various meanings (e.g., nil, boolean, int, etc.). Second, if the function name included the type, the type parameter could be eliminated, and failures would no longer occur.

Please review #1137 to see if it addresses your issue.

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.

3 participants