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

New "configure" ncurses detection code #1512

Merged
merged 10 commits into from
Sep 20, 2024
Merged

Conversation

Explorer09
Copy link
Contributor

This PR is intended to supersede #1506

New detection code for curses library for configure.

  • Both pkg-config and ncurses*-config will be used to find the compiler flags and linker flags for curses/ncurses. Fall back to simple -l<libname> linking if neither utilities are available.
  • Environment variables CURSES_CFLAGS and CURSES_LIBS may be used to override the --cflags and -libs value.
  • New configure option --with-curses=<libname> to specify the curses/ncurses module name. Should be used if the default detect list is not sufficient to find the library.

@BenBE BenBE added enhancement Extension or improvement to existing feature build system 🔧 Affects the build system rather then the user experience labels Aug 6, 2024
@Explorer09
Copy link
Contributor Author

Explorer09 commented Aug 6, 2024

I found one possible room for improvement:

EDIT: This is done in a separate commit.


In the static build of htop (see this CI build log), -ltinfo is sometimes required (to be specified after -lncursesw) to link to ncurses. But our configure script detects tinfo after curses, which is probably of the cause of the configure error as seen in this build log in PR #1506.

Apparently the --libs output from pkg-config should be self-contained (with all the dependency libraries listed), but the detection would matter when pkg-config is not available.

@Explorer09 Explorer09 force-pushed the ncurses-detect branch 3 times, most recently from b77e202 to 2019d85 Compare August 7, 2024 14:46
configure.ac Outdated Show resolved Hide resolved
@BenBE
Copy link
Member

BenBE commented Aug 17, 2024

@explorer: Please don't use draft in a commit message. Commits should aim to be "the final product". If the patch series overall is not ready for merging yet, that's what the "draft" status of PRs is for.

On the patch series: LGTM. Maybe switch the order of the last two commits. If there's no objection from the OpenEmbedded people, I think we can go ahead with this one.

@Explorer09
Copy link
Contributor Author

Please don't use draft in a commit message. Commits should aim to be "the final product". If the patch series overall is not ready for merging yet, that's what the "draft" status of PRs is for.

I say "draft" in the commit because I don't think I'm done yet, and to prevent accidental pulling. Don't worry, I'll remove that tag when I think I'm ready.

configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
@Explorer09 Explorer09 force-pushed the ncurses-detect branch 3 times, most recently from a811dcd to 9b58ebd Compare August 19, 2024 11:15
@Explorer09 Explorer09 marked this pull request as ready for review August 19, 2024 11:40
@Explorer09
Copy link
Contributor Author

The configure script is revised and ready. However I got an unexpected configure error in OpenBSD build job in CI. It seems that OpenBSD build system in CI has pkg-config and libncursesw, but there is no ncurses.pc file describing the configuration. I can't tell if there is a package missing (to be installed) or if the system is designed that way.

@Explorer09 Explorer09 force-pushed the ncurses-detect branch 2 times, most recently from 0d12439 to ba89c02 Compare August 19, 2024 20:42
@BenBE
Copy link
Member

BenBE commented Aug 19, 2024

There's also a failure for Solaris AFAICS. Any intel on that one?

@Explorer09
Copy link
Contributor Author

There's also a failure for Solaris AFAICS. Any intel on that one?

@BenBE I was testing whether htop can build on all BSD systems and Solaris with pkg-config installed. Ideally I want the ncurses compile and linker flags all grabbed from pkg-config, but some systems don't seem to provide the ncurses.pc file for us to use.

@Explorer09 Explorer09 marked this pull request as draft August 19, 2024 22:15
@Explorer09 Explorer09 marked this pull request as ready for review August 21, 2024 11:27
@Explorer09 Explorer09 force-pushed the ncurses-detect branch 4 times, most recently from ab52bf9 to ba0735e Compare August 21, 2024 22:12
@BenBE
Copy link
Member

BenBE commented Sep 18, 2024

Some minor notes on the current state of the patch:

diff --git a/configure.ac b/configure.ac
index 6347859f..7c9a745b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -383,11 +383,13 @@ dnl PKG_PROG_PKG_CONFIG initializes $PKG_CONFIG and related variables.
 dnl If the macro is not called, some pkg-config checks might be skipped
 dnl and $PKG_CONFIG might be unset.
 m4_ifdef([PKG_PROG_PKG_CONFIG], [
-PKG_PROG_PKG_CONFIG
+   PKG_PROG_PKG_CONFIG()
 ], [
-pkg_m4_absent=1
-m4_warn([syntax],
-   [pkg.m4 is absent or older than version 0.16; this 'configure' would have incomplete pkg-config support])
+   pkg_m4_absent=1
+   m4_warn(
+      [syntax],
+      [pkg.m4 is absent or older than version 0.16; this 'configure' will have incomplete pkg-config support]
+   )
 ])
 
 AC_ARG_ENABLE([unicode],
@@ -565,14 +567,14 @@ esac
 
 case ${htop_curses_capability}-$enable_unicode in
 none-*|nonwide-yes)
-   if test "x$PKG_CONFIG" = x; then (
+   if test "x$PKG_CONFIG" = x; then
       # Friendly warning to advise user to install pkg-config.
       # The local variables get discarded when done.
       list=""
       echo "" >conftest.c
       if test "$cross_compiling" != yes; then
          # "-print-multi-directory" supported in GCC 3.0 or later
-        name=""
+         name=""
          if $CC $CPPFLAGS $CFLAGS -print-multi-directory -E conftest.c >/dev/null 2>&1; then
             name=`$CC $CPPFLAGS $CFLAGS -print-multi-directory -E conftest.c 2>/dev/null |
                sed '/^ *#/ d; s/^\.$//; q'`
@@ -610,7 +612,7 @@ none-*|nonwide-yes)
             break
          fi
       done
-   ) fi
+   fi
 
    # OpenBSD and Solaris are known to not provide '*curses*.pc' files.
    AC_MSG_RESULT([no curses information found through '*-config' utilities; will guess the linker flags])
@@ -796,7 +798,7 @@ AC_DEFINE_UNQUOTED([OSRELEASEFILE], ["$with_os_release"], [File with OS release
 
 AC_ARG_WITH([config],
             [AS_HELP_STRING([--with-config=DIR],
-                           [configuration path @<:@default=/.config@:>@])],
+                            [configuration path @<:@default=/.config@:>@])],
             [],
             [with_config="/.config"])
 dnl Performance Co-Pilot configuration location to prevent overwrite
diff --git a/netbsd/README.md b/netbsd/README.md
index c319c199..30b344a1 100644
--- a/netbsd/README.md
+++ b/netbsd/README.md
@@ -13,8 +13,8 @@ NetBSD is one of the last operating systems to use and maintain its own
 implementation of Curses.
 
 htop(1) can be compiled against either ncurses or NetBSD's curses(3).
-htop(1) would use ncurses by default when it is found, as NetBSD's curses
-support in htop is limited.
+By default, htop(1) will use ncurses when it is found, as support for
+NetBSD's curses in htop is limited.
 
 To use NetBSD's libcurses, htop(1) must be configured with `--disable-unicode`.
 Starting with htop 3.4.0, a new option `--with-curses=curses` may be specified


case ${htop_curses_capability}-$enable_unicode in
none-*|nonwide-yes)
if test "x$PKG_CONFIG" = x; then (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replying the suggestion from @BenBE.

The parentheses on that if block was intentional by me. This way I can isolate the local variables used within since all this block does is printing warning messages and it can all be done in a subshell.

Copy link
Member

Choose a reason for hiding this comment

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

Reasonable. The way that scope is introduced just looks kinda strange without any annotation.

I'm also not sure if starting a subshell just for isolation is the right choice here or if braces ({}) might do the trick too …

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The {} won't isolate local variables like I had originally intended.
If it was braces {} block, then it's no different from having no braces at all.

I wonder if I can make this code more obvious by turning into a shell function.

pkg-config will be used for detecting 'curses' library in a future
commit. We have been using pkg-config for detecting multiple libraries
('hwloc' and formerly 'libnl'), thus the PKG_PROG_PKG_CONFIG macro
should be called early.

Signed-off-by: Kang-Che Sung <[email protected]>
Commit 24b1513 removed the warning
when the configure script is generated without pkg.m4. I added that
warning years ago (see 103f1a4) to
prevent downstream distributions from creating a tarball with an
"incomplete" configure script. Add the warning back, reword the
messages to tell exactly what feature would be missing (to builders),
and also add FORCE_MAKE_DIST variable for builders who want to ignore
the warning. :)

Signed-off-by: Kang-Che Sung <[email protected]>
This allows configure to be generated with pkg.m4 in FreeBSD build job.
The new curses library detection code in configure script
* Uses pkg-config for detecting CFLAGS and LIBS for curses library,
  falls back to 'ncurses*-config' if pkg-config is not available, and
  falls back to "-l${library}" if both fail.
* Supports override of compiling flags and linking flags through
  CURSES_CFLAGS and CURSES_LIBS variables.
* Supports '--with-curses=library' option to manually specify the curses
  library name.

Signed-off-by: Kang-Che Sung <[email protected]>
Since "-lncurses" might require explicit "-ltinfo" flag to link
(especially for static libncurses without libtool or pkg-config),
"-ltinfo" needs to be checked alongside "-lncurses" and not after it.

Signed-off-by: Kang-Che Sung <[email protected]>
If we can detect the presence of '*curses*.pc' files in some
pkg-config default search directories, print a warning message.

Signed-off-by: Kang-Che Sung <[email protected]>
Let configure detect ncurses if possible
@BenBE BenBE merged commit 8a6e519 into htop-dev:main Sep 20, 2024
18 checks passed
@BenBE BenBE added this to the 3.4.0 milestone Sep 20, 2024
@Explorer09 Explorer09 deleted the ncurses-detect branch September 20, 2024 21:56
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 enhancement Extension or improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants