libE57Format icon indicating copy to clipboard operation
libE57Format copied to clipboard

Make the default value of E57_RELEASE_LTO dependent on E57_BUILD_SHARED

Open SunBlack opened this issue 1 year ago • 10 comments

By default BUILD_SHARED_LIBS is not defined, therefore the default of E57_BUILD_SHARED is also OFF. Therefore this lib will be build as static library by default. This leads by default to linker issues later:

/usr/bin/ld: /.../libE57Format.a(BlobNode.cpp.o): plugin needed to handle lto object

As the comment about E57_RELEASE_LTO says, this option should be OFF when building a static library. Therefore the default value is now adjusted accordingly. As changing E57_BUILD_SHARED later will not change the default value of E57_RELEASE_LTO I have added a warning so that there is a hint that the combination may cause issues.

GDAL has IPO disabled by default: https://github.com/OSGeo/gdal/blob/305b23f5a90bf6d86c7fb895dfc745321def0aab/gdal.cmake#L27

Qt seems to also not set this value, but in case CMAKE_INTERPROCEDURAL_OPTIMIZATION is set they enforce it to OFF for MSVC 3rd party: https://github.com/qt/qtbase/commit/0d708a9aad8fa7f44e94f5bc9093a6c93d6140b.

So basically I would argue that E57_RELEASE_LTO should be always OFF or removed and in case someone needs this, he can build it by passing CMAKE_INTERPROCEDURAL_OPTIMIZATION (in case canon has an issue with it, they can add it to their build script as it seems it was not necessary for vcpkg when checking the port scripts from before libE57Format 3.1.0 release)

SunBlack avatar Nov 29 '24 13:11 SunBlack

As the comment about E57_RELEASE_LTO says, this option should be OFF when building a static library.

It's actually talking specifically about release static builds for distribution.

which is a problem if compiling statically for distribution (e.g. in a package manager). Generally you will only want to turn this off for distributing static release builds.

If you're using a static lib in your own code (i.e. including this library as a static lib) it's fine.

asmaloney avatar Nov 29 '24 13:11 asmaloney

If you're using a static lib in your own code (i.e. including this library as a static lib) it's fine.

Sadly not. See the linker issue in the initial post of me.

SunBlack avatar Nov 29 '24 13:11 SunBlack

I use it as a static lib on macOS and Windows (MSYS2) without issues so this sounds like maybe a compiler difference.

What OS and compiler are you using?

asmaloney avatar Nov 29 '24 13:11 asmaloney

I use it as a static lib on macOS and Windows (MSYS2) without issues so this sounds like maybe a compiler difference.

What OS and compiler are you using?

On MSVC everything is fine with 3.2.0. On our CI we are using a docker image based on ubuntu:22.04 (my local VM is also Ubuntu 22.04 and there I have the same issue). Compiler is clang (19) and gcc (11).

SunBlack avatar Nov 29 '24 13:11 SunBlack

I've never seen "plugin needed to handle lto object" - sounds like something isn't installed? Are you getting the same message from both clang and gcc on Ubuntu?

Any more info about it when you search for it?

asmaloney avatar Nov 29 '24 13:11 asmaloney

I've never seen "plugin needed to handle lto object" - sounds like something isn't installed? Are you getting the same message from both clang and gcc on Ubuntu?

Tried again. On GCC I'm getting (build libE57 with GCC 11.3, but our project than with GCC 12.0):

lto1: fatal error: bytecode stream in file ‘/.../libE57Format/lib/libE57Format.a’ generated with LTO version 11.3 instead of the expected 12.0

So basically LTO seems to break the use case, to build a dependency with the default OS compiler and then compile it with a different compiler. Since we test our project on our CI with different GCC versions (standard OS compiler when we build a release so that we don't have to install another standard lib at the customer's site) and the latest compiler for more compiler warnings, you have to build multiple versions of libE57 (if you have LTO enabled) so that you can build with multiple compilers at the same time.

In principle, I have the feeling that you really don't want LTO as default, unless you build a release where you want to have a better optimization again - and in that case you can also activate it manually.

Any more info about it when you search for it?

Here for example

SunBlack avatar Nov 29 '24 14:11 SunBlack

OK thanks. I'll consider it.

In the meantime, you should be able to set E57_RELEASE_LTO to OFF.

asmaloney avatar Nov 29 '24 15:11 asmaloney

@SunBlack Your setup is just combining the worst of all worlds.

When distributing the library (even for internal use), you should use a shared library. This provides the benefit of better optimization of calls inside the library (i.e. one libE57format function calling another), and better diagnostics (as the compilier can "see" both caller and callee, it can analyze the code in more depth).

For even more optimization and better diagnostics, you should include libE57format as e.g. a submodule, and compile everything with the same compiler. Then you won't have any problems with different versions. Or you build it separately multiple times, but use the same compiler+linker for all steps.

Your use case is IMHO an outlier. The two typical cases are either static linking, building everything combined; or building a shared library. Only if you want to distribute the static library for reuse by others (where you have no control or knowledge about the used toolchain) a static library with regular object files is useful.

StefanBruens avatar May 16 '25 14:05 StefanBruens

I also hit this problem when building my app with the nixpkgs package distribution.

When upgrading from NixOS 24.05 to 25.05, which has the upgrade from 2.2.0 to 3.1.1 via my commit, E57_RELEASE_LTO got enabled.

This makes that in my downstream app built with nix that discovers libE57Format via Meson and thus passes /nix/store/bl0qh9rx56w6vrha03d4ch2i7qgbi2v0-libe57format-3.2.0/lib/libE57Format.a to g++ for linking, I get linker error:

/nix/store/...-binutils-2.44/bin/ld: /root/benaco-nixos-25.05/serkezet/cpp/build-incremental/../src/E57.cpp:91:(.text+0x155):
  undefined reference to `e57::ImageFile::root() const'

When Linux distributions like NixOS build the .a file, should they then pass -DE57_RELEASE_LTO=OFF to CMake?

nh2 avatar May 25 '25 12:05 nh2

Perhaps tangentially related:

In nixpkgs we build both .a and .so using this hack:

https://github.com/NixOS/nixpkgs/blob/55d1f923c480dadce40f5231feb472e81b0bab48/pkgs/by-name/li/libe57format/package.nix#L69-L84

  # The build system by default builds ONLY static libraries, and with
  # `-DE57_BUILD_SHARED=ON` builds ONLY shared libraries, see:
  #     https://github.com/asmaloney/libE57Format/issues/48
  #     https://github.com/asmaloney/libE57Format/blob/f657d470da5f0d185fe371c4c011683f6e30f0cb/CMakeLists.txt#L82-L89
  # We support building both by building statically and then
  # building an .so file here manually.
  # The way this is written makes this Linux-only for now.
  postInstall = ''
    cd $out/lib
    g++ -Wl,--no-undefined -shared -o libE57FormatShared.so -L. -Wl,-whole-archive -lE57Format -Wl,-no-whole-archive -lxerces-c
    mv libE57FormatShared.so libE57Format.so

    if [ "$dontDisableStatic" -ne "1" ]; then
      rm libE57Format.a
    fi
  '';

nh2 avatar May 25 '25 12:05 nh2