Bug 234216

Summary: net/bredbandskollen: fix build with GCC-based architectures
Product: Ports & Packages Reporter: Piotr Kubaj <pkubaj>
Component: Individual Port(s)Assignee: Mark Linimon <linimon>
Status: Closed FIXED    
Severity: Affects Only Me CC: linimon, pkubaj, zeising
Priority: --- Flags: bugzilla: maintainer-feedback? (zeising)
Version: Latest   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
patch
none
v2
none
v3
none
bredbandskollen update
none
Update bredbandskollen and fix build with gcc
none
Update bredbandskollen and fix build with gcc
none
Update bredbandskollen and fix build with gcc none

Description Piotr Kubaj freebsd_committer 2018-12-20 14:45:55 UTC
Created attachment 200304 [details]
patch

This port needs USES=compiler:c++11-lang to build on GCC architectures. It also specifies directly CXX with =, replace that with ?=.

Add missing includes (sys/types.h and sys/select.h) that GCC doesn't include by default.

Tested on powerpc64 and amd64.

Hardware sponsored by IntegriCloud.
Comment 1 Niclas Zeising freebsd_committer 2018-12-20 20:04:48 UTC
Any reason not to do the CXX= -> CXX?= for all architectures?
Comment 2 Piotr Kubaj freebsd_committer 2018-12-27 12:34:41 UTC
Created attachment 200558 [details]
v2

Also fix double https in pkg-descr.
Comment 3 Niclas Zeising freebsd_committer 2019-02-21 18:32:15 UTC
Hi!
What is the reason for the extra includes?
Comment 4 Piotr Kubaj freebsd_committer 2019-02-21 18:58:57 UTC
Created attachment 202235 [details]
v3

Sorry for the lag, I just forgot about this PR.
CXX can be applied everywhere.

Extra includes are needed because GCC doesn't include them by default, which results in build failure.
Comment 5 Niclas Zeising freebsd_committer 2019-02-21 19:14:11 UTC
This feels a little strange.  I'm surprised if clang automatically includes them.  Could be differences in libc++ and GCC's counterpart perhaps?
Comment 6 Piotr Kubaj freebsd_committer 2019-02-21 19:24:08 UTC
(In reply to Niclas Zeising from comment #5)
It's probable, I'm not really sure (I'm neither LLVM nor GCC developer).

But I noticed it happened also with other ports. Quite many ports need including sys/types.h when compiling with GCC.
Comment 7 Niclas Zeising freebsd_committer 2019-02-21 19:34:55 UTC
(In reply to Piotr Kubaj from comment #6)

Are they using the C or C++ compiler?
Comment 8 Piotr Kubaj freebsd_committer 2019-02-21 19:52:32 UTC
(In reply to Niclas Zeising from comment #7)
I don't really remember, I didn't pay attention to that.
Comment 9 commit-hook freebsd_committer 2019-05-29 07:36:39 UTC
A commit references this bug:

Author: pkubaj
Date: Wed May 29 07:35:45 UTC 2019
New revision: 502934
URL: https://svnweb.freebsd.org/changeset/ports/502934

Log:
  net/bredbandskollen: fix build with GCC-based architectures, fix pkg-descr

  This port needs USES=compiler:c++11-lang to build on GCC architectures. It also specifies directly CXX with =, replace that with ?=.

  Add missing includes (sys/types.h and sys/select.h) that GCC doesn't include by default.

  Also fix double https in pkg-descr.

  PR:		234216
  Approved by:	tcberner (mentor), zeising (maintainer timeout)
  Differential Revision:	https://reviews.freebsd.org/D20442

Changes:
  head/net/bredbandskollen/Makefile
  head/net/bredbandskollen/files/
  head/net/bredbandskollen/files/patch-src_framework_engine.h
  head/net/bredbandskollen/files/patch-src_framework_socketreceiver.h
  head/net/bredbandskollen/pkg-descr
Comment 10 Niclas Zeising freebsd_committer 2019-05-29 08:24:31 UTC
Hi!
This was not maintainer timeout.
I asked questions from the sumbitter, for which I did not get an answer. Please revert it.

The patch is wrong, since bredbandkskollen does not use C++, which I asked about, but you never replied.
Comment 11 Piotr Kubaj freebsd_committer 2019-05-29 08:37:22 UTC
(In reply to Niclas Zeising from comment #10)
You asked about other ports and what programming language they use, which I answered.

I don't really make any statistics about which ports that fail use which programming language, why would I?

If the port doesn't use C++, what does it use $CXX variable for?

Why do I see in failed build logs lines like:
c++ -MM -O2 -pipe -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../http/websocketbridge.o ../http/websocketbridge.cpp > ../http/websocketbridge.d
c++ -MM -O2 -pipe -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../http/httprequestengine.o ../http/httprequestengine.cpp > ../http/httprequestengine.d
c++ -MM -O2 -pipe -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../http/httpserverconnection.o ../http/httpserverconnection.cpp > ../http/httpserverconnection.d
cc1plus: error: unrecognized command line option "-std=c++11"cc1plus: error: unrecognized command line option "-std=c++11"

cc1plus: error: unrecognized command line option "-std=c++11"
cc1plus: error: unrecognized command line option "-std=c++11"

As you can see, it does use C++.

Here's a build log from pylon: http://pylon.nyi.freebsd.org/data/head-powerpc64-default/p502184_s348025/logs/errors/bredbandskollen-0.20181210.log

It fails because of no C++11 as well.

And as it happens, my patch also fixes the build issues.
Comment 12 Niclas Zeising freebsd_committer 2019-05-29 10:47:13 UTC
Hi!
You sent the patch, you claimed build errors (without showing a log, or even snippet of a log), so it must be on you to answer questions and defend the patch.  The only reply I got from you was a "I don't remember", in comment #8.

There is definitely something weird going on here, especially sine bredbandskollen is presumably compiled with gcc on other platforms.  I ask of you to revert this, since it was committed  in error, and the patch might not be correct.
Comment 13 Piotr Kubaj freebsd_committer 2019-05-29 11:04:49 UTC
(In reply to Niclas Zeising from comment #12)
I replied "I don't know" because why would I remember about what programming language ports that fail to build are written in? This is simply not relevant.
Other than that, I answered all your questions according to my knowledge, but you still didn't answer.

Also, it's not only that GCC architectures use c++.

Clang architectures do it as well (poudriere testport with my patch):
===>  Building for bredbandskollen-0.20181210
gmake[1]: Entering directory '/wrkdirs/usr/ports/net/bredbandskollen/work/bbk-39b47a1/src/cli'
c++ -MM -O2 -pipe -fstack-protector-all -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../framework/loadbalancer.o ../framework/loadbalancer.cpp > ../framework/loadbalancer.d
c++ -MM -O2 -pipe -fstack-protector-all -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../framework/synchronousbridge.o ../framework/synchronousbridge.cpp > ../framework/synchronousbridge.d
c++ -MM -O2 -pipe -fstack-protector-all -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../framework/bridgetask.o ../framework/bridgetask.cpp > ../framework/bridgetask.d
c++ -MM -O2 -pipe -fstack-protector-all -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../framework/logger.o ../framework/logger.cpp > ../framework/logger.d
c++ -MM -O2 -pipe -fstack-protector-all -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../framework/socketreceiver.o ../framework/socketreceiver.cpp > ../framework/socketreceiver.d
c++ -MM -O2 -pipe -fstack-protector-all -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../framework/serversocket.o ../framework/serversocket.cpp > ../framework/serversocket.d
c++ -MM -O2 -pipe -fstack-protector-all -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../framework/socketconnection.o ../framework/socketconnection.cpp > ../framework/socketconnection.d
c++ -MM -O2 -pipe -fstack-protector-all -fno-strict-aliasing   -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -MT ../framework/socket.o ../framework/socket.cpp > ../framework/socket.d

Since the patch is ok and you didn't replied in 3 months, I fail to see why I would revert it.
Comment 14 Mark Linimon freebsd_committer freebsd_triage 2019-05-29 12:20:00 UTC
(In reply to Piotr Kubaj from comment #13)

You have to do the revert -- because the maintainer said so.

There is obviously some kind of communication problem here.  Let's take this conversation offline, please.
Comment 15 commit-hook freebsd_committer 2019-05-29 12:38:01 UTC
A commit references this bug:

Author: pkubaj
Date: Wed May 29 12:37:15 UTC 2019
New revision: 502955
URL: https://svnweb.freebsd.org/changeset/ports/502955

Log:
  net/bredbandskollen: revert 502934

  PR:		234216
  Submitted by:	zeising

Changes:
  head/net/bredbandskollen/Makefile
  head/net/bredbandskollen/files/
  head/net/bredbandskollen/pkg-descr
Comment 16 Niclas Zeising freebsd_committer 2019-05-30 08:17:45 UTC
After looking some more at this, I have more comments.

I did some tests with gcc8 on amd64, and I can see the need for sys/select.h in framework/engine.h, however, I can see no need to include sys/types.h in framework/socketreceiver.h.  Bredbandskollen builds fine without that patch, and I can't see anything in that file that warrants that inclusion.

Can you give me a snip of a log for that error?
Thanks!
Comment 17 Niclas Zeising freebsd_committer 2019-05-30 08:24:44 UTC
Just to be clear, since there's no flag in bugzilla for it, I'm waiting for feedback from submitter.
Comment 18 Piotr Kubaj freebsd_committer 2019-05-31 12:43:23 UTC
(In reply to Niclas Zeising from comment #17)
g++8 -O2 -pipe  -fstack-protector-strong -Wl,-rpath=/usr/local/lib/gcc8  -Wl,-rpath=/usr/local/lib/gcc8  -O2 -W -Wall -I.. -std=c++11 -DTASKRUNNER_LOGERR -DTASKRUNNER_LOGWARN -DTASKRUNNER_LOGINFO -c ../framework/socketreceiver.cpp -o ../framework/socketreceiver.o
In file included from /usr/include/sys/socket.h:41,
                 from ../framework/socketreceiver.h:6,
                 from ../framework/socketreceiver.cpp:6:
../framework/socketreceiver.h:36:18: error: 'register_t' was not declared in this scope
     char cmsgbuf[CMSG_SPACE(sizeof(int))];
                  ^~~~~~~~~~
../framework/socketreceiver.h:36:18: note: suggested alternative: 'register'
../framework/socketreceiver.h:36:18: error: 'register_t' was not declared in this scope
     char cmsgbuf[CMSG_SPACE(sizeof(int))];
                  ^~~~~~~~~~
../framework/socketreceiver.h:36:18: note: suggested alternative: 'register'
../framework/socketreceiver.h:36:18: error: 'register_t' was not declared in this scope
     char cmsgbuf[CMSG_SPACE(sizeof(int))];
                  ^~~~~~~~~~
../framework/socketreceiver.h:36:18: note: suggested alternative: 'register'
../framework/socketreceiver.h:36:18: error: 'register_t' was not declared in this scope
     char cmsgbuf[CMSG_SPACE(sizeof(int))];
                  ^~~~~~~~~~
../framework/socketreceiver.h:36:18: note: suggested alternative: 'register'
../framework/socketreceiver.cpp: In constructor 'SocketReceiver::SocketReceiver(Task*, int, pid_t)':
../framework/socketreceiver.cpp:19:30: error: 'cmsgbuf' was not declared in this scope
     fdpass_msg.msg_control = cmsgbuf;
                              ^~~~~~~
../framework/socketreceiver.cpp:19:30: note: suggested alternative: 'cmsg'
     fdpass_msg.msg_control = cmsgbuf;
                              ^~~~~~~
                              cmsg
In file included from /usr/include/sys/socket.h:41,
                 from ../framework/socketreceiver.h:6,
                 from ../framework/socketreceiver.cpp:6:
../framework/socketreceiver.cpp:24:22: error: 'register_t' was not declared in this scope
     cmsg->cmsg_len = CMSG_LEN(sizeof(int));
                      ^~~~~~~~
../framework/socketreceiver.cpp:24:22: note: suggested alternative: 'register'
../framework/socketreceiver.cpp: In member function 'virtual SocketConnection* SocketReceiver::incoming()':
../framework/socketreceiver.cpp:37:29: error: 'cmsgbuf' was not declared in this scope
     child_msg.msg_control = cmsgbuf;
                             ^~~~~~~
../framework/socketreceiver.cpp:37:29: note: suggested alternative: 'cmsg'
     child_msg.msg_control = cmsgbuf;
                             ^~~~~~~
                             cmsg
In file included from /usr/include/sys/socket.h:41,
                 from ../framework/socketreceiver.h:6,
                 from ../framework/socketreceiver.cpp:6:
../framework/socketreceiver.cpp:69:20: error: 'register_t' was not declared in this scope
     memcpy(&newfd, CMSG_DATA(cmsg), sizeof(newfd));
                    ^~~~~~~~~
../framework/socketreceiver.cpp:69:20: note: suggested alternative: 'register'
../framework/socketreceiver.cpp: In member function 'bool SocketReceiver::passSocketToPeer(int)':
../framework/socketreceiver.cpp:80:12: error: 'register_t' was not declared in this scope
     memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
            ^~~~~~~~~
../framework/socketreceiver.cpp:80:12: note: suggested alternative: 'register'
gmake[2]: *** [../framework/mk.inc:105: ../framework/socketreceiver.o] Error 1
gmake[2]: Leaving directory '/usr/local/poudriere/ports/default/net/bredbandskollen/work/bbk-39b47a1/src/cli'
===> Compilation failed unexpectedly.
Comment 19 Piotr Kubaj freebsd_committer 2019-06-19 15:12:36 UTC
(In reply to Niclas Zeising from comment #17)
Just to be clear, are you waiting for more feedback?
Comment 20 Mark Linimon freebsd_committer freebsd_triage 2019-07-05 16:10:47 UTC
Take, as this PR seems to be stuck.  I will test with only the one patch.
Comment 21 Niclas Zeising freebsd_committer 2019-07-05 16:25:33 UTC
(In reply to Mark Linimon from comment #20)
> Take, as this PR seems to be stuck.  I will test with only the one patch.

It is not stuck, however, I can't reproduce all the issues, and I've had more important things going on in xorg land, so I have had to prioritize.
Comment 22 Niclas Zeising freebsd_committer 2019-07-05 16:26:41 UTC
(In reply to Niclas Zeising from comment #21)
> (In reply to Mark Linimon from comment #20)
> > Take, as this PR seems to be stuck.  I will test with only the one patch.
> 
> It is not stuck, however, I can't reproduce all the issues, and I've had
> more important things going on in xorg land, so I have had to prioritize.

And I want a solution that is upstreamable, it is not a sustainable solution to keep local patches for everything.
Comment 23 Niclas Zeising freebsd_committer 2019-07-05 17:03:06 UTC
Created attachment 205533 [details]
bredbandskollen update

I have this patch, can you please use that as a basis instead?
Comment 24 Niclas Zeising freebsd_committer 2019-07-05 18:46:11 UTC
Hi!
The CMSG_ macros are from our own sys/socket.h.  What I can't understand is where the register_t comes from.  The bredbandskollen code never uses it.

Looking at the preprocessor output on amd64, with gcc8 (which doesn't have this issue), 

char cmsgbuf[CMSG_SPACE(sizeof(int))];

expands to

char cmsgbuf[
# 36 "../framework/socketreceiver.h" 3 4
                ((((__uintptr_t)(sizeof(struct cmsghdr)) + (sizeof(__register_t) - 1)) & ~(sizeof(__register_t) - 1)) + (((__uintptr_t)(
# 36 "../framework/socketreceiver.h"
                sizeof(int)
# 36 "../framework/socketreceiver.h" 3 4
                ) + (sizeof(__register_t) - 1)) & ~(sizeof(__register_t) - 1)))
# 36 "../framework/socketreceiver.h"
                                       ];


The macro CMSG_SPACE macro looks like this (from sys/sys/socket.h)

#define CMSG_SPACE(l)           (_ALIGN(sizeof(struct cmsghdr)) + _ALIGN(l))

I wonder if _ALIGN expands to it, but I can't find that define.

On my amd64 system, through a series of includes, it looks like sys/socket.h includes x86/_types.h, which has a typedef for __register_t.
Comment 25 Niclas Zeising freebsd_committer 2019-07-05 18:57:46 UTC
So.  On powerpc, in sys/powerpc/include/_align.h, _ALIGNBYTES is defined, and it uses register_t.  _ALIGN uses _ALIGNBYTES.

On x86 (i386 and amd64 share the same file), in sys/x86/include/_align.h, _ALIGNBYTES uses __register_t.  This is why the issue doesn't show up, I would guess, assuming that powerpc includes a similar _types.h as x86, where __register_t but not register_t has its typedef.

I believe the includepath is sys/socket.h includes sys/_types.h, which includes _machine/_types.h.  _machine/_types.h is from sys/amd64/include/_types.h which includes sys/x86/include/_types.h.  I would assume that _machine/_types.h is from sys/powerpc/include/_types.h, that file defines __register_t, but not register_t.
Comment 26 Niclas Zeising freebsd_committer 2019-07-05 19:13:20 UTC
Created attachment 205534 [details]
Update bredbandskollen and fix build with gcc

Patch updates bredbandskollen to the latest version, and tries to fix the build with gcc, and specifically powerpc.
Comment 27 Piotr Kubaj freebsd_committer 2019-07-06 12:54:46 UTC
(In reply to Niclas Zeising from comment #26)
make: "/usr/local/poudriere/ports/default/net/bredbandskollen/Makefile" line 29: Malformed conditional (${ARCH:Mpowerpc*})

You need .include <bsd.port.pre.mk> before .if ${ARCH:Mpowerpc*}.
Comment 28 Niclas Zeising freebsd_committer 2019-07-06 21:33:38 UTC
Created attachment 205554 [details]
Update bredbandskollen and fix build with gcc

Yeah, my bad.  Here is an updated patch.
Comment 29 Niclas Zeising freebsd_committer 2019-07-06 21:35:25 UTC
Created attachment 205555 [details]
Update bredbandskollen and fix build with gcc

And here is really really the new patch.
Comment 30 Piotr Kubaj freebsd_committer 2019-07-09 10:22:08 UTC
(In reply to Niclas Zeising from comment #29)
bredbandskollen builds with this patch on powerpc64.
Comment 31 commit-hook freebsd_committer 2019-07-09 19:04:00 UTC
A commit references this bug:

Author: zeising
Date: Tue Jul  9 19:03:50 UTC 2019
New revision: 506304
URL: https://svnweb.freebsd.org/changeset/ports/506304

Log:
  net/bredbandskollen: Update snapshot

  Update net/bredbandskollen to the latest snapshot.
  Fix build on GCC-based architectures, such as powerpc64 [1]

  PR:		234216 [1]
  Submitted by:	pkubaj [1]

Changes:
  head/net/bredbandskollen/Makefile
  head/net/bredbandskollen/distinfo
  head/net/bredbandskollen/files/
  head/net/bredbandskollen/files/extra-src_framework_socketreceiver.h
  head/net/bredbandskollen/files/patch-src_framework_engine.h
Comment 32 Niclas Zeising freebsd_committer 2019-07-09 19:05:03 UTC
Committed