search

contour-terminal / contour

Modern C++ Terminal Emulator
http://contour-terminal.org/
Apache License 2.0
2.44k stars 107 forks source link

Linker failures #548

Closed herrhotzenplotz closed 2 years ago

herrhotzenplotz commented 2 years ago

First of all, thank you for the big build system refactors.

However, it doesn't quite do the job yet:

Another strange one:

src/terminal/Line.h:212 gives a compiler error like:

In file included from /usr/home/nico/src/contour/src/terminal/MatchModes.cpp:2:
In file included from /usr/home/nico/src/contour/src/terminal/Terminal.h:17:
In file included from /usr/home/nico/src/contour/src/terminal/RenderBuffer.h:18:
In file included from /usr/home/nico/src/contour/src/terminal/Grid.h:17:
/usr/home/nico/src/contour/src/terminal/Line.h:215:34: error: member reference base type 'gsl::span' is not a structure or union
        return gsl::span(buffer_).subspan(unbox<size_t>(_start), unbox<size_t>(_count));
               ~~~~~~~~~~~~~~~~~~^~~~~~~~

When I do not inline the call to the span constructor, it works for some magic reason. No idea what is going on there.

See entire diff at https://gist.githubusercontent.com/herrhotzenplotz/2aca719d5133dcce2e2378043bd3ecc8/raw/f907f13eb2224c3c8d5823f7fef1c5bdc7e397da/diff

After all built, I hit a segfault in the LRU cache: https://gist.githubusercontent.com/herrhotzenplotz/2aca719d5133dcce2e2378043bd3ecc8/raw/f907f13eb2224c3c8d5823f7fef1c5bdc7e397da/foobar.txt

This happens when you decrease the font size (Ctrl+Scrollwheel) and then run the notcurses-demo.

herrhotzenplotz commented 2 years ago

For fun and profit I built again with a different LLVM version to test the compilation error:

nico@triton:~/src/contour $ /usr/bin/c++  --version
FreeBSD clang version 11.0.1 (git@github.com:llvm/llvm-project.git llvmorg-11.0.1-0-g43ff75f2c3fe)
Target: x86_64-unknown-freebsd13.0
Thread model: posix
InstalledDir: /usr/bin
nico@triton:~/src/contour $ /usr/local/llvm12/bin/clang++ --version
clang version 12.0.1
Target: x86_64-portbld-freebsd13.0
Thread model: posix
InstalledDir: /usr/local/llvm12/bin
nico@triton:~/src/contour $

It compiles on version 12, but fails on version 11. Looks like this is a LLVM 11 compiler bug (surprise, eh?).

christianparpart commented 2 years ago

Hey. Thanks for your input and support. I gonna readand respond to you ASAP

christianparpart commented 2 years ago

GSL doesn't have a shared library to link against, thus providing it in target_link_libraries causes linker failures on FreeBSD at least.

I am really questioning my own CMake skills here. I am very certain I do not know everything. I am however a little bit confused also, because it seems I'm using target_link_libraries for everything else, too, and it works. range-v3 is also a header-only lib and works on Linux/Mac/Windows. Hmmm... I googled around and found out that it seems to be right to use target_link_libraries.

yaml-cpp doesn't seem to give us the absolute path to the shared object which causes it to not link properly since when it is installed from ports/binary packages it resides in /usr/local/ and not in /usr/. Solution would be to use find_library to figure out the correct absolute link path.

If you look at the ThirdParties.cpp

https://github.com/contour-terminal/contour/blob/da8ed95d842f9b9eb73c0dd628d4d6ddad50733d/cmake/ThirdParties.cmake#L93-L98

This basically means: If the cmake target yaml-cpp does not exist it is assumed to not be embedded and therefore must be provided as system package. So find_package() will be used. I confess, Most likely I am missing some actions here. It feels like decades ago I last used find_package() actively and I only barely remember how to properly use it. I should google up what variables I should use after that (and I am certainly missing that part) - and how I can make that transparently work versus embedded yaml-cpp uses.

The install-deps.sh script seems to call sudo to install packages on all distros. We need su root -c "pkg install ..." on FreeBSD as an alternative because running the script as root seems like a strange idea.

Fixed (tm).

