Opened 6 years ago

Closed 6 years ago

#57875 closed defect (invalid)

port:poppler shouldn't require C++14?!

Reported by: RJVB (René Bertin) Owned by: dbevans (David B. Evans)
Priority: Normal Milestone:
Component: ports Version:
Keywords: Cc: mojca (Mojca Miklavec)
Port: poppler

Description

#782c7672 chanced port:poppler to build in C++14 mode, which apparently requires blacklisting quite a few clang versions. I don't even know what LLVM version "clang 800.0.38" corresponds to so I forced the port back to C++11 mode because according to cpppreference.com, thread_local was introduced in C++11. I built using macports-clang-5.0 and that went just fine (poppler 0.73). Works fine too (tested via poppler-qt5)

On an unrelated note: if gobject introspection really introduces a build conflict with its own installed self, why not make that feature optional? Do we even know which ports depend on the bindings that creates (probably only Gnome ports)?

Change History (14)

comment:1 Changed 6 years ago by mf2k (Frank Schima)

Cc: dbevans removed
Owner: set to dbevans
Status: newassigned

comment:2 Changed 6 years ago by sharris40 (Spencer Harris)

It's not required for thread_local; it's required for make_unique. It may work fine for you, but make_unique is not part of C++11, and it will not work on every C++11 compiler.

comment:4 Changed 6 years ago by RJVB (René Bertin)

Indeed, the changelog suggests as much. Curious that clang 5 accepts it when set to use C++11 then but whatever, maybe a snippet like below can be useful (adapted where necessary; I might still have clang 4 installed myself for instance to see if that version works):

foreach clv {3.3 3.4 3.5 3.6 3.7 3.8 3.9 4.0} {
    compiler.blacklist macports-clang-${clv}
}
foreach clv {5.0 6.0 7.0} {
    if {[file exists ${prefix}/bin/clang-mp-${clv}]} {
        compiler.whitelist-prepend macports-clang-${clv}
    }
    compiler.fallback-prepend macports-clang-${clv}
}

Without that my build attempt on 10.9.5 where I didn't specify a compiler picked the Xcode 6.2 clang version (AppleClang 6.x) which rather evidently failed (on thread_local) so the current blacklist formula isn't enough. The snippet above has the effect that the newest MacPorts clang version that the user has installed (from 5, 6 or 7) will be selected for building (seems like the obvious thing to do but yet "base" doesn't).

FWIW, poppler is a port that is used by a number of valuable utilities, so we should take reasonable measures to keep it building on platforms where C++14 support isn't so good. CPPPreference provides a possible std::make_unique implementation that works under C++11 which maybe we could inject if necessary.

comment:5 Changed 6 years ago by sharris40 (Spencer Harris)

Doing that worries me a little. Upstream didn't seem to point out the reason for C++14 source in the documentation, unless I missed something. I had to search around to figure out why they were requiring it. If they aren't communicating those changes very well, the port may need to start playing keep-up. It might be better to pin Poppler's version for older platforms if further breakage happens.

It's also worth pointing out that 0.73.0 renamed one of the options the portfile sets to the much more blatant "ENABLE_UNSTABLE_API_ABI_HEADERS", which sounds to me like they're planning on more major breakages (such as the one that broke texlive-bin until they switched back to the bundled version).

comment:6 Changed 6 years ago by mojca (Mojca Miklavec)

Oh, every single poppler release breaks texlive in a different way (that is: you need a different patch for every single version of poppler), and it's not like the trend is about to stop, don't worry about that :)

And now that poppler requires C++14, upstream TeX Live stopped updating poppler (to be able to compile TL on old machines) and as a consequence also stopped patching their own sources for pdftex/xetex to be compatible with newer versions of poppler. We only hope that someone will step in and port pdftex and xetex to pplib, like it was done for LuaTeX. If make_unique was in fact the only reason for requiring C++14, maybe one could actually patch this in TeX Live, but it would be just another time bomb until the next poppler version switches to C++17 or random other C++14 features anyway.

comment:7 Changed 6 years ago by RJVB (René Bertin)

Sounds like they should release a poppler2 or poppler-ng ... or we could do something like that ourselves (where the newer versions either get a suffix if the build system allows, or are installed to $prefix/libexec).

Upstream didn't seem to point out the reason for C++14 source in the documentation

Well, the changelog does, though not why they decided to use std::make_unique . A version diff should show if that change simplified the code.

BTW, I now understand why my build with -std=c++11 succeeded, it must have been overridden by CMake:

set (CMAKE_CXX_STANDARD 14)
set (CMAKE_CXX_EXTENSIONS OFF)

Anyway, I was happily using 0.68.0 until yesterday (= the least release without C++14), and only upgraded because that version crashed on a particular PDF (though only on Linux...). IOW, could be a good branch point for something like poppler-ng, or a pegged "poppler-og" port. Not really certain what's wisdom here but I sure know that I hate having to rebuild dependents just because some utility library broke ABI - again.

comment:8 Changed 6 years ago by mojca (Mojca Miklavec)

though not why they decided to use std::make_unique

What's to be explained here? I most definitely want that function too when I use unique pointers in C++.

I sure know that I hate having to rebuild dependents just because some utility library broke ABI - again.

As explained above, poppler is regularly breaking API, not just ABI. ABI changes are frequent enough everywhere, for plenty of other ports.

comment:9 Changed 6 years ago by RJVB (René Bertin)

But breaking API without breaking ABI is much less annoying (if you're not using the API in your own code).

ABI changes are too frequent everywhere

T,FTFY O:-)

comment:10 Changed 6 years ago by kencu (Ken)

we already pinned poppler once, at the last qt4 supporting version, fyi. see poppler-qt4.

Maybe we might just stop updating poppler so often? These monthly earthquakes are driving everyone crazy. The KGB or CIA can't be releasing malicious PDFs so frequently that we have to stay ahead of them... We haven't updated icu in years, just to avoid this aggro...

Last edited 6 years ago by kencu (Ken) (previous) (diff)

comment:11 Changed 6 years ago by RJVB (René Bertin)

The less closely you follow updates, the more work (and the bigger the earthquake) each update can become.

From that viewpoint it'd be better to allow concurrent installation of a "legacy port" (now 0.68 but could still be updated from time to time) and a "hot" port. The port:qt5 family already does something similar, for other reasons. Sadly "base" isn't really designed to make it easy to implement things in such a way that users can decide they're fine with building every poppler dependent against the "hot" poppler port. OTOH, we'd do this for a large part to avoid breakage in dependents, and in that light it's best simply to let the poppler port/version decision to the dependent ports and their maintainers.

Back to the required clang version: according to https://clang.llvm.org/cxx_status.html, C++14 is already fully supported in clang 3.4! Very strange then that my AppleClang (which should be a 3.5 version) doesn't even accept thread_local but well, it's from Apple ... they know better what's right for us don't they now ;)

comment:12 Changed 6 years ago by kencu (Ken)

so poppler-qt4-mac is a pegged poppler at

$ port info poppler-qt4-mac
poppler-qt4-mac @0.61.1 (graphics)

for anyone who wants that one. I don't relish 10 of them.

How about better a poppler-devel port that can be updated as often as we want, but a mainline poppler port that is updated no more than twice a year. That would be a good compromise.

Otherwise I fear we are just going to dump poppler like texlive-bin is doing as being too much of a PITA to bother with.

comment:13 Changed 6 years ago by RJVB (René Bertin)

But poppler-qt4-mac exposes only the Qt4 interface and doesn't provide the Qt5 interface.

I thought about a poppler-devel port, but that approach means you can either have a stable environment in which you cannot install ports that require a newer poppler, or an instable environment as we have now.

Installing a port info libexec is trivial and there isn't even need for explicit rpath specification on Mac.

This is 1 extra port, not 10. Oh, and removing poppler isn't really an option. Too many dependents.

FWIW, figuring out a workable framework to accomplish this sort of thing is bound to pay off because going forward we'll probably run into this kind of situation more and more often.

comment:14 Changed 6 years ago by dbevans (David B. Evans)

Resolution: invalid
Status: assignedclosed

Addressing @RJVBs concerns with C++14, thread_local storage, and blacklisting of Apple clang versions:

As I think you know, poppler started requiring a C++14 compiler in https://gitlab.freedesktop.org/poppler/poppler/commit/9bfc10eecb57354270806aa1d9278eebb1db2287. This also introduced the use of std::make_unique to replace std::unique_ptr as discussed above.

C++14 requires Apple clang 600.0.54 or later. See https://trac.macports.org/wiki/CompilerSelection.

As a separate issue, poppler introduced the use of thread_local in https://gitlab.freedesktop.org/poppler/poppler/commit/c46717a70341ac0120579c867d49c250bed4ed52.

Although vanilla clang supported thread_local earlier, for their own policy reasons, it is my understanding that Apple did not support thread_local in clang until much later (800.0.38 Xcode 8).

The current configuration allows poppler to build on all platforms from at least 10.6+ per the buildbots.

See https://stackoverflow.com/questions/28094794/why-does-apple-clang-disallow-c11-thread-local-when-official-clang-supports for a statement of Apple's position in 2014.

My position is to support poppler as provided upstream, rather than to second guess them. If you disagree, you should bring it up with them.

Note: See TracTickets for help on using tickets.