[NEW] devel/cjose

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[NEW] devel/cjose

YASUOKA Masahiko-3
Hi,

I'd like to add devel/cjose.  It's a C library of JOSE (JavaScript
Object Signing and Encryption).  JOSE is used in "OpenID connect"
protocol.

The original diff is written by my colleague, Naoaki Hoshi.

ok?

cjose.tar.gz (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [NEW] devel/cjose

YASUOKA Masahiko-4
Hi,

Anyone can take a look at this?

On Fri, 28 Dec 2018 20:16:27 +0900 (JST)
YASUOKA Masahiko <[hidden email]> wrote:
> Hi,
>
> I'd like to add devel/cjose.  It's a C library of JOSE (JavaScript
> Object Signing and Encryption).  JOSE is used in "OpenID connect"
> protocol.
>
> The original diff is written by my colleague, Naoaki Hoshi.
>
> ok?

Reply | Threaded
Open this post in threaded view
|

Re: [NEW] devel/cjose

Jeremie Courreges-Anglas-2
In reply to this post by YASUOKA Masahiko-3
On Fri, Dec 28 2018, YASUOKA Masahiko <[hidden email]> wrote:
> Hi,
>
> I'd like to add devel/cjose.  It's a C library of JOSE (JavaScript
> Object Signing and Encryption).  JOSE is used in "OpenID connect"
> protocol.
>
> The original diff is written by my colleague, Naoaki Hoshi.
>
> ok?

Fails to build on gcc archs, so not ok:

--8<--
cc1: warnings being treated as errors
In file included from include/jwk_int.h:8,
                 from jwk.c:8:
../include/cjose/jwk.h:283: warning: type qualifiers ignored on function return type
jwk.c:1003: warning: type qualifiers ignored on function return type
Error while executing cc -DPACKAGE_NAME="cjose" -DPACKAGE_TARNAME="cjose" -DPACKAGE_VERSION="0.6.1" -DPACKAGE_STRING="cjose 0.6.1" -DPACKAGE_BUGREPORT="" -DPACKAGE_URL="" -DPACKAGE="cjose" -DVERSION="0.6.1" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=".libs/" -DHAVE_LIBCRYPTO=1 -DHAVE_LIBJANSSON=1 -I. -I/include -I/usr/local/include/ -std=gnu99 --pedantic -Wall -Werror -g -O2 -I../include -O2 -pipe -I/usr/local/include/ -MT libcjose_la-jwk.lo -MD -MP -MF .deps/libcjose_la-jwk.Tpo -c jwk.c -fPIC -DPIC -o
.libs/libcjose_la-jwk.o
-->8--

Please drop the -Werror, it's a recipe for problems.  The "-g -O2"
part should be dropped too, even if -O2 is overriden later ("-O2 -pipe").
A cheap way to do this is to patch src/Makefile.in and test/Makefile.in.

devel/check should be added to BUILD_DEPENDS instead of TEST_DEPENDS, so
that configure can detect it and activate regress tests.

COMMENT should not start with a capital letter ("Implementation") unless
for good reasons. Here I would suggest something like:

  COMMENT-main = Javascript Object Signing and Encryption library

COMMENT-docs could be improved ("documetations" -> "documentation").

DESCR could be improved.  I would suggest:

  cjose is a library implementing Javascript Object Signing and
  Encryption (JOSE).

and maybe a brief list of use cases for JOSE (RFCs?)

Regarding the docs: doxygen updates can be painful, so mechanically
adding API docs is not a good approach.  I would suggest dropping the
doxygen docs, unless of course if you really see value in them.

Except for these points, LGTM.
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: [NEW] devel/cjose

YASUOKA Masahiko-4
Hi,

On Tue, 15 Jan 2019 07:19:54 +0100
Jeremie Courreges-Anglas <[hidden email]> wrote:

> On Fri, Dec 28 2018, YASUOKA Masahiko <[hidden email]> wrote:
>> Hi,
>>
>> I'd like to add devel/cjose.  It's a C library of JOSE (JavaScript
>> Object Signing and Encryption).  JOSE is used in "OpenID connect"
>> protocol.
>>
>> The original diff is written by my colleague, Naoaki Hoshi.
>>
>> ok?
>
> Fails to build on gcc archs, so not ok:
> --8<--
> cc1: warnings being treated as errors
> In file included from include/jwk_int.h:8,
>                  from jwk.c:8:
> ../include/cjose/jwk.h:283: warning: type qualifiers ignored on function return type
> jwk.c:1003: warning: type qualifiers ignored on function return type
> Error while executing cc -DPACKAGE_NAME="cjose" -DPACKAGE_TARNAME="cjose" -DPACKAGE_VERSION="0.6.1" -DPACKAGE_STRING="cjose 0.6.1" -DPACKAGE_BUGREPORT="" -DPACKAGE_URL="" -DPACKAGE="cjose" -DVERSION="0.6.1" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=".libs/" -DHAVE_LIBCRYPTO=1 -DHAVE_LIBJANSSON=1 -I. -I/include -I/usr/local/include/ -std=gnu99 --pedantic -Wall -Werror -g -O2 -I../include -O2 -pipe -I/usr/local/include/ -MT libcjose_la-jwk.lo -MD -MP -MF .deps/libcjose_la-jwk.Tpo -c jwk.c -fPIC -DPIC -o
> .libs/libcjose_la-jwk.o
> -->8--
>
> Please drop the -Werror, it's a recipe for problems.  The "-g -O2"
> part should be dropped too, even if -O2 is overriden later ("-O2 -pipe").
> A cheap way to do this is to patch src/Makefile.in and test/Makefile.in.
Dropped the flags from src/Makefile.am and changed Makefile to do
autoreconf.

Also I tested building this on Octeon.

> devel/check should be added to BUILD_DEPENDS instead of TEST_DEPENDS, so
> that configure can detect it and activate regress tests.

done.

> COMMENT should not start with a capital letter ("Implementation") unless
> for good reasons. Here I would suggest something like:
>
>   COMMENT-main = Javascript Object Signing and Encryption library

I took this.

> DESCR could be improved.  I would suggest:
>
>   cjose is a library implementing Javascript Object Signing and
>   Encryption (JOSE).
>
> and maybe a brief list of use cases for JOSE (RFCs?)

Added some use cases.

  ***
  cjose is a library implementing Javascript Object Signing and Encryption
  (JOSE).  JOSE consists of JSON Web Signature (JWS, RFC 7515), JSON Web
  Encryption (JWE, RFC 7516) and JSON Web Key (JWK, RFC 7517) and is used
  in some protocols (eg. OAUTH, OpenID Connect or XMPP).
  ***

> Regarding the docs: doxygen updates can be painful, so mechanically
> adding API docs is not a good approach.  I would suggest dropping the
> doxygen docs, unless of course if you really see value in them.

ok.  I dropped "docs" subpackage.

Also, "make test" didn't pass on Octeon.  The upstream seems to fix
this issue on master already.  So I took the fix and the diff for
another memory leak together, put them into patches/.

> Except for these points, LGTM.

Thanks.  Let me update the tar.gz.

ok?

cjose.tar.gz (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [NEW] devel/cjose

Jeremie Courreges-Anglas-2

On Tue, Jan 29 2019, YASUOKA Masahiko <[hidden email]> wrote:

> Hi,
>
> On Tue, 15 Jan 2019 07:19:54 +0100
> Jeremie Courreges-Anglas <[hidden email]> wrote:
>> On Fri, Dec 28 2018, YASUOKA Masahiko <[hidden email]> wrote:
>>> Hi,
>>>
>>> I'd like to add devel/cjose.  It's a C library of JOSE (JavaScript
>>> Object Signing and Encryption).  JOSE is used in "OpenID connect"
>>> protocol.
>>>
>>> The original diff is written by my colleague, Naoaki Hoshi.
>>>
>>> ok?
>>
>> Fails to build on gcc archs, so not ok:
>> --8<--
>> cc1: warnings being treated as errors
>> In file included from include/jwk_int.h:8,
>>                  from jwk.c:8:
>> ../include/cjose/jwk.h:283: warning: type qualifiers ignored on function return type
>> jwk.c:1003: warning: type qualifiers ignored on function return type
>> Error while executing
>> cc -DPACKAGE_NAME="cjose" -DPACKAGE_TARNAME="cjose" -DPACKAGE_VERSION="0.6.1" -DPACKAGE_STRING="cjose
>> 0.6.1" -DPACKAGE_BUGREPORT="" -DPACKAGE_URL="" -DPACKAGE="cjose" -DVERSION="0.6.1" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=".libs/" -DHAVE_LIBCRYPTO=1 -DHAVE_LIBJANSSON=1 -I. -I/include -I/usr/local/include/ -std=gnu99 --pedantic -Wall -Werror -g -O2 -I../include -O2 -pipe -I/usr/local/include/ -MT
>> libcjose_la-jwk.lo -MD -MP -MF .deps/libcjose_la-jwk.Tpo -c
>> jwk.c -fPIC -DPIC -o
>> .libs/libcjose_la-jwk.o
>> -->8--
>>
>> Please drop the -Werror, it's a recipe for problems.  The "-g -O2"
>> part should be dropped too, even if -O2 is overriden later ("-O2 -pipe").
>> A cheap way to do this is to patch src/Makefile.in and test/Makefile.in.
>
> Dropped the flags from src/Makefile.am and changed Makefile to do
> autoreconf.
Do you want to patch Makefile.am so that you can push the diff upstream?
If so, dropping "-g -O2" there makes sense because those are the
defaults used by autotools.

But then, you'll still have to convince upstream to also drop -Werror
from the default CFLAGS (because it's harmful on systems/architectures
they may not know about).

Anyway, if you want to patch Makefile.am instead of Makefile.in, you
need to declare a BUILD_DEPENDS on autoconf/automake.  Something like

BUILD_DEPENDS = devel/check \
              ${MODGNU_AUTOCONF_DEPENDS} \
              ${MODGNU_AUTOMAKE_DEPENDS}

While here, use the new "do-gen" step instead of "pre-configure".

[...]

> Added some use cases.
>
>   ***
>   cjose is a library implementing Javascript Object Signing and Encryption
>   (JOSE).  JOSE consists of JSON Web Signature (JWS, RFC 7515), JSON Web
>   Encryption (JWE, RFC 7516) and JSON Web Key (JWK, RFC 7517) and is used
>   in some protocols (eg. OAUTH, OpenID Connect or XMPP).
>   ***

thx, that's probably enough information for any user.  I've tweaked the
end of it to reduce the number of parens.

>> Regarding the docs: doxygen updates can be painful, so mechanically
>> adding API docs is not a good approach.  I would suggest dropping the
>> doxygen docs, unless of course if you really see value in them.
>
> ok.  I dropped "docs" subpackage.

It's not that simple: if doxygen is present at configure time, it will
be used at "make all" time, but dpb can remove doxygen between those two
steps.

--disable-doxygen-docs should be the solution, but it isn't: doxygen is
still used at "make fake" time if doxygen is detected at configure time.
So I propose to just unhook the "doc" directory.

And, obviously, we need to remove post-install from our Makefile, since
that step tries to install the doxygen docs. ;)

> Also, "make test" didn't pass on Octeon.  The upstream seems to fix
> this issue on master already.  So I took the fix and the diff for
> another memory leak together, put them into patches/.

Hah, indeed.  make test also passes on sparc64.

>> Except for these points, LGTM.
>
> Thanks.  Let me update the tar.gz.

Here's an updated tarball with all the fixes mentioned above.



> ok?

ok jca@ to import the attached tarball.

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

cjose.3.tgz (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [NEW] devel/cjose

YASUOKA Masahiko-4
Hi,

On Wed, 30 Jan 2019 02:13:44 +0100
Jeremie Courreges-Anglas <[hidden email]> wrote:

> On Tue, Jan 29 2019, YASUOKA Masahiko <[hidden email]> wrote:
>> On Tue, 15 Jan 2019 07:19:54 +0100
>> Jeremie Courreges-Anglas <[hidden email]> wrote:
>>> On Fri, Dec 28 2018, YASUOKA Masahiko <[hidden email]> wrote:
>>>> Hi,
>>>>
>>>> I'd like to add devel/cjose.  It's a C library of JOSE (JavaScript
>>>> Object Signing and Encryption).  JOSE is used in "OpenID connect"
>>>> protocol.
>>>>
>>>> The original diff is written by my colleague, Naoaki Hoshi.
>>>>
>>>> ok?
>>>
>>> Fails to build on gcc archs, so not ok:
>>> --8<--
>>> cc1: warnings being treated as errors
>>> In file included from include/jwk_int.h:8,
>>>                  from jwk.c:8:
>>> ../include/cjose/jwk.h:283: warning: type qualifiers ignored on function return type
>>> jwk.c:1003: warning: type qualifiers ignored on function return type
>>> Error while executing
>>> cc -DPACKAGE_NAME="cjose" -DPACKAGE_TARNAME="cjose" -DPACKAGE_VERSION="0.6.1" -DPACKAGE_STRING="cjose
>>> 0.6.1" -DPACKAGE_BUGREPORT="" -DPACKAGE_URL="" -DPACKAGE="cjose" -DVERSION="0.6.1" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=".libs/" -DHAVE_LIBCRYPTO=1 -DHAVE_LIBJANSSON=1 -I. -I/include -I/usr/local/include/ -std=gnu99 --pedantic -Wall -Werror -g -O2 -I../include -O2 -pipe -I/usr/local/include/ -MT
>>> libcjose_la-jwk.lo -MD -MP -MF .deps/libcjose_la-jwk.Tpo -c
>>> jwk.c -fPIC -DPIC -o
>>> .libs/libcjose_la-jwk.o
>>> -->8--
>>>
>>> Please drop the -Werror, it's a recipe for problems.  The "-g -O2"
>>> part should be dropped too, even if -O2 is overriden later ("-O2 -pipe").
>>> A cheap way to do this is to patch src/Makefile.in and test/Makefile.in.
>>
>> Dropped the flags from src/Makefile.am and changed Makefile to do
>> autoreconf.
>
> Do you want to patch Makefile.am so that you can push the diff upstream?
> If so, dropping "-g -O2" there makes sense because those are the
> defaults used by autotools.
>
> But then, you'll still have to convince upstream to also drop -Werror
> from the default CFLAGS (because it's harmful on systems/architectures
> they may not know about).

I actually didn't plan to push it.  But thank you for your comments.

> Anyway, if you want to patch Makefile.am instead of Makefile.in, you
> need to declare a BUILD_DEPENDS on autoconf/automake.  Something like
>
> BUILD_DEPENDS = devel/check \
>               ${MODGNU_AUTOCONF_DEPENDS} \
>               ${MODGNU_AUTOMAKE_DEPENDS}
>
> While here, use the new "do-gen" step instead of "pre-configure".

I see.

> [...]
>
>> Added some use cases.
>>
>>   ***
>>   cjose is a library implementing Javascript Object Signing and Encryption
>>   (JOSE).  JOSE consists of JSON Web Signature (JWS, RFC 7515), JSON Web
>>   Encryption (JWE, RFC 7516) and JSON Web Key (JWK, RFC 7517) and is used
>>   in some protocols (eg. OAUTH, OpenID Connect or XMPP).
>>   ***
>
> thx, that's probably enough information for any user.  I've tweaked the
> end of it to reduce the number of parens.

Thanks,

>>> Regarding the docs: doxygen updates can be painful, so mechanically
>>> adding API docs is not a good approach.  I would suggest dropping the
>>> doxygen docs, unless of course if you really see value in them.
>>
>> ok.  I dropped "docs" subpackage.
>
> It's not that simple: if doxygen is present at configure time, it will
> be used at "make all" time, but dpb can remove doxygen between those two
> steps.
>
> --disable-doxygen-docs should be the solution, but it isn't: doxygen is
> still used at "make fake" time if doxygen is detected at configure time.
> So I propose to just unhook the "doc" directory.

Ah, I missed that point.  Skipping "doc" makes sense.

> And, obviously, we need to remove post-install from our Makefile, since
> that step tries to install the doxygen docs. ;)

You're right, I forgot to delete the block..

>> Also, "make test" didn't pass on Octeon.  The upstream seems to fix
>> this issue on master already.  So I took the fix and the diff for
>> another memory leak together, put them into patches/.
>
> Hah, indeed.  make test also passes on sparc64.
>
>>> Except for these points, LGTM.
>>
>> Thanks.  Let me update the tar.gz.
>
> Here's an updated tarball with all the fixes mentioned above.

Thanks a lot.  I checked the tarball.

ok to commit?