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

When using curses, look for libterminfo as well. #9794

Merged
merged 2 commits into from May 16, 2023

Conversation

0-wiz-0
Copy link
Contributor

@0-wiz-0 0-wiz-0 commented May 16, 2023

Description

fish uses the cur_term symbol itself. On NetBSD (which has its own curses implementation) this is provided by libterminfo. The curses checks already look for libtinfo, extend this to look in libterminfo as well.

Fixes build failures like this:

[116/118] Linking CXX executable fish_key_reader
FAILED: fish_key_reader 
: && /scratch/shells/fish/work/.cwrapper/bin/c++ -O2 -g -fPIC -D_FORTIFY_SOURCE=2 -fstack-clash-protection -I/usr/pkg/include -I/usr/include -I/usr/pkg/include/python3.11 -fpermissive -Wredundant-move -g -DNDEBUG -lexecinfo -Wl,-zrelro -Wl,-znow -L/usr/lib -Wl,-R/usr/lib -L/usr/pkg/lib -Wl,-R/usr/pkg/lib CMakeFiles/fish_key_reader.dir/src/fish_key_reader.cpp.o CMakeFiles/fish_key_reader.dir/src/print_help.cpp.o -o fish_key_reader  libfishlib.a  /usr/lib/libcurses.so  -pthread  /usr/pkg/lib/libpcre2-32.so  /usr/pkg/lib/libintl.so && :
ld: libfishlib.a(env_dispatch.cpp.o): undefined reference to symbol 'cur_term'
ld: /usr/lib/libterminfo.so.2: error adding symbols: DSO missing from command line

@mqudsi mqudsi self-assigned this May 16, 2023
@@ -80,6 +80,11 @@ find_library(CURSES_TINFO tinfo)
if (CURSES_TINFO)
set(CURSES_LIBRARY ${CURSES_LIBRARY} ${CURSES_TINFO})
endif()
# on some systems, libterminfo has a longer name
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mention NetBSD here, so we aren't wondering down the line if this was in error or is no longer applicable.

Perhaps "On NetBSD, libtinfo has a longer name (libterminfo)"

(also it's libtinfo that has the longer name, and libterminfo is that longer name)

@0-wiz-0
Copy link
Contributor Author

0-wiz-0 commented May 16, 2023

Thanks for the review, I've updated the comment as suggested!

Copy link
Contributor

@mqudsi mqudsi left a comment

Choose a reason for hiding this comment

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

Sorry I didn't catch this earlier, but this block should be in the else() for if (CURSES_TINFO) so that we aren't checking if we already found libtinfo.

@0-wiz-0
Copy link
Contributor Author

0-wiz-0 commented May 16, 2023

Adapted, anything else?

@mqudsi
Copy link
Contributor

mqudsi commented May 16, 2023

If you've tested it and it still works on NetBSD with these changes then I think it LGTM.

@0-wiz-0
Copy link
Contributor Author

0-wiz-0 commented May 16, 2023

I think it's correct, but building git head currently fails for me with:

  --- stderr

  CXX include path:
    /archive/foreign/fish-shell-fork/a/./cargo/build/x86_64-unknown-netbsd/release/build/fish-rust-5ad3fdfa44e675d2/out/cxxbridge/include
    /archive/foreign/fish-shell-fork/a/./cargo/build/x86_64-unknown-netbsd/release/build/fish-rust-5ad3fdfa44e675d2/out/cxxbridge/crate
  /archive/foreign/fish-shell-fork/fish-rust/../src/proc.h:101:23: error: static assertion expression is not an integral constant expression
  /archive/foreign/fish-shell-fork/fish-rust/../src/proc.h:101:23: note: cast from 'void *' is not allowed in a constant expression
  Error: 
    × the include_cpp! macro couldn't be expanded into Rust bindings to C++:
    │ Bindgen was unable to generate the initial .rs bindings for this file.
    │ This may indicate a parsing problem with the C++ headers.

@faho
Copy link
Member

faho commented May 16, 2023

/archive/foreign/fish-shell-fork/fish-rust/../src/proc.h:101:23: note: cast from 'void *' is not allowed in a constant expression

That's some awkward static_assert:

static_assert(WIFEXITED(zerocode), "Synthetic exit status not reported as exited");

It can just be removed. Tho I have no idea why that would fail for you, or why it would fail now - that hasn't changed in 4 years. Possibly corrosion/cxx weirdness.

@mqudsi
Copy link
Contributor

mqudsi commented May 16, 2023

Yeah, master is in a state of flux as we are midway through a port to rust.

I'll tag your change for 3.6.2 which is a hypothetical patch release of the latest C++ version. You can check out LastC++11 and apply your patch against it to build the latest locally, if you like.

@mqudsi mqudsi added this to the fish 3.6.2 milestone May 16, 2023
@mqudsi mqudsi merged commit 67d1d80 into fish-shell:master May 16, 2023
6 of 7 checks passed
@mqudsi
Copy link
Contributor

mqudsi commented May 18, 2023

Yeah, I get similar but different hard errors that prevent me from compiling on an old macOS install because autocxx apparently consumes headers with -Wall -Wextra or something.

faho pushed a commit that referenced this pull request Oct 1, 2023
Supports NetBSD, where libtinfo isn't available but libterminfo is.

(cherry picked from commit 67d1d80)
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.

None yet

3 participants