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

CI: Fix NetBSD build. #1509

Merged
merged 2 commits into from
Aug 5, 2024
Merged

CI: Fix NetBSD build. #1509

merged 2 commits into from
Aug 5, 2024

Conversation

fraggerfox
Copy link
Contributor

@fraggerfox fraggerfox commented Aug 4, 2024

Add --disable-unicode for now so that the CI passes the build without failing the ncurses check.

Fix the build with proper ncurses support for NetBSD 10.

@fraggerfox fraggerfox marked this pull request as ready for review August 4, 2024 17:17
@fraggerfox fraggerfox force-pushed the ci-fix-netbsd-build branch 2 times, most recently from 292a8b3 to fb98f64 Compare August 4, 2024 18:03
@Explorer09
Copy link
Contributor

How about we let NetBSD teams figure out what happened that breaks the build before we implement more workarounds to the problem?

@fraggerfox
Copy link
Contributor Author

fraggerfox commented Aug 4, 2024

@Explorer09 : Do you have some specific person or a team within NetBSD to figure out what happened? If not, I am a developer associated with NetBSD, who helps maintains this package.

The build within NetBSD 10 and pkgsrc currently does not enable unicode at least according to the configure options, but I could be wrong.

If you would like someone else from the NetBSD team to have a look, happy to close the PR.

@Explorer09
Copy link
Contributor

@fraggerfox The htop used to succeed building in NetBSD with --enable-unicode until upstream removed the obsolete ncursesw package. The upstream claimed that ncursesw is merged with ncurses (see this: NetBSD/pkgsrc@af6a20f), but somehow our configure script cannot detect the wide character functionality from the new ncurses. So there is a compatibility issue, either on our side or on NetBSD's side. I hope you can figure out which side has the issue.

