Bug 76963 - I can't build poppler with Clang
Summary: I can't build poppler with Clang
Status: RESOLVED FIXED
Alias: None
Product: poppler
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-02 19:17 UTC by André Wöbbeking
Modified: 2014-12-14 17:53 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Only consider adding -fno-check-new if compiler supports it (1.27 KB, patch)
2014-11-02 20:10 UTC, Daniel Macks
Details | Splinter Review

Description André Wöbbeking 2014-04-02 19:17:21 UTC
Hi,

in PopplerMacros.cmake you use -fno-check-new as compile option which Clang doesn't know. Do you really need that option so that you need separate flags for Clang (if you want to support Clang) or could it be removed?


Cheers,
André
Comment 1 Mario Grgic 2014-06-02 14:50:58 UTC
To temporarily fix the problem, edit the Makefiles in place:

find . -name "Makefile" | xargs -L 1 perl -pi -e 's/-fno-check-new//g'


to remove the unrecognized option by clang, and build again.
Comment 2 chantepie 2014-07-16 10:03:21 UTC
It can be done in configure script, before calling it so option isn't put in Makefiles. A final fix would be to add condition in configure.ac not to have it on OS X.
Comment 3 Albert Astals Cid 2014-07-21 22:30:23 UTC
i'd prefer not changing our gcc options just because clang doesn't support them. You're welcome to supply patches to improve clang building in both configure and cmake (i.e. check if the flag is supported before adding it, i think cmake at least has something similar).

For me it's not a top priority
Comment 4 Daniel Macks 2014-07-24 22:19:23 UTC
The boilerplate for this sort of autoconf check is something like:


saved_CXXFLAGS="$CXXFLAGS"
CXXFLAGS="-fno-check-new $CXXFLAGS"
AC_MSG_CHECKING([for -fno-check-new compiler flag])
AC_TRY_COMPILE([], [int main (void) { return 0; }],
  AC_MSG_RESULT([yes]),
  AC_MSG_RESULT([no])
  CXXFLAGS="$saved_CXXFLAGS"
])


Obviously could extract this result into a temp variable first and concat into the larger CXXFLAGS assignments in the relevant case/esac sections rather than having the test itself leave the result in the live variable.
Comment 5 Daniel Macks 2014-07-24 22:21:05 UTC
(In reply to comment #4)

...which is why one should test things first. Either that AC_TRY_COMPILE needs to be fixed to use C++ or the flag being tested needs to go via CFLAGS for purposes of the test itself that otherwise is running as plain C.
Comment 6 Daniel Macks 2014-11-02 05:39:33 UTC
The following seems to work:

AC_MSG_CHECKING([for -fno-check-new compiler flag])
AC_LANG_PUSH([C++])
saved_CXXFLAGS=$CXXFLAGS
CXXFLAGS="-fno-check-new $CXXFLAGS"
AC_TRY_COMPILE([], [],
  AC_MSG_RESULT([yes]),
  AC_MSG_RESULT([no])
)
CXXFLAGS=$saved_CXXFLAGS
AC_LANG_POP

I'll be happy to provide a patch against current git, but there are two approaches. Reason I ask is that the flag is only *sometimes* added, via various branches of a case statement beginning at line 814.

1. Do this test (always) and store the result in a temp variable, and then use that result to control whether the flag is propagated in the branches where it was previously hardcoded to add.
2. Do this test and propagate the flag only in the branches where it is currently hardcoded to use.

The tradeoff is that option 1 avoids duplicating the test code itself (less fragile if the case logic changes) but means the test is run even in situations where its results would be ignored. Please let me know which approach you would prefer (or some other approach altogether).
Comment 7 Albert Astals Cid 2014-11-02 14:51:40 UTC
Less code is always better, don't care if the autoconf step is a bit slower, is not something that is time critical.
Comment 8 Daniel Macks 2014-11-02 20:10:45 UTC
Created attachment 108810 [details] [review]
Only consider adding -fno-check-new if compiler supports it

On my OS X 10.8 with clang, this correctly recognizes that it is not supported and does not propagate it (even when propagating the other flags). If I hack this test to use a different flag that is supported, it correctly recognizes that and propagates it. But I don't have an actual machine that does support this flag to do a direct test that the patch behaves there.
Comment 9 Albert Astals Cid 2014-11-13 11:38:54 UTC
Do we really need this? I've tested clang 3.5 and clang 3.6 and both seem to support the flag, is people using older clangs?
Comment 10 Ting-Wei Lan 2014-11-13 11:55:04 UTC
> Do we really need this? I've tested clang 3.5 and clang 3.6 and both seem to
> support the flag, is people using older clangs?

Yes, FreeBSD 10.1 uses clang 3.4.1 as the system compiler.
Comment 11 Albert Astals Cid 2014-12-14 17:53:18 UTC
Pushed.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.