batterycenter / embed

A CMake/C++20 library to embed resource files at compile time
Apache License 2.0
116 stars 4 forks source link

When adding using FetchContent LTO/IPO gets disabled #4

Closed gczuczy closed 6 months ago

gczuczy commented 8 months ago

Hello,

I've found a strange thing. I've copy-pasted adding this thing to my root CMakeLists.txt:

FetchContent_Declare(
  battery-embed
  GIT_REPOSITORY https://github.com/batterycenter/embed.git
  GIT_TAG        v1.2.8
)
FetchContent_MakeAvailable(battery-embed)

And then suddenly I'm getting a message then LTO/IPO is not supported:

$ gmake tests_fermd && bin/tests-fermd '[db]' -d yes
-- Thread library to use: -pthread
-- Thread library pthread compatible:
-- Boost found
-- ryml: using C++ standard: C++20
-- ryml: setting C++ standard required: ON
-- ryml: importing subproject c4core (SUBDIRECTORY)... /root/aegir/controllers/_deps/ryml-src/ext/c4core
-- c4core: using C++ standard: C++20
-- ryml: -----> target ryml PUBLIC incorporating lib c4core
-- IPO/LTO not supported: <CMake doesn't support IPO for current C compiler>

When I'm commenting the above FetchContent_* out for your lib, suddenly LTO is back again:

gmake tests_fermd && bin/tests-fermd '[db]' -d yes
-- Thread library to use: -pthread
-- Thread library pthread compatible:
-- Boost found
-- ryml: using C++ standard: C++20
-- ryml: setting C++ standard required: ON
-- ryml: importing subproject c4core (SUBDIRECTORY)... /root/aegir/controllers/_deps/ryml-src/ext/c4core
-- c4core: using C++ standard: C++20
-- ryml: -----> target ryml PUBLIC incorporating lib c4core
-- Enabling IPO/LTO
HerrNamenlos123 commented 6 months ago

Hi @gczuczy sorry I must have missed your issue somehow, notifications were not enabled. I just saw the issue. Are you still having this problem?

gczuczy commented 6 months ago

@HerrNamenlos123 Not really, but that's because I've solved this without using this library. Your lib was the most promising for the job, however the side effects vetod its use for me.

Once this is fixed, I probably should give it another go.

HerrNamenlos123 commented 6 months ago

Unfortunately it's not clear to me where the issue comes from or how i should even start debugging it. Can you give a bit more information on your environment? like OS, CMake version, Compiler and Compiler version, project type, what is your project and in what ways is it different from a standard CMake hello world?

Can you try to create a minimal reproducible example that has the error? Like creating a CMake hello world that works and slowly adding parts of your project until the error occurs? It must be something with non-standard compiler flags of yours.

gczuczy commented 6 months ago

A minimal repro might take some time to work, however I've added it back to a commit to my work in progress branch, guarded b by a conditional: https://github.com/gczuczy/aegir/blob/f1ed5b8644d5971335f6449a4e7b4c7a56010191/controllers/CMakeLists.txt#L47

That can be debugged. I don't think I'm doing anything extraordinary, also I'm not very seasoned in cmake, but I'm trying to follow best practices.

Regarding the environment:

# uname -a
FreeBSD aeferm 14.0-STABLE FreeBSD 14.0-STABLE #0 stable/14-n266021-399961e0a413: Sat Dec 30 16:49:01 UTC 2023     toor@x:/tank/rpi3/obj/usr/src/arm64.aarch64/sys/AEGIR arm64
# c++ -v
FreeBSD clang version 16.0.6 (https://github.com/llvm/llvm-project.git llvmorg-16.0.6-0-g7cbf1a259152)
Target: aarch64-unknown-freebsd14.0
Thread model: posix
InstalledDir: /usr/bin
# cmake --version
cmake version 3.26.1

CMake suite maintained and supported by Kitware (kitware.com/cmake).
gczuczy commented 6 months ago

Actually it was a lot easier than I thought: https://github.com/gczuczy/battery-embed-4

I'm not sure anything worths removing. By removing the CXX standard I also get the same:

set(CMAKE_CXX_STANDARD 20)

However battery-embed needs c++20, so I don't think that has a reason to go.

So, I'm not sure I can reduce it any more. This is a minmal CMakeLists.txt what reproduces it.

gczuczy commented 6 months ago

Checked this example on an amd64, got the same result:

$ cmake --fresh -DFETCH_EMBED=OFF .
-- The CXX compiler identification is Clang 16.0.6
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Enabling IPO/LTO
-- Configuring done (1.5s)
-- Generating done (0.0s)
-- Build files have been written to: /home/gczuczy/battery-embed-4
$ cmake --fresh -DFETCH_EMBED=ON .
-- The CXX compiler identification is Clang 16.0.6
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- The C compiler identification is Clang 16.0.6
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- IPO/LTO not supported: <CMake doesn't support IPO for current C compiler>
-- Configuring done (2.3s)
-- Generating done (0.0s)
-- Build files have been written to: /home/gczuczy/battery-embed-4
$ uname -a; c++ -v ;cmake --version
FreeBSD dirk.jeles.harmless.lan 14.0-STABLE FreeBSD 14.0-STABLE #0 stable/14-n266021-399961e0a413: Tue Dec 19 23:30:32 UTC 2023     toor@x:/usr/obj/usr/src/amd64.amd64/sys/DIRK amd64
FreeBSD clang version 16.0.6 (https://github.com/llvm/llvm-project.git llvmorg-16.0.6-0-g7cbf1a259152)
Target: x86_64-unknown-freebsd14.0
Thread model: posix
InstalledDir: /usr/bin
cmake version 3.27.9

CMake suite maintained and supported by Kitware (kitware.com/cmake).
HerrNamenlos123 commented 6 months ago

Interesting. I can't test myself as I don't have a FreeBsd machine currently.

Seems like you are not the first, but it's not really solved: https://discourse.cmake.org/t/checkiposupported-inside-subdirectory-says-that-its-not-supported/1581

Can you test if it changes something if you reorder the calls? like moving the embed fetchcontent to the top just below project call, or to the very bottom?

Also, since it's not a lot of work as nothing needs to actually compile, can you check if boost is the issue or the checkipo module itself, by commenting it out? Do you need IPO?

You could comment out more and more until only embed is left

HerrNamenlos123 commented 6 months ago

Oh sorry i did not see your repro, gimme a sec

HerrNamenlos123 commented 6 months ago

Hey, as it says c compiler, can you try adding C as a language in the project call?

HerrNamenlos123 commented 6 months ago

I think i see a hint: When disabling embed, only a C++ compiler is found by CMake, but when embed is enabled, it also finds a c compiler and complains that the c compiler does not support IPO.

Maybe embed by default loads both C++ and C compilers. Can you please go into the build directory for cmake, in _deps or something like that, where battery embed was cloned by fetchcontent, and change the project() call to explicitly use CXX but not C? If that solves the issue I might implement that by default.

gczuczy commented 6 months ago

Apparently it did:

$ cmake --fresh -DFETCH_EMBED=ON .
-- The CXX compiler identification is Clang 16.0.6
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Enabling IPO/LTO
-- Configuring done (2.3s)
-- Generating done (0.0s)
-- Build files have been written to: /home/gczuczy/battery-embed-4
$ grep project _deps/battery-embed-src/CMakeLists.txt
project(embed LANGUAGES CXX)
    add_subdirectory(examples)      # Set Example 'simple' as the default project in Visual Studio

Yup, seems like this does the trick.

HerrNamenlos123 commented 6 months ago

Perfect! For now you can simply use it like that and manually change it in the build directory and when i have time to get to it, i will test if it doesn't cause any other issues, and implement CXX by default

HerrNamenlos123 commented 6 months ago

Oh and btw i would be damn honored to hear that my library is running on a beer brewery - i am excited! :)

Until i permanently fix the issue, ...... happy embedding i guess! :)

gczuczy commented 6 months ago

Oh, it's just a home brewery, that's what you get when an IT focused on automation picks up brewing as a hobby :)

Thank you very much for fixing this. What I've tried I've found your library to be the "C++-est" that came across for the job. I'm also using this little petproject for honing my C++ knowledge, it's quite fun.

HerrNamenlos123 commented 6 months ago

I released a new version that features CXX. You can simply update the tag version in FetchContent to the latest tag

gczuczy commented 6 months ago

Thank you, checked it, and it's indeed working as intended