By the way, the detection logic of ncurses is pending for a rework (see #1506). Maybe the two issues can be solved together.

@fraggerfox
Copy link
Contributor Author

@Explorer09 : Yup I am aware of the ncurses merger, as of now the NetBSD's pkgsrc links it against the base libcurses

/usr/pkg/bin/htop:
        -lexecinfo.0 => /usr/lib/libexecinfo.so.0
        -lelf.2 => /usr/lib/libelf.so.2
        -lc.12 => /usr/lib/libc.so.12
        -lgcc_s.1 => /usr/lib/libgcc_s.so.1
        -lcurses.9 => /usr/lib/libcurses.so.9
        -lterminfo.2 => /usr/lib/libterminfo.so.2
        -lprop.1 => /usr/lib/libprop.so.1
        -lkvm.6 => /usr/lib/libkvm.so.6
        -lm.0 => /usr/lib/libm.so.0

And the build was configured to use the built in curses, as mentioned in the Readme

I guess when I setup the CI I should have used the default values as pkgsrc was using https://github.com/NetBSD/pkgsrc/blob/trunk/sysutils/htop/options.mk#L18 since ncurses is marked as the optional one with curses as the default.

But if you would like to wait until the ncurses issue is resolved, I can close the PR for now and investigate on how to re-enable the ncurses logic.

@fraggerfox fraggerfox closed this Aug 4, 2024
@fraggerfox fraggerfox deleted the ci-fix-netbsd-build branch August 4, 2024 18:58
@Explorer09
Copy link
Contributor

@fraggerfox Wait a second. So NetBSD has ncurses as well as NetBSD's own curses implementation?

Can we let our CI select ncurses as default with --enable-unicode, and curses with --disable-unicode? This way we can satisfy both build cases, I mean users.

@BenBE BenBE added bug 🐛 Something isn't working build system 🔧 Affects the build system rather then the user experience NetBSD 🎏 NetBSD related issues labels Aug 4, 2024
@BenBE BenBE added this to the 3.4.0 milestone Aug 4, 2024
@fraggerfox fraggerfox restored the ci-fix-netbsd-build branch August 4, 2024 19:33
@fraggerfox fraggerfox reopened this Aug 4, 2024
@BenBE
Copy link
Member

BenBE commented Aug 4, 2024

Seems even more like a reason to implement a mechanism like #1506 to allow users to easily switch the detection. But that's just an aside. We can discuss the details for it over there.

Regarding this issue: I think, even if the proposed workaround somewhat "limits" the coverage on NetBSD, it's still better than no coverage at all. For the time being, I can live with the reduced feature set for now, as it also introduces a system with disabled Unicode support (which we do not have many AFAIR).

In the long run we should though, even if this PR is merged, aim to get the underlying issue on NetBSD fixed to restore full Unicode support.

run: |
set -e
./autogen.sh
CPPFLAGS="-I/usr/pkg/include -I/usr/pkg/include/ncurses" LDFLAGS="-L/usr/pkg/lib -Wl,-R/usr/pkg/lib" ./configure --enable-unicode --enable-werror
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restore the build with ncurses support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  htop 3.4.0-dev

  platform:                  netbsd
  os-release file:           /etc/os-release
  (Linux) proc directory:    /proc
  (Linux) openvz:            no
  (Linux) vserver:           no
  (Linux) ancient vserver:   no
  (Linux) delay accounting:  no
  (Linux) sensors:           no
  (Linux) capabilities:      no
  unicode:                   yes
  affinity:                  no
  unwind:                    no
  hwloc:                     no
  debug:                     no
  static:                    no

Output of ./configure

Copy link
Contributor

Choose a reason for hiding this comment

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

One issue here I see is that you need explicit CFLAGS and LDFLAGS in order to link against ncurces and not curses. This is not convenient for builders, and is the issue I wish to see it resolved in #1506.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When building via pkgsrc with the ncurses option these flags are set before doing ./configure

The ${MAKE_ENV} sets them in this case.

I agree it is not clean but with #1506 may be we can tweak the build to look cleaner outside the pkgsrc build script.

Copy link
Contributor Author

@fraggerfox fraggerfox Aug 4, 2024

Choose a reason for hiding this comment

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

I have removed the LDFLAGS and I think it should work now.

Due to ncurses being an additionally installed package I guess I still need to specify

LDFLAGS="-L/usr/pkg/lib"

Copy link
Contributor

Choose a reason for hiding this comment

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

@fraggerfox Is it possible to obtain the library include path via a pkg-config command? That's the way I expect to see it done.

LDFLAGS=$($PKG_CONFIG --libs ncurses)

Similarly for CPPFLAGS:

CPPFLAGS=$($PKG_CONFIG --cflags ncurses)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Explorer09 : Should be done in the latest commit.

@fraggerfox
Copy link
Contributor Author

fraggerfox commented Aug 4, 2024

Due to the following warnings

gcc -DHAVE_CONFIG_H -I.  -DNDEBUG -I/usr/pkg/include -I/usr/pkg/include/ncurses -std=c99 -pedantic -Wall -Wcast-align -Wcast-qual -Wextra -Wfloat-equal -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wpointer-arith -Wshadow -Wstrict-prototypes -Wundef -Wunused -Wwrite-strings -Wnull-dereference -Werror -D_XOPEN_SOURCE_EXTENDED -DSYSCONFDIR="\"/usr/local/etc\"" -I"./netbsd" -g -O2 -MT RichString.o -MD -MP -MF $depbase.Tpo -c -o RichString.o RichString.c &&\
mv -f $depbase.Tpo $depbase.Po
In file included from /usr/include/ctype.h:100,
                 from RichString.c:12:
RichString.c: In function ‘RichString_writeFromAscii’:
RichString.c:147:87: error: array subscript has type ‘char’ [-Werror=char-subscripts]
  147 |       this->chptr[i] = (CharType) { .attr = attrs & 0xffffff, .chars = { (isprint(data[j]) ? data[j] : L'\xFFFD') } };
      |                                                                                       ^
cc1: all warnings being treated as errors

and

gcc -DHAVE_CONFIG_H -I.  -DNDEBUG -I/usr/pkg/include -I/usr/pkg/include/ncurses -std=c99 -pedantic -Wall -Wcast-align -Wcast-qual -Wextra -Wfloat-equal -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wpointer-arith -Wshadow -Wstrict-prototypes -Wundef -Wunused -Wwrite-strings -Wnull-dereference -D_XOPEN_SOURCE_EXTENDED -DSYSCONFDIR="\"/usr/local/etc\"" -I"./netbsd" -g -O2 -MT Settings.o -MD -MP -MF $depbase.Tpo -c -o Settings.o Settings.c &&\
mv -f $depbase.Tpo $depbase.Po
In file included from /usr/include/ctype.h:100,
                 from Settings.c:12:
Settings.c: In function ‘toFieldIndex’:
Settings.c:235:19: warning: array subscript has type ‘char’ [-Wchar-subscripts]
  235 |    if (isdigit(str[0])) {
      |        

the build would still fail and we need to fix those before merging if we have --enable-werror, I am looking into this.

@BenBE
Copy link
Member

BenBE commented Aug 4, 2024

I think with some similar cases we mostly just added some explicit typecasts IIRC.

@fraggerfox
Copy link
Contributor Author

Due to the following warnings

gcc -DHAVE_CONFIG_H -I.  -DNDEBUG -I/usr/pkg/include -I/usr/pkg/include/ncurses -std=c99 -pedantic -Wall -Wcast-align -Wcast-qual -Wextra -Wfloat-equal -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wpointer-arith -Wshadow -Wstrict-prototypes -Wundef -Wunused -Wwrite-strings -Wnull-dereference -Werror -D_XOPEN_SOURCE_EXTENDED -DSYSCONFDIR="\"/usr/local/etc\"" -I"./netbsd" -g -O2 -MT RichString.o -MD -MP -MF $depbase.Tpo -c -o RichString.o RichString.c &&\
mv -f $depbase.Tpo $depbase.Po
In file included from /usr/include/ctype.h:100,
                 from RichString.c:12:
RichString.c: In function ‘RichString_writeFromAscii’:
RichString.c:147:87: error: array subscript has type ‘char’ [-Werror=char-subscripts]
  147 |       this->chptr[i] = (CharType) { .attr = attrs & 0xffffff, .chars = { (isprint(data[j]) ? data[j] : L'\xFFFD') } };
      |                                                                                       ^
cc1: all warnings being treated as errors

and

gcc -DHAVE_CONFIG_H -I.  -DNDEBUG -I/usr/pkg/include -I/usr/pkg/include/ncurses -std=c99 -pedantic -Wall -Wcast-align -Wcast-qual -Wextra -Wfloat-equal -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wpointer-arith -Wshadow -Wstrict-prototypes -Wundef -Wunused -Wwrite-strings -Wnull-dereference -D_XOPEN_SOURCE_EXTENDED -DSYSCONFDIR="\"/usr/local/etc\"" -I"./netbsd" -g -O2 -MT Settings.o -MD -MP -MF $depbase.Tpo -c -o Settings.o Settings.c &&\
mv -f $depbase.Tpo $depbase.Po
In file included from /usr/include/ctype.h:100,
                 from Settings.c:12:
Settings.c: In function ‘toFieldIndex’:
Settings.c:235:19: warning: array subscript has type ‘char’ [-Wchar-subscripts]
  235 |    if (isdigit(str[0])) {
      |        

the build would still fail and we need to fix those before merging if we have --enable-werror, I am looking into this.

These were an oversight from my end, they have been fixed in main.

@fraggerfox
Copy link
Contributor Author

fraggerfox commented Aug 4, 2024

@BenBE / @Explorer09 : I think we are good to merge this into main and see if the CI works.

https://github.com/fraggerfox/htop-dev/actions/runs/10239034633/job/28324149404

@fraggerfox fraggerfox force-pushed the ci-fix-netbsd-build branch 5 times, most recently from 80169b5 to 188bbd6 Compare August 4, 2024 20:45
@BenBE BenBE merged commit ebb5898 into htop-dev:main Aug 5, 2024
18 checks passed
@fraggerfox fraggerfox deleted the ci-fix-netbsd-build branch August 5, 2024 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working build system 🔧 Affects the build system rather then the user experience NetBSD 🎏 NetBSD related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants