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: |
|
||||||||||||||||||
Any reason not to do the CXX= -> CXX?= for all architectures? Created attachment 200558 [details]
v2
Also fix double https in pkg-descr.
Hi! What is the reason for the extra includes? 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.
This feels a little strange. I'm surprised if clang automatically includes them. Could be differences in libc++ and GCC's counterpart perhaps? (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. (In reply to Piotr Kubaj from comment #6) Are they using the C or C++ compiler? (In reply to Niclas Zeising from comment #7) I don't really remember, I didn't pay attention to that. 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 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. (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. 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. (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. (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. 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 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! Just to be clear, since there's no flag in bugzilla for it, I'm waiting for feedback from submitter. (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. (In reply to Niclas Zeising from comment #17) Just to be clear, are you waiting for more feedback? Take, as this PR seems to be stuck. I will test with only the one patch. (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. (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. Created attachment 205533 [details]
bredbandskollen update
I have this patch, can you please use that as a basis instead?
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.
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. 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.
(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*}. Created attachment 205554 [details]
Update bredbandskollen and fix build with gcc
Yeah, my bad. Here is an updated patch.
Created attachment 205555 [details]
Update bredbandskollen and fix build with gcc
And here is really really the new patch.
(In reply to Niclas Zeising from comment #29) bredbandskollen builds with this patch on powerpc64. 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 Committed |
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.