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

Implement a meta-check for ncurses detection #1506

Closed
wants to merge 1 commit into from

Conversation

BenBE
Copy link
Member

@BenBE BenBE commented Jul 24, 2024

This was inspired by a patch for OpenEmbedded:
https://git.openembedded.org/meta-openembedded/tree/meta-oe/recipes-support/htop/files/0001-Use-pkg-config.patch?id=110667ac3798b5b64552cb4b8dc706aad8fbcdfe

Using this patch, detection can be switched as appropriate.

@BenBE BenBE added build system 🔧 Affects the build system rather then the user experience dependencies Pull requests that update a dependency file labels Jul 24, 2024
@BenBE BenBE added this to the 3.4.0 milestone Jul 24, 2024
@BenBE BenBE requested review from natoscott and fasterit July 24, 2024 08:12
@BenBE BenBE marked this pull request as draft July 24, 2024 08:23
@BenBE
Copy link
Member Author

BenBE commented Jul 24, 2024

Something seems off with the Ubuntu build. Any ideas?

@fasterit
Copy link
Member

Something seems off with the Ubuntu build. Any ideas?

I get the same on a local build with `--enable-static`` (on Debian bookworm) after your change:

...
checking for a way to check for ncurses... pkg-config (auto-detected)
checking for addnwstr in -lncursesw6... no
checking for addnwstr in -lncursesw... no
checking for addnwstr in -lncurses... no
configure: error: can not find required library libncursesw; you may want to use --disable-unicode

previously it compiles fine to a static executable and the configure output snippet is:

....
checking for waddwstr in -lncursesw6... no
checking for waddwstr in -lncursesw... yes
checking for ncursesw/curses.h... yes
checking for ncursesw/term.h... yes

@BenBE
Copy link
Member Author

BenBE commented Jul 24, 2024

Does it build with ./configure --enable-static --with-check-ncurses=nc-config? Currently the patch changes the default when auto-detecting the setting.

@fasterit
Copy link
Member

Does it build with ./configure --enable-static --with-check-ncurses=nc-config? Currently the patch changes the default when auto-detecting the setting.

Yes, it does.

...
checking for a way to check for ncurses... ncurses*-config helpers
checking for waddwstr in -lncursesw6... no
checking for waddwstr in -lncursesw... yes
checking for ncursesw/curses.h... yes
checking for ncursesw/term.h... yes
checking for library containing keypad... none required

@Explorer09
Copy link
Contributor

The problem in my opinion is that it doesn't make sense to let user specify how to check ncurses. The user should only specify whether ncurses exists, and optionally how to link to it.
So the option should look like this: --with-ncurses=<libname> while <libname> could be ncursesw6, ncursesw5, ncurses6, ncurses, etc.
Perhaps we can also introduce two variables NCURSES_CFLAGS and NCURSES_LDFLAGS just to allow user to override the flags when the values retrieved from "*-config" has errors.

Now, when it comes to automatic detection of ncurses, I can suggest this order:

  1. NCURSES_CFLAGS and NCURSES_LDFLAGS take highest priority if both are specified.
  2. --with-ncurses=<libname> if user specifies it. We can retrieve NCURSES_CFLAGS and NCURSES_LDFLAGS with the specified library name. First through pkg-config, and then <libname>-config. The pkg-config is preferred as it's a more generic library information utility. If pkg-config is not available, look for <libname>-config instead (which is specific to ncurses information).
  3. If user does not specify --with-ncurses, then we guess all possible names of ncurses installed in the user's system. First through pkg-config, and then <libname>-config.

@BenBE
Copy link
Member Author

BenBE commented Jul 24, 2024

@Explorer09 Please see the linked patch from OpenEmbedded for context.

@Explorer09
Copy link
Contributor

@BenBE I did read the linked patch, and thus I get the conclusion we are doing the wrong way.

The --with-check-ncurses variable should be internal to configure. These is no sense for user to specify that. Because either pkg-config works or it doesn't, and if user has to specify a awkward script name (say, ncursesfoo-config2) just to get the linker flags for ncurses, then their ncurses installation is broken. We don't need to find any script name other than <libname>-config because that's what the ncurses config information utility would be installed as. Thus, --with-ncurses=<libname> would be enough.

@BenBE
Copy link
Member Author

BenBE commented Jul 25, 2024

Ideally the method with which you can detect ncurses should not matter. The special case with OpenEmbedded is that you are building a system (from scratch) with minimal tooling available in the build environment, thus it's totally reasonable for them to assume pkg-config to be available (minimal dev tooling), but not ncurses-config (which may need emulation to run). After all OpenEmbedded is usually used for cross-building, thus minimizing reliance on tooling used in build systems is absolutely justified.

What I'm more confused about though is that pkg-config seems to fail when finding libncurses6 in the static build case, but works for the normally linked one. Thus running configure with either methods (or a secret third option) should not make a difference for the build as long as proper settings are returned. And based on the OE patch running ncurses6-config or pkg-config ncurses6 should be equal. And THAT is the question that currently needs solving.

@Explorer09 Explorer09 mentioned this pull request Aug 4, 2024
@Explorer09
Copy link
Contributor

The problem in my opinion is that it doesn't make sense to let user specify how to check ncurses. The user should only specify whether ncurses exists, and optionally how to link to it. So the option should look like this: --with-ncurses=<libname> while <libname> could be ncursesw6, ncursesw5, ncurses6, ncurses, etc. Perhaps we can also introduce two variables NCURSES_CFLAGS and NCURSES_LDFLAGS just to allow user to override the flags when the values retrieved from "*-config" has errors.

I implemented the idea in #1512. Please take a look and make some comments.

@BenBE BenBE closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system 🔧 Affects the build system rather then the user experience dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants