diff mbox

Add msgpack

Message ID 87lh2b7009.fsf@openmailbox.org
State New
Headers show

Commit Message

Lukas Gradl June 11, 2016, 11:56 p.m. UTC
Hi,

I am sorry for the long silence, this week was busy and I did not find a
lot of time to look into this.  My apologies for that!

Leo Famulari <leo@famulari.name> writes:

> On Mon, Jun 06, 2016 at 10:07:37AM -0400, Leo Famulari wrote:
>> On Mon, May 30, 2016 at 03:50:41PM -0500, Lukas Gradl wrote:
>> > * gnu/packages/serialization.scm (msgpack): New variable.
>> 
>> > +    (inputs
>> > +     `(("zlib" ,zlib)))
>> 
>> I noticed that the output does not refer to zlib:
>> 
>> $ guix gc --references $(./pre-inst-env guix build msgpack)
>> /gnu/store/8m00x5x8ykmar27s9248cmhnkdb2n54a-glibc-2.22
>> /gnu/store/jh52sklgbvq5f0z637wr99a64y25cx8r-msgpack-1.4.1
>> /gnu/store/v39bh3ln3ncnzhyw0kd12d46kww9747v-gcc-4.9.3-lib
>>

Indeed.  I was not aware of this '--references' before.

>> However, the compilation fails without it.
>>

Specifically, the phase that fails is 'check'.  The phase 'build' passes.

>> We need to make sure that msgpack can find zlib at run time. Lukas, can
>> you check if there is a build-time configuration option to provide the
>> path to zlib?
>
> This is from the top of the msgpack source tree:
> $ grep -rI zlib
> include/msgpack/zbuffer.h:#include <zlib.h>
> include/msgpack/zbuffer.hpp:#include <zlib.h>
>
> Should we do string substitution to provide the abolute path to the zlib
> headers?

This is probably not necessary, since it can find zlib (at compile time)
if zlib is provided as an input, i.e. all phases pass in this case.  I
think the reason for the missing reference is another:

I did:

$ grep -ri "Zlib" .
./include/msgpack/zbuffer.hpp:#include <zlib.h>
./include/msgpack/zbuffer.h:#include <zlib.h>
./CMakeLists.txt:FIND_PACKAGE (ZLIB)
./CMakeLists.txt:IF (GTEST_FOUND AND ZLIB_FOUND AND THREADS_FOUND)
./test/CMakeLists.txt:FIND_PACKAGE (ZLIB REQUIRED)
./test/CMakeLists.txt:    ${ZLIB_INCLUDE_DIRS}
./test/CMakeLists.txt:        ${ZLIB_LIBRARIES}


Cmake is also supported as build system.  I looked into ./CMakeLists.txt
in the top of the source tree and this is the passage that refers to
zlib:


FIND_PACKAGE (GTest)
FIND_PACKAGE (ZLIB)
FIND_PACKAGE (Threads)
IF (GTEST_FOUND AND ZLIB_FOUND AND THREADS_FOUND)
    OPTION (MSGPACK_BUILD_TESTS "Build msgpack tests." ON)
ENDIF ()


It seems that zlib is only required for tests.  In README and README.md
zlib is never  mentioned among the dependencies, which also supports
that it is only needed for tests.  I was not aware of that before (Shame
on me).

I moved zlib from inputs to native-inputs.  An updated patch is
attached.

Sorry for the confusion and thank you very much for your work on this!

Best,
Lukas

Comments

Leo Famulari June 12, 2016, 12:17 a.m. UTC | #1
On Sat, Jun 11, 2016 at 06:56:38PM -0500, Lukas Gradl wrote:
> It seems that zlib is only required for tests.  In README and README.md
> zlib is never  mentioned among the dependencies, which also supports
> that it is only needed for tests.  I was not aware of that before (Shame
> on me).

Is zlib really only required for tests? I see this:

$ grep -rIi zlib /gnu/store/xn8z7k6j7zm4qz14bm29fgk0kwwvz3c4-msgpack-1.4.1
/gnu/store/xn8z7k6j7zm4qz14bm29fgk0kwwvz3c4-msgpack-1.4.1/include/msgpack/zbuffer.h:#include <zlib.h>
/gnu/store/xn8z7k6j7zm4qz14bm29fgk0kwwvz3c4-msgpack-1.4.1/include/msgpack/zbuffer.hpp:#include <zlib.h>

So, there are headers in the msgpack output that require zlib.

If you are at the stage where you are using Ring based on these
packages, I wonder if it is not using the part of msgpack that uses
zlib? Or, if it's finding zlib in the environment (if you are on a
foreign distro)?

Or have I misunderstood?
Lukas Gradl June 12, 2016, 4:24 a.m. UTC | #2
Leo Famulari <leo@famulari.name> writes:

> On Sat, Jun 11, 2016 at 06:56:38PM -0500, Lukas Gradl wrote:
>> It seems that zlib is only required for tests.  In README and README.md
>> zlib is never  mentioned among the dependencies, which also supports
>> that it is only needed for tests.  I was not aware of that before (Shame
>> on me).
>
> Is zlib really only required for tests? I see this:
>
> $ grep -rIi zlib /gnu/store/xn8z7k6j7zm4qz14bm29fgk0kwwvz3c4-msgpack-1.4.1
> /gnu/store/xn8z7k6j7zm4qz14bm29fgk0kwwvz3c4-msgpack-1.4.1/include/msgpack/zbuffer.h:#include <zlib.h>
> /gnu/store/xn8z7k6j7zm4qz14bm29fgk0kwwvz3c4-msgpack-1.4.1/include/msgpack/zbuffer.hpp:#include <zlib.h>
>
> So, there are headers in the msgpack output that require zlib.

Oh, that is true.  I searched around, but I could not find anything
about zbuffer.h[pp].  I am not sure what it is used for.  FWIW, it is
only referenced in src/Makefile, src/Makefile.in, src/Makefile.am
CMakeLists.txt and in the tests:


$ grep -ri zbuffer
src/Makefile:	../include/msgpack/zbuffer.hpp \
src/Makefile:	../include/msgpack/vrefbuffer.h ../include/msgpack/zbuffer.h \
src/Makefile:	../include/msgpack/zbuffer.hpp ../include/msgpack/zone.hpp \
src/Makefile:	../include/msgpack/vrefbuffer.h ../include/msgpack/zbuffer.h \
src/Makefile.in:@ENABLE_CXX_TRUE@	../include/msgpack/zbuffer.hpp \
src/Makefile.in:	../include/msgpack/vrefbuffer.h ../include/msgpack/zbuffer.h \
src/Makefile.in:	../include/msgpack/zbuffer.hpp ../include/msgpack/zone.hpp \
src/Makefile.in:	../include/msgpack/vrefbuffer.h ../include/msgpack/zbuffer.h \
src/Makefile.am:		../include/msgpack/zbuffer.h \
src/Makefile.am:		../include/msgpack/zbuffer.hpp \
ChangeLog:  * Fix zbuffer with empty string problem (#303)
include/msgpack/zbuffer.hpp:#ifndef MSGPACK_ZBUFFER_HPP
include/msgpack/zbuffer.hpp:#define MSGPACK_ZBUFFER_HPP
include/msgpack/zbuffer.hpp:#ifndef MSGPACK_ZBUFFER_RESERVE_SIZE
include/msgpack/zbuffer.hpp:#define MSGPACK_ZBUFFER_RESERVE_SIZE 512
include/msgpack/zbuffer.hpp:#ifndef MSGPACK_ZBUFFER_INIT_SIZE
include/msgpack/zbuffer.hpp:#define MSGPACK_ZBUFFER_INIT_SIZE 8192
include/msgpack/zbuffer.hpp:class zbuffer {
include/msgpack/zbuffer.hpp:    zbuffer(int level = Z_DEFAULT_COMPRESSION,
include/msgpack/zbuffer.hpp:            size_t init_size = MSGPACK_ZBUFFER_INIT_SIZE)
include/msgpack/zbuffer.hpp:    ~zbuffer()
include/msgpack/zbuffer.hpp:            if(m_stream.avail_out < MSGPACK_ZBUFFER_RESERVE_SIZE) {
include/msgpack/zbuffer.hpp:    zbuffer(const zbuffer&);
include/msgpack/zbuffer.hpp:    zbuffer& operator=(const zbuffer&);
include/msgpack/zbuffer.hpp:    zbuffer(const zbuffer&) = delete;
include/msgpack/zbuffer.hpp:    zbuffer& operator=(const zbuffer&) = delete;
include/msgpack/zbuffer.hpp:#endif /* msgpack/zbuffer.hpp */
include/msgpack/zbuffer.h:#ifndef MSGPACK_ZBUFFER_H
include/msgpack/zbuffer.h:#define MSGPACK_ZBUFFER_H
include/msgpack/zbuffer.h: * @defgroup msgpack_zbuffer Compressed buffer
include/msgpack/zbuffer.h:typedef struct msgpack_zbuffer {
include/msgpack/zbuffer.h:} msgpack_zbuffer;
include/msgpack/zbuffer.h:#ifndef MSGPACK_ZBUFFER_INIT_SIZE
include/msgpack/zbuffer.h:#define MSGPACK_ZBUFFER_INIT_SIZE 8192
include/msgpack/zbuffer.h:static inline bool msgpack_zbuffer_init(
include/msgpack/zbuffer.h:    msgpack_zbuffer* zbuf, int level, size_t init_size);
include/msgpack/zbuffer.h:static inline void msgpack_zbuffer_destroy(msgpack_zbuffer* zbuf);
include/msgpack/zbuffer.h:static inline msgpack_zbuffer* msgpack_zbuffer_new(int level, size_t init_size);
include/msgpack/zbuffer.h:static inline void msgpack_zbuffer_free(msgpack_zbuffer* zbuf);
include/msgpack/zbuffer.h:static inline char* msgpack_zbuffer_flush(msgpack_zbuffer* zbuf);
include/msgpack/zbuffer.h:static inline const char* msgpack_zbuffer_data(const msgpack_zbuffer* zbuf);
include/msgpack/zbuffer.h:static inline size_t msgpack_zbuffer_size(const msgpack_zbuffer* zbuf);
include/msgpack/zbuffer.h:static inline bool msgpack_zbuffer_reset(msgpack_zbuffer* zbuf);
include/msgpack/zbuffer.h:static inline void msgpack_zbuffer_reset_buffer(msgpack_zbuffer* zbuf);
include/msgpack/zbuffer.h:static inline char* msgpack_zbuffer_release_buffer(msgpack_zbuffer* zbuf);
include/msgpack/zbuffer.h:#ifndef MSGPACK_ZBUFFER_RESERVE_SIZE
include/msgpack/zbuffer.h:#define MSGPACK_ZBUFFER_RESERVE_SIZE 512
include/msgpack/zbuffer.h:static inline int msgpack_zbuffer_write(void* data, const char* buf, size_t len);
include/msgpack/zbuffer.h:static inline bool msgpack_zbuffer_expand(msgpack_zbuffer* zbuf);
include/msgpack/zbuffer.h:static inline bool msgpack_zbuffer_init(msgpack_zbuffer* zbuf,
include/msgpack/zbuffer.h:    memset(zbuf, 0, sizeof(msgpack_zbuffer));
include/msgpack/zbuffer.h:static inline void msgpack_zbuffer_destroy(msgpack_zbuffer* zbuf)
include/msgpack/zbuffer.h:static inline msgpack_zbuffer* msgpack_zbuffer_new(int level, size_t init_size)
include/msgpack/zbuffer.h:    msgpack_zbuffer* zbuf = (msgpack_zbuffer*)malloc(sizeof(msgpack_zbuffer));
include/msgpack/zbuffer.h:    if(!msgpack_zbuffer_init(zbuf, level, init_size)) {
include/msgpack/zbuffer.h:static inline void msgpack_zbuffer_free(msgpack_zbuffer* zbuf)
include/msgpack/zbuffer.h:    msgpack_zbuffer_destroy(zbuf);
include/msgpack/zbuffer.h:static inline bool msgpack_zbuffer_expand(msgpack_zbuffer* zbuf)
include/msgpack/zbuffer.h:static inline int msgpack_zbuffer_write(void* data, const char* buf, size_t len)
include/msgpack/zbuffer.h:    msgpack_zbuffer* zbuf = (msgpack_zbuffer*)data;
include/msgpack/zbuffer.h:        if(zbuf->stream.avail_out < MSGPACK_ZBUFFER_RESERVE_SIZE) {
include/msgpack/zbuffer.h:            if(!msgpack_zbuffer_expand(zbuf)) {
include/msgpack/zbuffer.h:static inline char* msgpack_zbuffer_flush(msgpack_zbuffer* zbuf)
include/msgpack/zbuffer.h:            if(!msgpack_zbuffer_expand(zbuf)) {
include/msgpack/zbuffer.h:static inline const char* msgpack_zbuffer_data(const msgpack_zbuffer* zbuf)
include/msgpack/zbuffer.h:static inline size_t msgpack_zbuffer_size(const msgpack_zbuffer* zbuf)
include/msgpack/zbuffer.h:static inline void msgpack_zbuffer_reset_buffer(msgpack_zbuffer* zbuf)
include/msgpack/zbuffer.h:static inline bool msgpack_zbuffer_reset(msgpack_zbuffer* zbuf)
include/msgpack/zbuffer.h:    msgpack_zbuffer_reset_buffer(zbuf);
include/msgpack/zbuffer.h:static inline char* msgpack_zbuffer_release_buffer(msgpack_zbuffer* zbuf)
include/msgpack/zbuffer.h:#endif /* msgpack/zbuffer.h */
CMakeLists.txt:    include/msgpack/zbuffer.h
CMakeLists.txt:        include/msgpack/zbuffer.hpp
test/buffer.cpp:#include <msgpack/zbuffer.hpp>
test/buffer.cpp:#include <msgpack/zbuffer.h>
test/buffer.cpp:TEST(buffer, zbuffer)
test/buffer.cpp:    msgpack::zbuffer zbuf;
test/buffer.cpp:TEST(buffer, zbuffer_c)
test/buffer.cpp:    msgpack_zbuffer zbuf;
test/buffer.cpp:    EXPECT_TRUE(msgpack_zbuffer_init(&zbuf, 1, MSGPACK_ZBUFFER_INIT_SIZE));
test/buffer.cpp:    EXPECT_EQ(0, msgpack_zbuffer_write(&zbuf, "a", 1));
test/buffer.cpp:    EXPECT_EQ(0, msgpack_zbuffer_write(&zbuf, "a", 1));
test/buffer.cpp:    EXPECT_EQ(0, msgpack_zbuffer_write(&zbuf, "a", 1));
test/buffer.cpp:    EXPECT_EQ(0, msgpack_zbuffer_write(&zbuf, "", 0));
test/buffer.cpp:    EXPECT_TRUE(msgpack_zbuffer_flush(&zbuf) != NULL);
test/buffer.cpp:    msgpack_zbuffer_destroy(&zbuf);


Judging from this, I am inclined to think that msgpack does not use
these two headers for anything but tests.  But I am not sure about
this.

> If you are at the stage where you are using Ring based on these
> packages, I wonder if it is not using the part of msgpack that uses
> zlib? Or, if it's finding zlib in the environment (if you are on a
> foreign distro)?

I am on GuixSD, but I do not have any packages besides opendht that
would depend on msgpack, yet.  I do not have a working definition for
Ring.  At this point, I do not see any way I could really test if
msgpack works properly with/without zlib.

FWIW, I looked at what other distributions do:

The Arch PKGBUILD does not refer to Zlib at all:
https://git.archlinux.org/svntogit/community.git/tree/trunk/PKGBUILD?h=packages/msgpack-c

Gentoo has Zlib for tests:
https://gitweb.gentoo.org/repo/gentoo.git/tree/dev-libs/msgpack/msgpack-1.1.0.ebuild

Debian seems to not refer to Zlib:
https://packages.debian.org/jessie/libmsgpackc2
https://packages.debian.org/jessie/libmsgpack3
https://packages.debian.org/jessie/libmsgpack-dev

This Nix expression does not mention zlib:
https://github.com/NixOS/nixpkgs/blob/f199be0fafcff0713dd4340b4373e575294aad58/pkgs/development/libraries/libmsgpack/generic.nix#L41


However, all of these seem to be different versions of msgpack.


On one hand, since these distributions seem to not include zlib as a
dependency, maybe zbuffer.h and zbuffer.hpp are not meant to be used or are
seldom used by other projects.  On the other hand msgpack installs these
headers, so they should be useful.

>
> Or have I misunderstood?

No, I missed that.  My mistake.  But I am not sure what to do about
this.

I am working on packaging pjproject now.  After that I think I will have
most of the dependencies of libring.  At that point I could probably
test if msgpack works the way it is now.  I am perfectly fine if we wait
with the inclusion of msgpack until then. What do you think?

Best,
Lukas
Leo Famulari June 13, 2016, 4:58 p.m. UTC | #3
On Sat, Jun 11, 2016 at 11:24:23PM -0500, Lukas Gradl wrote:
> Oh, that is true.  I searched around, but I could not find anything
> about zbuffer.h[pp].  I am not sure what it is used for.  FWIW, it is
> only referenced in src/Makefile, src/Makefile.in, src/Makefile.am
> CMakeLists.txt and in the tests:

[...]

> Judging from this, I am inclined to think that msgpack does not use
> these two headers for anything but tests.  But I am not sure about
> this.

I think that another application would call the zbuffer functions,
although the string 'zbuffer' does not exist in the opendht source code.
But, we should make sure msgpack will work for other calling
applications that might be added later.

On #guix, bavier suggested we patch 'msgpack.pc.in'. Specifically, we
should append the include flag to 'Cflags:' and create a 'Libs.private:'
field for the -L and -l flags. There are some examples in
'gnu/packages'.

More details on the IRC log:
https://gnunet.org/bot/log/guix/2016-06-13#T1056507

I've cc-ed Eric since I don't know much about pkg-config.

> FWIW, I looked at what other distributions do:
> 
> The Arch PKGBUILD does not refer to Zlib at all:
> https://git.archlinux.org/svntogit/community.git/tree/trunk/PKGBUILD?h=packages/msgpack-c
> 
> Gentoo has Zlib for tests:
> https://gitweb.gentoo.org/repo/gentoo.git/tree/dev-libs/msgpack/msgpack-1.1.0.ebuild
> 
> Debian seems to not refer to Zlib:
> https://packages.debian.org/jessie/libmsgpackc2
> https://packages.debian.org/jessie/libmsgpack3
> https://packages.debian.org/jessie/libmsgpack-dev

I think that zlib is so ubiquitous that it's a safe assumption it will
can be found in the environment on the '/usr' distros.

> This Nix expression does not mention zlib:
> https://github.com/NixOS/nixpkgs/blob/f199be0fafcff0713dd4340b4373e575294aad58/pkgs/development/libraries/libmsgpack/generic.nix#L41

An oversight? I don't know the Nix equivalent of `guix refresh -l` to
check which of their packages use msgpack.
Leo Famulari June 13, 2016, 5:59 p.m. UTC | #4
On Mon, Jun 13, 2016 at 12:58:52PM -0400, Leo Famulari wrote:
> On Sat, Jun 11, 2016 at 11:24:23PM -0500, Lukas Gradl wrote:
> > Oh, that is true.  I searched around, but I could not find anything
> > about zbuffer.h[pp].  I am not sure what it is used for.  FWIW, it is
> > only referenced in src/Makefile, src/Makefile.in, src/Makefile.am
> > CMakeLists.txt and in the tests:
> 
> [...]
> 
> > Judging from this, I am inclined to think that msgpack does not use
> > these two headers for anything but tests.  But I am not sure about
> > this.
> 
> I think that another application would call the zbuffer functions,
> although the string 'zbuffer' does not exist in the opendht source code.
> But, we should make sure msgpack will work for other calling
> applications that might be added later.
> 
> On #guix, bavier suggested we patch 'msgpack.pc.in'. Specifically, we
> should append the include flag to 'Cflags:' and create a 'Libs.private:'
> field for the -L and -l flags. There are some examples in
> 'gnu/packages'.
> 
> More details on the IRC log:
> https://gnunet.org/bot/log/guix/2016-06-13#T1056507
> 
> I've cc-ed Eric since I don't know much about pkg-config.

Of course, I forgot to do this :p
diff mbox

Patch

From ed3dae4a8f34599c0dc5cf47c26df01adda5a209 Mon Sep 17 00:00:00 2001
From: Lukas Gradl <lgradl@openmailbox.org>
Date: Sat, 11 Jun 2016 18:46:13 -0500
Subject: [PATCH] gnu: Add msgpack.

* gnu/packages/serialization.scm (msgpack): New variable.
---
 gnu/packages/serialization.scm | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/gnu/packages/serialization.scm b/gnu/packages/serialization.scm
index 8dfd21d..9bd5d60 100644
--- a/gnu/packages/serialization.scm
+++ b/gnu/packages/serialization.scm
@@ -1,5 +1,6 @@ 
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015 Ricardo Wurmus <rekado@elephly.net>
+;;; Copyright © 2016 Lukas Gradl <lgradl@openmailbox.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -21,7 +22,11 @@ 
   #:use-module (guix packages)
   #:use-module (guix download)
   #:use-module (guix build-system cmake)
+  #:use-module (guix build-system gnu)
   #:use-module (gnu packages)
+  #:use-module (gnu packages autotools)
+  #:use-module (gnu packages check)
+  #:use-module (gnu packages compression)
   #:use-module (gnu packages documentation))
 
 (define-public cereal
@@ -72,3 +77,28 @@ 
 arbitrary data types and reversibly turns them into different representations,
 such as compact binary encodings, XML, or JSON.")
     (license license:bsd-3)))
+
+
+(define-public msgpack
+  (package
+    (name "msgpack")
+    (version "1.4.1")
+    (source
+     (origin
+       (method url-fetch)
+       (uri
+        (string-append
+         "https://github.com/msgpack/msgpack-c/releases/download/"
+         "cpp-" version "/msgpack-" version ".tar.gz"))
+       (sha256
+        (base32
+         "0bpjfh9vz0n2k93mph3x15clmigkgs223xfn8h12ymrh5gsi5ica"))))
+    (build-system gnu-build-system)
+    (native-inputs
+     `(("googletest" ,googletest)
+       ("zlib" ,zlib)))
+    (home-page "http://www.msgpack.org")
+    (synopsis "Binary serialization library")
+    (description "Msgpack is a library for C/C++ that implements binary
+serialization.")
+    (license license:boost1.0)))
-- 
2.7.4