I backported a couple of minor fixes to this version, yesterday. Would it be ok to replace the diff, or should i just wait until this one is committed and then send in another bug report ? Not sure what you guys like better.

Created attachment 167613 [details]
unified diff of ports/net/asterisk-chan_sccp/

Including some minor fixes

Comment 3 Raphael Kubo da Costa 2016-03-01 19:07:50 UTC

Comment on attachment 167613 [details]
unified diff of ports/net/asterisk-chan_sccp/

Please remember to set the "maintainer-approval+" flag on your patches so it's easier for other people to find maintainer updates.

(In reply to Raphael Kubo da Costa from comment #3)
Thanks for that. Wasn't sure if i was supposed to :-)

Comment 5 Raphael Kubo da Costa 2016-03-05 00:12:02 UTC

It failed to build here with 9.3-i386 on Poudriere:

In file included from ast.c:13:
../../../src/sccp_device.h:338: error: expected declaration specifiers or '...' before 'ICONV_CONST'
In file included from ast.c:15:
../../../src/sccp_utils.h:145: error: expected ')' before 'char'
*** [libast_la-ast.lo] Error code 1

Comment 6 Raphael Kubo da Costa 2016-03-05 00:13:00 UTC

While at it, I suggest you replace your several CONFIGURE_ARGS+= entries with just
  CONFIGURE_ARGS= --foo \
                  --bar \
                  --baz

Created attachment 167758 [details]
unified diff of ports/net/asterisk-chan_sccp/

Fixes issues raised by: Raphael Kubo da Costa

(In reply to Raphael Kubo da Costa from comment #6)
Thanks for reporting these to me. Running poudriere on my local system causes some real issues in general (cpu load / size / throughput).

Comment 9 Raphael Kubo da Costa 2016-03-06 13:13:40 UTC

The code is still failing to build on 9.3 as described in comment #5.

(In reply to Raphael Kubo da Costa from comment #9)
I have changed the source code to include "config.h" in both source files, which should have taken care of the issue reported.

I am not sure if 9.3 does include iconv in the same way that 10.2 does. But even if not provided, then configure should have detected that and not set ICONV_CONST. I have test this on my local system (with and without iconv), which compiled cleanly.

Is there any way to run poudriere somewhere in a hosted environment, or dig through the configure / make logging somewhere ?

BTW: I am not currently a regular BSD user, so going through a bit of a learning curve here. I used to use BSD regularly but that is a comple of eon's ago ;-)

Comment 11 Raphael Kubo da Costa 2016-03-06 15:39:13 UTC

(In reply to Diederik de Groot from comment #10)
> (In reply to Raphael Kubo da Costa from comment #9)
> I have changed the source code to include "config.h" in both source files,
> which should have taken care of the issue reported.
>
> I am not sure if 9.3 does include iconv in the same way that 10.2 does. But
> even if not provided, then configure should have detected that and not set
> ICONV_CONST. I have test this on my local system (with and without iconv),
> which compiled cleanly.

iconv support is a problematic topic: basically in 9.x we still shipped with iconv(3) that used a const in the second argument, which doesn't match most implementations out there. This has been fixed in 10.x+, which is why you didn't see any problems in your tests. Mk/Uses/iconv.mk contains all the hairy details that make USES=iconv work on all supported FreeBSD versions.

The whole build issue is happening because the configure script was considering iconv was not installed even though it is present and must be used. The ICONV_CONST macro was then not being defined anywhere and the compiler thought it was a symbol it did not recognize.

This is caused by the way the iconv m4 and autotools code is written: --with-libiconv-prefix=${LOCALBASE} (which is what you get with ${ICONV_CONFIGURE_ARG}) is not enough and the iconv(3) detection code only works when iconv's include path and library are passed to the compiler/linker.

This can be fixed by adding a few more variables to the Makefile:
    CPPFLAGS+=-I${ICONV_INCLUDE_PATH}
    LDFLAGS+= -L${LOCALBASE}/lib ${ICONV_LIB}

Comment 12 Raphael Kubo da Costa 2016-03-06 15:41:20 UTC

(In reply to Diederik de Groot from comment #10)
> Is there any way to run poudriere somewhere in a hosted environment, or dig
> through the configure / make logging somewhere ?
> 
> BTW: I am not currently a regular BSD user, so going through a bit of a
> learning curve here. I used to use BSD regularly but that is a comple of
> eon's ago ;-)

Unfortunately I'm not aware of any free offerings of this kind of service. Running your own Poudriere instance is not difficult, but you do need some computing power for that to work minimally fast.

If you don't have access to the necessary resources, we can continue doing the required checks for you, it just takes longer for the patches to be committed :-)

(In reply to Raphael Kubo da Costa from comment #11)

So a simple solution would be to make sure ICONV_CONST is always added to config.h (defined as '') even if HAVE_ICONV is not going to be defined, that way the header file would still be fine. Bit of an ugly hack maybe.

> CPPFLAGS+=-I${ICONV_INCLUDE_PATH}
> LDFLAGS+= -L${LOCALBASE}/lib ${ICONV_LIB}
</quote>
ANd that would also work correctly for FreeBSD 10+, right. That's a simple change to make. Committing another verion of the patch in a couple of seconds.

Created attachment 167768 [details]
unified diff of ports/net/asterisk-chan_sccp/

Adding 
CPPFLAGS+=             -I${ICONV_INCLUDE_PATH}
LDFLAGS+=              -L${LOCALBASE}/lib ${ICONV_LIB}

(In reply to Raphael Kubo da Costa from comment #12)
Ooh that's a shame. Thanks for helping me out !

Comment 16 Raphael Kubo da Costa 2016-03-06 16:19:49 UTC

Thanks, that version builds fine on both 9.3-i386 and 11-amd64 on my Poudriere tests.

I'll land a slightly modified version:
* Instead of doing "CONFIGURE_ARGS+=${ICONV_CONFIGURE_ARG}", I'll add ICONV_CONFIGURE_ARG to the original CONFIGURE_ARGS definition a few lines before.
* There was a double assignment to LDFLAGS because it was already being set before this change, I've removed the second one.

Comment 17 commit-hook 2016-03-06 16:21:26 UTC

A commit references this bug:

Author: rakuco
Date: Sun Mar  6 16:20:40 UTC 2016
New revision: 410464
URL: https://svnweb.freebsd.org/changeset/ports/410464

Log:
  Update to 4.2.3.

  New Features in V4.2.3
  - Warning about use of ancient gcc compiler
  - New desktop/background images
  - New AMI Function:SCCPShowRefcount
  - Support for newer/future version of automake
  - Small size reduction of chan_sccp.so

  Bug Fixes
  - Channel being left behind after hangup
  - Scheduled Hangup / Schedule Digit Timeout
  - Potential Deadlock Fallback Atomic Functions
  - MWI loosing track of new voicemails under certain circumstances
  - using gnu version of libiconv fixed
  - gen_sccp_enum improved / gnu awk extension dependency removed
  - set socket option should happen before binding to the socket
  - channel monitor/record handling fixed
  - Code cleanup / fixed memory leaks / fixed potential null pointer dereferences
  - Some smaller patches (Check ChangeLog for details)
  - Some minor BSD Compilation issues

  PR:		207291
  Submitted by:	Diederik de Groot <ddegroot@talon.nl> (maintainer)

Changes:
  head/net/asterisk-chan_sccp/Makefile
  head/net/asterisk-chan_sccp/distinfo

Comment 18 Raphael Kubo da Costa 2016-03-06 16:21:52 UTC

Finally committed, thanks for the patch!

(In reply to Raphael Kubo da Costa from comment #16)
I was in a hurry to leave the house, so just patched up the diff file using your previous comments :-) Sorry about that one. Thanks for fixing the patch.

Thanks for your help and for committing it !