Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add notcurses v3 #4059

Merged
merged 8 commits into from
Dec 15, 2021
Merged

Add notcurses v3 #4059

merged 8 commits into from
Dec 15, 2021

Conversation

KristofferC
Copy link
Member

No description provided.

Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ararslan any idea about FreeBSD?

[20:15:06] CMake Error at CMakeLists.txt:163 (message):
[20:15:06]   Couldn't find unigbrk.h from GNU libunistring

@giordano
Copy link
Member

giordano commented Dec 11, 2021

On Windows:

[20:15:07] -- The following REQUIRED packages have been found:
[20:15:07] 
[20:15:07]  * PkgConfig
[20:15:07]  * terminfo
[20:15:07]  * Threads
[20:15:07]  * libunistring
[20:15:07]  * libdeflate
[20:15:07] 
[20:15:07] -- Configuring done
[20:15:07] CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
[20:15:07] Please set them or make sure they are set and tested correctly in the CMake files:
[20:15:07] libdeflate

So libdeflate is both found and not found? Thanks CMake

@KristofferC
Copy link
Member Author

KristofferC commented Dec 12, 2021

https://dev.azure.com/JuliaPackaging/Yggdrasil/_build/results?buildId=15194&view=logs&jobId=03816dbd-db8d-541a-9811-ffc3f605c6df&j=03816dbd-db8d-541a-9811-ffc3f605c6df&t=7c39363c-b907-5cfc-f492-92c9dedf1126

[13:38:07] --   Package 'libavcodec', required by 'virtual:world', not found
[13:38:07] CMake Error at /usr/share/cmake/Modules/FindPkgConfig.cmake:467 (message):
[13:38:07]   A required package was not found
[13:38:07] Call Stack (most recent call first):
[13:38:07]   /usr/share/cmake/Modules/FindPkgConfig.cmake:647 (_pkg_check_modules_internal)
[13:38:07]   CMakeLists.txt:103 (pkg_check_modules)

Any idea why this wouldn't be found on arm? FFMPEG should provide it, right?

@ararslan
Copy link
Member

@ararslan any idea about FreeBSD?

Unfortunately no... I confirmed that the libunistring artifact for FreeBSD includes the header this is complaining about, so my best guess would be that something is missing an -I flag (or equivalent). FreeBSD has notcurses in the Ports Collection but the Makefile for it doesn't suggest it's using any funny business to build on FreeBSD.

@ararslan
Copy link
Member

Ah, the notcurses FAQ has a relevant tidbit for a similar issue:

Try cmake -DCMAKE_REQUIRED_INCLUDES=/usr/local/include. This is passed by bsd.port.mk.

@KristofferC
Copy link
Member Author

KristofferC commented Dec 13, 2021

I tried adding that but it doesn't seem like it worked :/

Anyone know what this up with

Undefined symbols for architecture arm64:
  "___divdc3", referenced from:
      _normal_demo in normal.c.o
ld: symbol(s) not found for architecture arm64

on apple aarch 64? I see another reference to it in #2938 (comment) but I don't know what fixed it upstream (or if it even got fixed or the offending part just didn't build anymore). The symbol is in there..

sandbox:${WORKSPACE}/srcdir/notcurses-3.0.0/build/CMakeFiles/notcurses-demo.dir/src/demo # nm normal.c.o | grep __div
                 U ___divdc3

@giordano
Copy link
Member

Unfortunately no... I confirmed that the libunistring artifact for FreeBSD includes the header this is complaining about, so my best guess would be that something is missing an -I flag (or equivalent).

Ugh, I think I had looked in the wrong place! Thanks for looking into this. Then I think this is #3949, and yes, the solution should be to add

export CFLAGS="-I${includedir}"

before running cmake

I tried adding that but it doesn't seem like it worked :/

Uhm, what did you try exactly?

I see another reference to it in #2938 (comment) but I don't know what fixed it upstream (or if it even got fixed or the offending part just didn't build anymore)

My understanding was that was an upstream bug that got fixed, but I didn't really follow closely

@KristofferC
Copy link
Member Author

Uhm, what did you try exactly?

Adding: -DCMAKE_REQUIRED_INCLUDES=/usr/local/include. But the CFLAGS seems to work, indeed.

@KristofferC
Copy link
Member Author

KristofferC commented Dec 13, 2021