Said script tries to download GSL using curl/fetch despite it being already installed as a system dependency

So the basic idea behind install-deps.sh is to either fetch the deps as embedded libs, otherwise ensure it's used from the system using find_package. So find_package should ALWAYS be preferred, IMHO, and I tried hard by trial'n'error to find out what Linux distributions / Windows / Mac's have and need. To be honest, for FreeBSD I was shamelessly guessing. :)

Another strange one:

/usr/home/nico/src/contour/src/terminal/Line.h:215:34: error: member reference base type 'gsl::span' is not a structure or union

This is most likely a non-conforming (a.k.a. too old) compiler, can it be? I know these errors, and there is usually a trivial workaround by simply rewriting these lines due to lag of proper auto type deduction or sometimes not getting the right constructor.

I agree, Contour is making extensive use of C++17.

It compiles on version 12, but fails on version 11. Looks like this is a LLVM 11 compiler bug (surprise, eh?).

I was reading this one after having written the other text. So yes, that's what I meant. :-)

It would be nice if I could get Contour build almost out of the box simply by git clone and calling install-deps.sh.

For packaging (on any OS) I'd prefer to always favor existing system packages over embedded ones (won't work for libunicode/termbench-pro that easily, but for the prominent rest :) )

Okay, I hope I did not miss anything. 🙏

herrhotzenplotz commented 2 years ago

I googled around and found out that it seems to be right to use target_link_libr

That link is talking about the GNU Scientific Library GSL.

I should google up what variables I should use after that (and I am certainly missing that part) - and how I can make that transparently work versus embedded yaml-cpp uses.

Just for reference, this is what the yaml-cpp config gives us on FreeBSD:

nico@triton:~ $ cat /usr/local/lib/cmake/yaml\-cpp/yaml\-cpp\-config.cmake 
# - Config file for the yaml-cpp package
# It defines the following variables
#  YAML_CPP_INCLUDE_DIR - include directory
#  YAML_CPP_LIBRARIES    - libraries to link against

set(YAML_CPP_INCLUDE_DIR "/usr/local/include")
set(YAML_CPP_LIBRARIES "yaml-cpp")
set(YAML_CPP_LIB_DIR "-L/usr/local/lib")
nico@triton:~ $
christianparpart commented 2 years ago

Sigh, Oops, Sorry (SOS). I was taking the wrong example. We are using the GSL from github.com/Microsoft/GSL actually. Thanks for the paste, I'll integrate that into the existing PR. :)

christianparpart commented 2 years ago

I have installed FreeBSD in a VM (actually twice, on both laptops^^).

So far I am making progress in fixing but what really bites me is that I start to believe that the yaml-cpp linker error is caused by a provided and buggy .cmake file for yaml-cpp.

If I compare this to other libraries I am linking against that is also located via find_package to find their .cmake files, I believe there's nothing necessarily wrong on my end.

I could however do:

if(FREEBSD)
    target_link_directories(mylib PUBLIC "/usr/local/lib")
endif()

I'm now trying to add a FreeBSD CI job to it (sadly only possible via embedded VM, not natively as runner).

christianparpart commented 2 years ago 
nico@triton:~ $ cat /usr/local/lib/cmake/yaml\-cpp/yaml\-cpp\-config.cmake
# - Config file for the yaml-cpp package # It defines the following variables
# YAML_CPP_INCLUDE_DIR - include directory
# YAML_CPP_LIBRARIES - libraries to link against
set(YAML_CPP_INCLUDE_DIR "/usr/local/include")
set(YAML_CPP_LIBRARIES "yaml-cpp")
set(YAML_CPP_LIB_DIR "-L/usr/local/lib")

Why the heck is my file contents different than yours? I'd expect mine to be at least as up to date as yours, given the fact that I literally just installed today:

 trapni  ~  freebsd-version
13.0-RELEASE
 trapni  ~  cat /usr/local/lib/cmake/yaml\-cpp/yaml\-cpp\-config.cmake

# - Config file for the yaml-cpp package
# It defines the following variables
#  YAML_CPP_INCLUDE_DIR - include directory
#  YAML_CPP_LIBRARIES    - libraries to link against

set(YAML_CPP_INCLUDE_DIR "/usr/local/include")
set(YAML_CPP_LIBRARIES "yaml-cpp")
herrhotzenplotz commented 2 years ago

On Mon, 20 Dec 2021 12:22:39 -0800 Christian Parpart @.***> wrote:

Why the heck is my file contents different than yours? I'd expect mine to be at least as up to date as yours, given the fact that I literally just installed today:


 trapni  ~  freebsd-version
13.0-RELEASE
 trapni  ~  cat
/usr/local/lib/cmake/yaml\-cpp/yaml\-cpp\-config.cmake

You may wanna do a freebsd-update fetch install as you're still on patch level 0. The most recent for FreeBSD 13.0 RELEAESE is p4. Also, in /etc/pkg/FreeBSD.conf I switched from quarterly to latest.

-- Sent from hades / FreeBSD 13.0-RELEASE

Please remember: https://useplaintext.email/#etiquette HTML-formatted mail will end up in the Spam folder. Do NOT send me Microsoft Office documents. I shall refrain from opening them.

Nico Sonack @.***>

herrhotzenplotz commented 2 years ago

On Mon, 20 Dec 2021 12:06:16 -0800 Christian Parpart @.***> wrote:

So far I am making progress in fixing but what really bites me is that I start to believe that the yaml-cpp linker error is caused by a provided and buggy .cmake file for yaml-cpp.

I did have that suspicion too and was digging, without any luck, in the ports bugs.

I could however do:

if(FREEBSD)
    target_link_directories(mylib PUBLIC "/usr/local/lib")
endif()

Tbh, this looks like a hack to me and it may not work on some package builders. However, this is easily patchable. Or, as an alternative have an external CMake flag to override this (this path is commonly known as LOCALBASE in the ports tree).

-- Sent from hades / FreeBSD 13.0-RELEASE

Please remember: https://useplaintext.email/#etiquette HTML-formatted mail will end up in the Spam folder. Do NOT send me Microsoft Office documents. I shall refrain from opening them.

Nico Sonack @.***>

herrhotzenplotz commented 2 years ago

On Mon, 20 Dec 2021 12:22:39 -0800 Christian Parpart @.***> wrote:

***@***.***:~ $ cat
/usr/local/lib/cmake/yaml\-cpp/yaml\-cpp\-config.cmake # - Config
file for the yaml-cpp package # It defines the following variables
# YAML_CPP_INCLUDE_DIR - include directory # YAML_CPP_LIBRARIES -
libraries to link against set(YAML_CPP_INCLUDE_DIR
"/usr/local/include") set(YAML_CPP_LIBRARIES "yaml-cpp")
set(YAML_CPP_LIB_DIR "-L/usr/local/lib")

Why the heck is my file contents different than yours? I'd expect mine to be at least as up to date as yours, given the fact that I literally just installed today:

Sorry, I just noticed, this file is indeed a little weird on that particular machine. It must have gotten overridden by something I have yet to figure out. On a second machine it is the same as you pasted.

-- Sent from hades / FreeBSD 13.0-RELEASE

Please remember: https://useplaintext.email/#etiquette HTML-formatted mail will end up in the Spam folder. Do NOT send me Microsoft Office documents. I shall refrain from opening them.

Nico Sonack @.***>

christianparpart commented 2 years ago

@herrhotzenplotz Hey. I think the latest branch merge should have made it way easier for FreeBSD.

Since I cannot rely on yaml-cpp being packaged properly (wrt. .cmake file), I decided to simply embed the library, just like what we have done initially and what we do as "last-resort" for libraries that cannot be taken from system libraries.

Many thanks for your patience. :)

herrhotzenplotz commented 2 years ago

Sorry to bother, but #549 didn't fix the MS GSL related link failures.

See https://gitlab.com/herrhotzenplotz/ports/-/tree/x11/contour/x11/contour/files for the patches I had to make to build in ports.

christianparpart commented 2 years ago

I wonder why it builds on CI (FreeBSD), hmm...

Looking at your patch(es), it seems like you simply drop any cmake target "GSL", which should work if the include path is present already and it's header-only anyways. Sound smore like a positive side-effect-abuse, hmmm.... I'm thinking about it. :)

herrhotzenplotz commented 2 years ago

Fixed as of #562