Re: Latest libnghttp2 checkin broken

[Joerg Mayer <jmayer () loplof de>]

Hello Alexis,

On Fri, May 01, 2015 at 03:23:37PM +0200, Alexis La Goutte wrote:
> On Fri, May 1, 2015 at 2:17 PM, Joerg Mayer <jmayer () loplof de> wrote:
>
> > On Fri, May 01, 2015 at 10:42:01AM +0200, Alexis La Goutte wrote:
> > > On Fri, May 1, 2015 at 1:46 AM, Joerg Mayer <jmayer () loplof de> wrote:
> > >
> > > > The latest checkin to libnghttp2 should not have happend:
> > > > a) it breaks compilation on my system
> > > >   /home/jmayer/work/wireshark/git/epan/nghttp2/nghttp2_hd.c: In
> > function
> > > > ‘hd_inflate_remove_bufs_with_name’:
> > > > /home/jmayer/work/wireshark/git/epan/nghttp2/nghttp2_hd.c:1736:10:
> > error:
> > > > variable ‘rv’ set but not used [-Werror=unused-but-set-variable]
> > > >    size_t rv;
> > > >           ^
> > > What compile and release do you using ?
> >
> > I have found and fixed the problem: I did a non-debug build (see commit
> > f708c5cb56135515b6b777f144e90d488870c470)
> > So we would probably need at least one buildbot to build with NDEBUG set to
> > catch this.
> What is a "NDEGUG" build ?
> Because, i will push the patch to the upstream

A build without debugging code, i.e. assert(stuff) evaluates to nothing.
Call it a release build if you want.

> > > > b) it can only have been committed with --no-verify as it contains
> > > > deprecated
> > > >    functions (checkAPI.pl). I hit this during conflict resolution (git
> > > > commit).
> > > >
> > > > pre-commit tools is no perfect... it is for help when commit code and
> > > there is some false postive...
> >
> > Please verify on your system and let me know the results. Here's the
> > result on mine:
> > jmayer@egg nghttp2(master)$ ../../tools/checkAPIs.pl *
> > Error: Found prohibited APIs in nghttp2_helper.c: ntohl,ntohs,htonl,htons
> > Error: Found prohibited APIs in nghttp2_mem.c: malloc,calloc,realloc,free
> > Error: Found prohibited APIs in nghttp2_net.h: ntohl,ntohs,htonl,htons
> For me checkAPIs is only for dissectors file
> You have the same type of warning on wiretap folder :
>
> wireshark/wiretap$ ../tools/checkAPIs.pl *
> Error: Found prohibited APIs in ascend.c: malloc,free
> Error: Found prohibited APIs in ascend_scanner.c: malloc,realloc,free
> Error: Found prohibited APIs in k12text.c: malloc,realloc,free

No, I don't, because I do out of tree builds and all of these are generated
file ;-)

What I don't understand is, that if you have the pre-commit hook installed,
you should never have been able to commit these files.
I hit the problem when I did a "git commit -a" during merge conflict resolution.

We currently have one file that I know of that "validly" fails the pre-commit
script, and that is doc/packet-PROTOABBREV.c. I'm not aware of any other file
(that gets checked into the repo) that should be allowed to fail the pre-commit
hook.

 Ciao
   Jörg
-- 
Joerg Mayer                                           <jmayer () loplof de>
We are stuck with technology when what we really want is just stuff that
works. Some say that should read Microsoft instead of technology.
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe