Re: wip/gnurl: Request for review

[ng0%n0.is@localhost]

Leonardo Taccari transcribed 1.7K bytes:
> Hello N.,
> Nice work, some possible comments/suggestions inline!
> (I will try to test it soon, I have only reviewed it without build
> testing it)
> 
> ng0%n0.is@localhost writes:
> > [...]
> > gnurl (https://gnunet.org/gnurl) is a microfork of curl I'm
> > maintaining for gnunet as a gnunet developer.
> > [...]
> 
> I think it is probably worth to add that informmation in DESCR and
> COMMENT too.

Can you point me to a package where something similar is pointed out?
The DESCR is already limited in space and to write that I am the
upstream maintainer would take some space. I'm open to suggestions..
maybe just in COMMENT? But then again, how and why?
 
> Other possible suggestions directly inline (based on current
> wip/gnurl/Makefile):
> 
>  | # $NetBSD$
>  | 
>  | [...]
>  | CONFIGURE_ARGS+=	--with-ssl=${BUILDLINK_PREFIX.openssl}
>  | CONFIGURE_ARGS+=	--with-ca-path=${SSLCERTS}
> 
> Are them copypastos from www/curl/Makefile? Isn't gnutls used
> instead? (If that's the case I think it can be removed)

Oh. Very likely copypasta! 

> 
>  | [...] 
>  | # We want GnuTLS with Dane.
>  | .include "../../security/gnutls/buildlink3.mk"
>  | .include "../../security/gnutls/libgnutls-config.mk"
>  | CONFIGURE_ARGS+= --with-gnutls
>  | 
>  | .include "../../devel/libidn2/buildlink3.mk"
>  | [...] 
> 
> Minor nitpick: it is probably better to add all bl3 inclusion at
> the end for consistency.

Okay, noted.
 
> Other suggestions regarding buildlink3.mk:
> 
>  | [...]
>  | BUILDLINK_API_DEPENDS.gnurl+=	gnurl>=7.12.3
>  | BUILDLINK_ABI_DEPENDS.gnurl+=	gnurl>=7.61.0nb2
>  | BUILDLINK_PKGSRCDIR.gnurl?=	../../wip/gnurl
>  | [...]
> 
> This is mostly a nitpick but I think that BUILDLINK_ABI_DEPENDS.gnurl
> line can be removed and and BUILDLINK_API_DEPENDS.gnurl can be
> bumped to `gnurl>=7.63.0' (7.63.0 - apart if locally packaged - is
> the first gnurl version appeared in pkgsrc.).
> 
>  | [...]
>  | .include "../../devel/gettext-lib/buildlink3.mk"
>  | .include "../../devel/zlib/buildlink3.mk"
>  | [...]
> 
> Please also include gnutls and libidn2 bl3 (they are included
> unconditionally in the Makefile).

Okay.

Since this can build against idn conditionally (if I didn't miss
a change in curl), maybe providing an option to build against
idn or idn2 and defaulting to idn2 might also be worth adding?
I wanted to get the basic package as it is meant to function
done first.

Thanks for your review
 
> 
> Thank you!
>