Bug 238780 - converters/pdf2djvu: fix build with GCC-based architectures
Summary: converters/pdf2djvu: fix build with GCC-based architectures
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-ports-bugs mailing list
URL:
Keywords:
Depends on:
Blocks: 239366
  Show dependency treegraph
 
Reported: 2019-06-23 19:26 UTC by Piotr Kubaj
Modified: 2019-10-16 15:36 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (samy.mahmoudi)


Attachments
patch (736 bytes, patch)
2019-06-23 19:26 UTC, Piotr Kubaj
pkubaj: maintainer-approval? (samy.mahmoudi)
Details | Diff
Patch file generated with svn diff (1.32 KB, patch)
2019-07-21 06:18 UTC, Samy Mahmoudi
no flags Details | Diff
Poudriere log - Clang (58.55 KB, text/plain)
2019-07-21 06:20 UTC, Samy Mahmoudi
no flags Details
Poudriere log - GCC (62.03 KB, text/plain)
2019-07-21 06:22 UTC, Samy Mahmoudi
no flags Details
Patch file generated with svn diff (1.45 KB, patch)
2019-08-09 21:04 UTC, Samy Mahmoudi
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Piotr Kubaj freebsd_committer 2019-06-23 19:26:10 UTC
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>'?
Comment 1 Samy Mahmoudi 2019-07-01 07:37:38 UTC
(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.
Comment 2 Piotr Kubaj freebsd_committer 2019-07-19 15:07:36 UTC
(In reply to Samy Mahmoudi from comment #1)
Anything about this issue?
Comment 3 Samy Mahmoudi 2019-07-20 20:28:57 UTC
(In reply to Piotr Kubaj from comment #2)
Yes, I just wrote a second patch. poudriere is now building for a port test.
Comment 4 Samy Mahmoudi 2019-07-21 06:18:47 UTC
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
Comment 5 Samy Mahmoudi 2019-07-21 06:20:51 UTC
Created attachment 205962 [details]
Poudriere log - Clang
Comment 6 Samy Mahmoudi 2019-07-21 06:22:35 UTC
Created attachment 205963 [details]
Poudriere log - GCC
Comment 7 Samy Mahmoudi 2019-07-21 06:25:35 UTC
N.B. With USES=compiler:gcc-c++11-lib, patching sys-uuid.cc was not necessary.
Comment 8 Piotr Kubaj freebsd_committer 2019-08-06 19:51:20 UTC
Your patch is wrong because users will need to enable GCC_BUILD option manually. The package won't be built on a package builder.
Comment 9 Samy Mahmoudi 2019-08-06 20:38:49 UTC
(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.
Comment 10 Piotr Kubaj freebsd_committer 2019-08-06 20:47:25 UTC
(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.
Comment 11 Samy Mahmoudi 2019-08-09 20:15:21 UTC
(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 ?
Comment 12 Samy Mahmoudi 2019-08-09 20:43:05 UTC
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.
Comment 13 Samy Mahmoudi 2019-08-09 20:54:21 UTC
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 ?
Comment 14 Samy Mahmoudi 2019-08-09 21:04:11 UTC
Created attachment 206409 [details]
Patch file generated with svn diff

- Use GCC on GCC-architectures
- Correct DOCS into DOCS_DESC for a better description
Comment 15 Samy Mahmoudi 2019-08-09 21:30:38 UTC
(In reply to Samy Mahmoudi from comment #14)
Auxiliary problem still holds (comment 12 and comment 13).
Comment 16 Piotr Kubaj freebsd_committer 2019-08-24 13:01:07 UTC
(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?
Comment 17 Samy Mahmoudi 2019-08-24 15:58:30 UTC
(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 !
Comment 18 Piotr Kubaj freebsd_committer 2019-08-24 16:07:44 UTC
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.
Comment 19 Samy Mahmoudi 2019-08-31 15:37:57 UTC
(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.
Comment 20 Samy Mahmoudi 2019-08-31 15:40:33 UTC
(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.
Comment 21 Piotr Kubaj freebsd_committer 2019-09-11 12:44:40 UTC
(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.
Comment 22 commit-hook freebsd_committer 2019-10-16 15:36:23 UTC
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