Created attachment 205301 [details] patch When compiling with GCC, as used on GCC architectures, it errors with: sys-uuid.cc:96:12: error: 'strlen' was not declared in this scope assert(strlen(s) == 36U); ^~~~~~ sys-uuid.cc:96:12: note: 'strlen' is defined in header '<cstring>'; did you forget to '#include <cstring>'?
(In reply to Piotr Kubaj from comment #0) Hi, Thank you for your report and your patch. I think we will add an option to build with GCC (using compiler:gcc-c++11-lib) as USE_GCC={6|7|8} is not sufficient to build properly with XMP=on (the linker fails). I will work on it as soon as possible.
(In reply to Samy Mahmoudi from comment #1) Anything about this issue?
(In reply to Piotr Kubaj from comment #2) Yes, I just wrote a second patch. poudriere is now building for a port test.
Created attachment 205961 [details] Patch file generated with svn diff - Add an option to build with GCC instead of Clang - Correct DOCS into DOCS_DESC for a better description
Created attachment 205962 [details] Poudriere log - Clang
Created attachment 205963 [details] Poudriere log - GCC
N.B. With USES=compiler:gcc-c++11-lib, patching sys-uuid.cc was not necessary.
Your patch is wrong because users will need to enable GCC_BUILD option manually. The package won't be built on a package builder.
(In reply to Piotr Kubaj from comment #8) > users will need to enable GCC_BUILD option manually. That was indeed the intention. > The package won't be built on a package builder. I did not take package builders into account, wrongly. I will look for a better solution using the ARCH variable.
(In reply to Samy Mahmoudi from comment #9) You could just add to the default USES=compiler:c11. This will use ports GCC on GCC architectures and base Clang elsewhere.
(In reply to Piotr Kubaj from comment #10) Hi, This would have been simpler than using the ARCH variable. Unfortunately, with compiler:c11 (and your initial patch) the linker fails again when XMP=on. Here is what I will probably write: ----------------------------------- ... .include <bsd.port.pre.mk> .if ${ARCH:Mmips*} || ${ARCH:Mpowerpc*} || ${ARCH} == sparc64 USES+= compiler:gcc-c++11-lib .else USES+= compiler:c++11-lang .endif ... .include <bsd.port.post.mk> ----------------------------------- Is your GCC-architecture included in the previous conditionals ?
Auxiliary problem: first, I went for something like: If on GCC-architecture, then use gcc. Else, default to clang with gcc as an option. These considerations led me to: ----------------------------------- GCC_BUILD_DESC= Build with GCC (useless on GCC-architectures) ... .include <bsd.port.pre.mk> .if ${ARCH:Mmips*} || ${ARCH:Mpowerpc*} || ${ARCH} == sparc64 USES+= compiler:gcc-c++11-lib .else .if ${PORT_OPTIONS:MGCC_BUILD} USES+= compiler:gcc-c++11-lib .else USES+= compiler:c++11-lang .endif .endif ... .include <bsd.port.post.mk> ---------------------------------- It seemed that the inner conditional was not taken into account.
While investigating, it seemed even the following didn't work as expected (the following is just a test, would use an option helper in a real case): -------------------------- .include <bsd.port.pre.mk> .if ${PORT_OPTIONS:MGCC_BUILD} USES+= compiler:gcc-c++11-lib .else USES+= compiler:c++11-lang .endif .include <bsd.port.post.mk> --------------------------- By trial and error, I found that to manually test for an option, I should resort to: ------------------------------ .include <bsd.port.options.mk> .if ${PORT_OPTIONS:MGCC_BUILD} USES+= compiler:gcc-c++11-lib .else USES+= compiler:c++11-lang .endif .include <bsd.port.mk> ---------------------- This would prevent me from testing for ARCH. emulators/wine-devel is an example of a port using pre/post *.mk to test for ARCH and manually testing for STAGING on PORT_OPTIONS. Why can't I do the same ?
Created attachment 206409 [details] Patch file generated with svn diff - Use GCC on GCC-architectures - Correct DOCS into DOCS_DESC for a better description
(In reply to Samy Mahmoudi from comment #14) Auxiliary problem still holds (comment 12 and comment 13).
(In reply to Samy Mahmoudi from comment #14) This assumes that powerpc* uses GCC which won't hold true forever. In fact, powerpc* on head soon switches to clang. Why not set USES=compiler:c++11-lang?
(In reply to Samy Mahmoudi from comment #11) > .if ${ARCH:Mmips*} || ${ARCH:Mpowerpc*} || ${ARCH} == sparc64 > Is your GCC-architecture included in the previous conditionals ? Based on comment 16, I assume it is. (In reply to Piotr Kubaj from comment #16) > In fact, powerpc* on head soon switches to clang. Thanks for this information, I did not know that would happen soon, but... (In reply to Piotr Kubaj from comment #16) > This assumes that powerpc* uses GCC which won't hold true forever. ...the conditional expression was not meant to be immutable, e.g. to add new GCC-architectures when needed, or to remove the ones clang become functional on. Software sometimes needs to be maintained, even with the best software methodologies and practices. > Why not set USES=compiler:c++11-lang? This was the default under which you had to file a bug report, so this would only bring us back to Description (comment 0) and subsequent comments. Do you mean USES=compiler:c++11-lib ? I am reading Mk/Uses/compiler.mk and will give it a try. If this automatically chooses between clang and gcc depending on architecture, it would avoid us to have to maintain the conditional. This would definitely be a better practice !
Yes, USES=compiler:c++11-lang will use base clang if it's available, if only gcc is available, then it will use gcc from ports. The original patch was just to include <cstring> in sys-uuid.cc.
(In reply to Piotr Kubaj from comment #18) Unfortunately, I can only test on amd64. When testing your initial patch on this architecture, the port builds just as before. Yet, if I try to use GCC (to simulate building on a GCC architecture), your initial patch fails when linking. Have you tested your own patch on a GCC-architecture ? If you successfully did so, then my failure to build with GCC does not mirror the reality of building on GCC architectures. I would consequently trust you and approve your patch.
(In reply to Samy Mahmoudi from comment #17) Changing USES=compiler:c++11-lang into USES=compiler:c++11-lib does not make any difference.
(In reply to Samy Mahmoudi from comment #19) You can't test with GCC on amd64 unless you have world compiled with GCC because of difference in C++ symbol mangling.
A commit references this bug: Author: pkubaj Date: Wed Oct 16 15:35:58 UTC 2019 New revision: 514602 URL: https://svnweb.freebsd.org/changeset/ports/514602 Log: converters/pdf2djvu: fix build on GCC architectures When compiling with GCC, as used on GCC architectures, it errors with: sys-uuid.cc:96:12: error: 'strlen' was not declared in this scope assert(strlen(s) == 36U); ^~~~~~ sys-uuid.cc:96:12: note: 'strlen' is defined in header '<cstring>'; did you forget to '#include <cstring>'? PR: 238780 Approved by: samy.mahmoudi@gmail.com (maintainer timeout), tcberner (mentor) Differential Revision: https://reviews.freebsd.org/D22025 Changes: head/converters/pdf2djvu/files/patch-sys-uuid.cc