Created a JUCE module for mpack.

jrlanglois commented 5 years ago

This makes integrating mpack in C++ JUCE projects really swift and easy.

Implementation Notes

JUCE Unity Build Module Format
The module code is explicitly designed to be CRLF to support MSVC. This is a consistent design choice for JUCE projects everywhere.

Testing Question

I'm open to ideas for how you would like to have these files be automatically tested as part of your CI.

You would definitely need a clone of JUCE from its master or develop branch.
You would need to build its Projucer app.
You will need to have a Projucer-based project ready with your module.
You can run the Projucer from the command-line:
- Just resave the project.
- pathToProjucer --resave path/to/MyProject.jucer

I'm not sure what you would want to see run as part of this project. Although, JUCE provides a UnitTest class which could be integrated into the module, and selectively enabled via a module configuration macro.

MSVC Warning Notes

There are two types of warnings that come up. Seeing that I don't have the entire context for why the code is the way it is, I'm not totally sure how you would prefer tackling them so I'll leave that up to you!

C4127 is a common occurrence.
- Example: if (sizeof(unsigned int) == 4)
There are a handful of truncation warnings (C4310).
- mpack_write_byte_element(writer, (char)0xc0);
- 0xc0 is 192 in decimal, causing overflow and therefore UB.

Module Usage Example in JUCE's Projucer

ludocode commented 5 years ago

Hey Joël, long time no see! Thanks for the PR, it looks good.

Does it work if the ludocode_mpack/ folder is not at the root of the repo? Could it be put under bindings/juce/ludocode_mpack/ instead? I just want to make sure there aren't too many top-level folders, and it's not clear that this is for JUCE. Most people will still integrate without a package manager and I don't want them to worry what the various folders are. This is essentially a "binding" to JUCE, and "bindings" looks like a folder that can safely be ignored if people are just looking for the core code. I've done some work on MPack bindings in other languages and had them in a bindings/ folder as well so they might end up getting brought back at some point if I ever get around to completing them.

As for the warnings, yes, there are a bunch under /W4. I've only made it compile cleanly under /W3 so far. Supporting /W4 is of questionable value because many of the warnings aren't useful or even fixable, but I'm planning on supporting it anyway, most likely by just disabling a lot of them. There are way more warnings from the unit tests than from the library itself which is part of why I haven't bothered to fix it. In the meantime you could disable them by adding this to the top of ludocode_mpack.cpp:

#ifdef _MSC_VER
#pragma warning(disable: 4127 4310)
#endif

To test it automatically you could add a "juce" job to the Travis CI build matrix. It could install JUCE and Projucer, create a test project from the command-line (or use an included one if that's not possible), and then build it. You could also wrap the unit tests to run as a single test under JUCE's unit test runner. It's kind of time consuming to do all this though. It's fine by me if you don't want to bother with unit tests of the JUCE integration.

jrlanglois commented 5 years ago

Hey Nick! Long time indeed!

Does it work if the ludocode_mpack/ folder is not at the root of the repo? Could it be put under bindings/juce/ludocode_mpack/ instead?

It doesn't matter where you want it as Projucer only cares about the folder name matching the provided module name. I'll move it around as requested!

Tangentially: your logic is totally sound to me. But does this mean you're planning on adding more bindings?

RE: the warnings. Yeah, I'll just disable them.

To test it automatically you could add a "juce" job to the Travis CI build matrix. It could install JUCE and Projucer, create a test project from the command-line (or use an included one if that's not possible), and then build it. You could also wrap the unit tests to run as a single test under JUCE's unit test runner. It's kind of time consuming to do all this though. It's fine by me if you don't want to bother with unit tests of the JUCE integration.

I'm 100% down to follow through with CI and a unit test integration! It's a fun puzzle.

ludocode commented 5 years ago

I have some local branches with C++ and Objective-C bindings under a top-level bindings/ folder. I never bothered to merge them since they're incomplete and it didn't seem like anyone would use them, but yeah, it adds stuff like mpack_write_nsstring() to write an NSString*, mpack_read_vector() to read a bin as std::vector<char>, etc. There's also Tree/Node classes to give a proper C++-style API but that stuff is even less complete.

I had also planned to write some Python bindings which would be a good exercise for learning to write a CPython extension. It might even be faster than the reference msgpack-python, and if not faster then at least way more maintainable. I sort of abandoned that idea because I abandoned Python altogether as you might have seen from the recent Lua buildsystem code. If I do ever finish any of this stuff, it'll go under bindings/.

I'm also going to remove the top-level projects/ folder (so thanks for not putting the Projucer project there.) I'm deleting the Visual Studio and Xcode projects in favor of building with Lua+Ninja just like on Ubuntu. In fact I've already got the Lua+Ninja code building with MSVC, just not in the Appveyor CI yet. In the future if we do need project files, they should go under test/ like you have here.

It looks like you've got a syntax error in your .travis.yml which is making it not build. I recommend yaml2json to debug issues like this; I use it all the time to write YAML. Here's what I get:

$ /home/nick/.gem/ruby/2.6.0/bin/yaml2json .travis.yml 
Traceback (most recent call last):
    7: from /home/nick/.gem/ruby/2.6.0/bin/yaml2json:23:in `<main>'
    6: from /home/nick/.gem/ruby/2.6.0/bin/yaml2json:23:in `load'
    5: from /home/nick/.gem/ruby/2.6.0/gems/yaml2json-0.0.2/bin/yaml2json:14:in `<top (required)>'
    4: from /home/nick/.gem/ruby/2.6.0/gems/yaml2json-0.0.2/bin/yaml2json:14:in `each'
    3: from /home/nick/.gem/ruby/2.6.0/gems/yaml2json-0.0.2/bin/yaml2json:15:in `block in <top (required)>'
    2: from /usr/lib/ruby/2.6.0/psych.rb:561:in `load_stream'
    1: from /usr/lib/ruby/2.6.0/psych.rb:456:in `parse_stream'
/usr/lib/ruby/2.6.0/psych.rb:456:in `parse': (<unknown>): did not find expected '-' indicator while parsing a block collection at line 34 column 9 (Psych::SyntaxError)

Once you've got that fixed I'm good to merge it :+1:

ludocode commented 2 years ago

Hey Joël, I ended up removing the Xcode and Visual Studio projects getting rid of the projects/ folder entirely. I'm also not planning on adding any other bindings any time soon, so there are no integrations for any IDEs, any packaging formats, any other languages etc. I hope it's okay if I close this. Feel free to maintain a fork that adds JUCE support.

jrlanglois commented 2 years ago

Totally reasonable and no worries there!

ludocode / mpack