Down to Heissen CMake on Windows now.

Edit: I just disabled libdeflate on Windows for now.

@KristofferC
Copy link
Member Author

So the next one on Windows is stuff like:

[13:07:17] In file included from /workspace/srcdir/notcurses-3.0.0/src/lib/internal.h:10:0,
[13:07:17]                  from /workspace/srcdir/notcurses-3.0.0/src/lib/fill.c:1:
[13:07:17] /workspace/srcdir/notcurses-3.0.0/src/compat/compat.h: In function ‘clock_getns’:
[13:07:17] /workspace/srcdir/notcurses-3.0.0/src/compat/compat.h:109:6: warning: implicit declaration of function ‘clock_gettime’; did you mean ‘clock_getns’? [-Wimplicit-function-declaration]
[13:07:17]    if(clock_gettime(clockid, &tspec) < 0){
[13:07:17]       ^~~~~~~~~~~~~
[13:07:17]       clock_getns

[13:07:19] /workspace/srcdir/notcurses-3.0.0/src/lib/render.c: In function ‘ncpile_render’:
[13:07:19] /workspace/srcdir/notcurses-3.0.0/src/lib/render.c:1588:17: error: ‘CLOCK_MONOTONIC’ undeclared (first use in this function); did you mean ‘LOCK_ONLYONCE’?
[13:07:19]    clock_gettime(CLOCK_MONOTONIC, &start);

clock_gettime is apparently posix and not standard C. I've seen suggestions adding stuff like #define _POSIX_C_SOURCE 199309L but that doesn't seem to work (maybe because these functions are not available at all)...

@giordano
Copy link
Member

giordano commented Dec 13, 2021

Regarding libdeflate on Windows, running CMake with --debug-find shows:

 find_library considered the following locations:

    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/opt/bin/x86_64-w64-mingw32-libgfortran4-cxx11/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/opt/x86_64-w64-mingw32/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/opt/bin/x86_64-linux-musl-cxx11/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/opt/x86_64-linux-musl/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/usr/local/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/usr/local/sbin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/usr/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/usr/sbin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/sbin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/workspace/x86_64-linux-musl-cxx11/destdir/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/workspace/destdir/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/usr/lib/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/usr/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/workspace/destdir/lib/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/workspace/destdir/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/lib/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/workspace/srcdir/notcurses-3.0.0/REQUIRED/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/bin/x86_64-w64-mingw32-libgfortran4-cxx11/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-w64-mingw32/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/bin/x86_64-linux-musl-cxx11/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /opt/x86_64-linux-musl/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /usr/local/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /usr/local/sbin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /usr/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /usr/sbin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /sbin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /workspace/x86_64-linux-musl-cxx11/destdir/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /workspace/destdir/bin/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /usr/lib/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /usr/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /workspace/destdir/lib/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /workspace/destdir/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /lib/(lib|)deflate(\.dll\.a|\.a|\.lib)
    /workspace/srcdir/notcurses-3.0.0/REQUIRED/(lib|)deflate(\.dll\.a|\.a|\.lib)

  The item was not found.

So this is looking for the import library only, which can't be found because apparently libdeflate doesn't generate/install it. Adding

set(CMAKE_FIND_LIBRARY_SUFFIXES ".$ENV{dlext}" ${CMAKE_FIND_LIBRARY_SUFFIXES})

to CMakeLists.txt solves the issue for me. Patch inspired by

+# Ensure on Windows we look for shared libraries `*.dll`, instead of just import
+# libraries (we don't have those for suitesparseconfig).
+set(CMAKE_FIND_LIBRARY_SUFFIXES ".$ENV{dlext}" ${CMAKE_FIND_LIBRARY_SUFFIXES})

See also #3353 for a similar debugging experience.

@giordano
Copy link
Member

Side note: I tried setting CMAKE_FIND_LIBRARY_SUFFIXES in our toolchain file as we can run into this issue again in the future, but it doesn't seem to work, maybe the toolchain file is evaluated too early for it to have an effect. Thanks again CMake, sigh.

@KristofferC
Copy link
Member Author

Do you think that should be upstreamed or is it too unique to our setup?

@giordano
Copy link
Member

Do you think that should be upstreamed or is it too unique to our setup?

The exact command I showed above is definitely specific to BinaryBuilder because the dlext environment variable is defined only in our environment. Regarding upstreaming the patch, I'd like confirmation from someone outside of our little world that this is indeed needed.

@giordano
Copy link
Member

Regarding the clock error on Windows:

sandbox:${WORKSPACE}/srcdir/notcurses/build # grep -r 'CLOCK_MONOTONIC' /opt/${target}/${target}
/opt/x86_64-w64-mingw32/x86_64-w64-mingw32/include/c++/7.1.0/x86_64-w64-mingw32/bits/c++config.h:/* #undef _GLIBCXX_USE_CLOCK_MONOTONIC */
sandbox:${WORKSPACE}/srcdir/notcurses/build # grep -r 'TIMER_ABSTIME' /opt/${target}/${target}
sandbox:${WORKSPACE}/srcdir/notcurses/build # 

Those macros aren't defined anywhere. Of course you could get past to that with

export CFLAGS="-DCLOCK_MONOTONIC=1 -DTIMER_ABSTIME=1"

but that's an indication that non-standard features are used.

@KristofferC
Copy link
Member Author

but that's an indication that non-standard features are used.

Yes, they are posix and not C. But they are available in winpthreads in mingw:

https://github.com/Alexpux/mingw-w64/blob/d0d7f784833bbb0b2d279310ddc6afb52fe47a46/mingw-w64-libraries/winpthreads/include/pthread_time.h#L61-L63

@giordano
Copy link
Member

Ok, we have

sandbox:${WORKSPACE}/srcdir/notcurses/build # cat /opt/x86_64-w64-mingw32/x86_64-w64-mingw32/sys-root/include/pthread_time.h
/* Dummy header, which gets overridden, if winpthread library gets installed.  */

which looks like https://github.com/Alexpux/mingw-w64/blob/d0d7f784833bbb0b2d279310ddc6afb52fe47a46/mingw-w64-headers/crt/pthread_time.h, of the files you liked above. The funny thing is that we should be building MinGW with winpthreads:

# Build winpthreads
mkdir -p $WORKSPACE/srcdir/mingw_winpthreads_build
cd $WORKSPACE/srcdir/mingw_winpthreads_build
${WORKSPACE}/srcdir/mingw-*/mingw-w64-libraries/winpthreads/configure \
--prefix=/ \
--host=${COMPILER_TARGET} \
--enable-static \
--enable-shared

@staticfloat any clue of what may be going on here? 😬

@dankamongmen
Copy link

upstream author here -- please let me know anything i can do to assist!

@KristofferC
Copy link
Member Author

@giordano, Even if we would have winpthreads available in the mingw compiler shard, we still need to somehow declare a dependency on it, so that it gets shipped to users machines since it needs to be available at runtime, right?

@giordano
Copy link
Member

giordano commented Dec 14, 2021

We have bin/libwinpthread-1.dll in CompilerSupportLibraries_jll, so we might have to depend on that package

@giordano
Copy link
Member

Actually I see bin/libwinpthread-1.dll also in Julia Windows distribution, so we probably don't even need to depend on CompilerSupportLibraries_jll

@KristofferC
Copy link
Member Author

@giordano, Ok, I just overwrote the pthread_time.h with the one we want, then I had to bump the windows build. Now I am getting

/opt/x86_64-w64-mingw32/bin/../lib/gcc/x86_64-w64-mingw32/7.1.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find -lSecur32

Any ideas?

@giordano
Copy link
Member

Not at the computer now, but I seem to remember also libraries are all lowercase.

@KristofferC
Copy link
Member Author

Nice one!

@KristofferC
Copy link
Member Author

KristofferC commented Dec 15, 2021

🎉

Next steps, which don't have to be done here are to enable multimedia support and eventually get the binaries generated. Even if we don't need them it would be nice to figure out what's up with #4059 (comment) on M1.

@giordano
Copy link
Member

Alright, this looks great, apart from the borked MinGW pthreads header files (I opened an issue about that: #4067), thanks a lot also for upstreaming all those patches!

@dankamongmen
Copy link

Alright, this looks great, apart from the borked MinGW pthreads header files (I opened an issue about that: #4067), thanks a lot also for upstreaming all those patches!

this has been fun to watch!

@giordano giordano changed the title WIP: add notcurses v3 Add notcurses v3 Dec 15, 2021
@giordano giordano merged commit 97a6d5c into master Dec 15, 2021
@giordano giordano deleted the kc/netcurses branch December 15, 2021 21:11
@KristofferC
Copy link
Member Author

KristofferC commented Dec 17, 2021

There seems to be some issue with the Ncurses dependency here. If I try to initialize notcurses using the library built here I get:

interrogate_terminfo:1121:Terminfo error -1 for [screen-256color] (see terminfo(3ncurses))

which is a call into terminfo. I'll see if I can figure out what is going on there.

@giordano
Copy link
Member

giordano commented Dec 17, 2021

Maybe need to set TERMINFO and TERMINFO_DIRS? See #455 (comment). We should probably do it in the JLL __init__, now that we can. See also JuliaPackaging/BinaryBuilder.jl#687 (comment).

@KristofferC
Copy link
Member Author

Nice. Any idea why this has to be done in the jll library but not the local one? Does it compile in some information from the build env?

@giordano
Copy link
Member

That'd be my guess, yes. It probably hardcodes some build-time absolute paths that can be overridden at runtime with environment variables.

@KristofferC
Copy link
Member Author

An interesting difference between Windows and Linux/mac:

Windows:

julia> (@ccall libnotcurses_core.notcurses_version()::Ptr{Cchar}) |> unsafe_string
"3.0.1"

julia> (@ccall libnotcurses.notcurses_version()::Ptr{Cchar}) |> unsafe_string
ERROR: could not load symbol "notcurses_version":

Linux:

julia> (@ccall libnotcurses_core.notcurses_version()::Ptr{Cchar}) |> unsafe_string
"3.0.1"

julia> (@ccall libnotcurses.notcurses_version()::Ptr{Cchar}) |> unsafe_string
"3.0.1"

So I think I have to be more careful saying which library each symbol actually comes from?

@dankamongmen
Copy link

  1. i definitely don't compile these paths in at compile time -- they're strictly aspects of libterminfo
  2. i depend on terminfo on all platforms but windows at both compilation and linking time
  3. if i can't get a terminfo handle (based on TERM) at startup, i refuse to start (this will change over coming months)

not sure if that helps

@KristofferC
Copy link
Member Author

KristofferC commented Dec 18, 2021

i definitely don't compile these paths in at compile time -- they're strictly aspects of libterminfo

We were talking about the ncurses dependency here, not notcurses :). I was just surprised I had to set TERMINFO explicitly when using the remotely compiled ncurses but not for the local one.

@giordano
Copy link
Member

giordano commented Dec 18, 2021

An interesting difference between Windows and Linux/mac:

Windows:

julia> (@ccall libnotcurses_core.notcurses_version()::Ptr{Cchar}) |> unsafe_string
"3.0.1"

julia> (@ccall libnotcurses.notcurses_version()::Ptr{Cchar}) |> unsafe_string
ERROR: could not load symbol "notcurses_version":

Linux:

julia> (@ccall libnotcurses_core.notcurses_version()::Ptr{Cchar}) |> unsafe_string
"3.0.1"

julia> (@ccall libnotcurses.notcurses_version()::Ptr{Cchar}) |> unsafe_string
"3.0.1"

So I think I have to be more careful saying which library each symbol actually comes from?

Yeah, the symbol is only in libnotcurses_core:

julia> using Notcurses_jll

julia> filter!(l -> endswith(l, "notcurses_version"), readlines(`nm $(Notcurses_jll.libnotcurses_path)`))
String[]

julia> filter!(l -> endswith(l, "notcurses_version"), readlines(`nm $(Notcurses_jll.libnotcurses_core_path)`))
1-element Vector{String}:
 "000000000003d3a0 T notcurses_version"

I'm not entirely sure how that works, but I think that on Linux the OS/dynamic linker may be happy to resolve the symbol for you in other linked library, while other OSes are stricter.

@KristofferC
Copy link
Member Author

Shows that it is good to test on multiple OSs :)

@dankamongmen
Copy link

Shows that it is good to test on multiple OSs :)

oh, i always want as widespread and diverse of testing as i can get, especially when i don't have to do it!

simeonschaub pushed a commit to simeonschaub/Yggdrasil that referenced this pull request Feb 23, 2022
* add netcurses

* also look for shared libraries on Windows

* ok...

* lowercase Secur32

* fix 32-bit windows

* fix license?

* [Notcurses] Simplify Windows `if`

* remove thingy

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants