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

switch back to standard ncurses #1880

Merged
merged 4 commits into from
Aug 21, 2017
Merged

Conversation

CvH
Copy link
Member

@CvH CvH commented Aug 15, 2017

This changes the build system to use standard ncurses instead of netbsd-curses.
Motivation for this step back is the lack of support for netbsd-curses in real world and the resulting problems due this.
Afaik beside LE/OE and Sabotage Linux nobody is using this Mod of the curses implementation for netbsd.
So better stick with the Linux default to avoid useless time wasting due this.

Currently I have not squashed any commits (will do before rm WIP).

It looks like there is currently still a problem that curses get stuff from outside the build system (same problem for netbsd-curses). I have no idea how to fix this or even debug it (help welcome).
Beside that it build for me, but as seen in the past this may differ from system to system.

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 15, 2017

What exactly is the problem with curses get stuff from outside the build system?
Will build projects here to find any potential problem.

@CvH
Copy link
Member Author

CvH commented Aug 15, 2017

@vpeter4 I couldn't track it down (due no idea how to debug)
We had already that it builds at my clean VM, at my usual (rather dirty due other stuff) VM not (always 100% clean build dir) - same Ubuntu.
Also at Ubuntu there are problems that do not appear at Arch Linux.

So there are still depends to the outside of the builddir (btw I have no better explanation).

For example we tried to compile https://github.com/escalade/LibreELEC.tv/tree/le82/ MC/htop (without current patch) at Ubuntu that never worked, it worked well at Arch (this is what @escalade build from).
At Arch it found curses without any problem, at Ubuntu only due rather big hacks - this shouldn't happen (I guess). Maybe @escalade has some better explanation/ideas :) (this is really not my expertise, so pls feel free to disagree with everything I might say)

Also the rather frequently need of -I$SYSROOT_PREFIX/usr/include/ncurses looks rather strange.

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 15, 2017

I think I understand the problem :)

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 15, 2017

Did some tests but at the end I broke something - need to rebuild.

But from what I see is that ncurses needs to be configured with --with-termlib. This builds two libs: libncurses.a and libtinfo.a. The second one is looked by some configure scripts. If there is no libtinfo then libncurses includes it's functionality (could also be symbolic link from libncurses to libtinfo).

Then htop patch can be removed, without -I...ncurses and with PKG_CONFIGURE_OPTS_TARGET="--disable-unicode" in package.mk.

Continue investigation...

Update:
now connmand needs export LIBS="$LIBS -ltinfo"

@CvH
Copy link
Member Author

CvH commented Aug 15, 2017

@vpeter4

Htop patch can be removed, without -I...ncurses and with PKG_CONFIGURE_OPTS_TARGET="--disable-unicode" in package.mk.

PKG_CONFIGURE_OPTS_TARGET="--disable-unicode"

pre_configure_target() {
  export CFLAGS="$CFLAGS -fno-strict-aliasing -lncurses"
}

doesn't build for me (tried different things too)
configure: error: missing libraries: libncurses without patch

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 15, 2017

You need to rebuild ncurses to get libtinfo library.

But seems with this ncurses change some packages needs export LIBS="$LIBS -ltinfo" (connman, bluez and maybe others). Not sure if this is correct approach.

Regarding ncurses and libtinfo here is some discussion (very old one but seems still appropriate): https://bugs.launchpad.net/ubuntu/+source/ncurses/+bug/259139

@CvH
Copy link
Member Author

CvH commented Aug 15, 2017

@vpeter4 adding --with-termlib and added LIBS="$LIBS -ltinfo" won't help either (maybe you done something else ?)

@escalade
Copy link
Contributor

escalade commented Aug 16, 2017

@vpeter4

Why would you need libtinfo? The bugreport you are linking to is with regards to a third party library being linked to libtinfo. That's not needed in a distribution like LibreELEC where everything is compiled. I don't use --with-termlib in my build and Arch does not use this option either. It's not the default and probably for a reason.

Options like that should not be used without purpose. As you can see other packages will actually need modification to compile against ncurses if you are using this option (-ltinfo).

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 16, 2017

I don't need libtinfo. But configure script for example htop fails without (I think). Also this lib is present on Ubuntu 16.04.3 LTS.
Yes, other packages who explicitly needs -lncurses would also need -ltinfo in this case.

But I'm just exploring options. Seems no changes is needed anyway.

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 16, 2017

But I think ncurses could be build without --disable-overwrite. Then headers will be installed in usr/include and not in ncurses subfolder. Which means no need to set -I.../ncurses in package.mk.

@escalade
Copy link
Contributor

escalade commented Aug 16, 2017

That's because it's package specifies -lterminfo in CFLAGS. I can compile htop without any patches just fine both with netbsd-curses and ncurses (needs --disable-unicode).

The option --disable-overwrite is already included. Where do you see -I../ncurses?

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 16, 2017

Ncurses is build with --disable-overwrite. This installs all it's headers in subfolder SYSROOT_PREFIX/usr/include/ncurses. And because of this some packages needs CFLAGS -I$SYSROOT_PREFIX/usr/include/ncurses. This is what cvh mentioned.

@escalade
Copy link
Contributor

escalade commented Aug 16, 2017

Yes sorry, it should be excluded, my original commit had this (excluded). Makes absolutely no sense to use a separate include dir when packages expect the opposite.

@CvH
Copy link
Member Author

CvH commented Aug 16, 2017

@escalade I had the expectation that the "old" ncurses had worked properly (as there were not those workarounds), I try without

@escalade
Copy link
Contributor

They used this option in the old package and as a consequence had to modify other packages to find the correct include path. It wouldn't be a bad idea to go through every option used in this package to see if it's actually needed. I believe lots of them are the default already, so IMO no need to specify them.

@CvH
Copy link
Member Author

CvH commented Aug 16, 2017

@vpeter4 I squashed some of the commits and added the rm of the --disable-overwrite.
Due this all those -I$SYSROOT_PREFIX/usr/include/ncurses" could be removed (packages remove workarounds). Everything is build tested and works for me.

The only concern is about htop, it still needs the patch and still much more then what @escalade has needed.

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 17, 2017

For htop: remove the patch and use this function

pre_configure_target() {
   PKG_CONFIGURE_OPTS_TARGET="--disable-unicode HTOP_NCURSES_CONFIG_SCRIPT=ncurses-config"
}

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 17, 2017

I think mc package doesn't need pre_configure_target function anymore.
Also iftop doesn't need -lncurses in LIBS.

@CvH
Copy link
Member Author

CvH commented Aug 17, 2017

@vpeter4 the cflags are
CFLAGS="-march=armv7ve -mtune=cortex-a7 -Wl,--as-needed -fuse-ld=gold -fuse-linker-plugin -flto"
with that sed the cflags are (-lncurses looks now useless)
CFLAGS="-march=armv7ve -mtune=cortex-a7 -fuse-ld=gold -fuse-linker-plugin -flto -lncurses"

both builds so I guess we go with removing the pre_configure_target at mc
I added 2 commits for htop/mc, the other 2 commits are just minor cosmetic.

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 18, 2017

Seems it builds fine here for Generic. Other projects will follow.

@MilhouseVH
Copy link
Contributor

No problems building RPi/RPi2/Generic along with various PVR addons.

@CvH
Copy link
Member Author

CvH commented Aug 18, 2017

squashed commits, updated dates and removed WIP
from my pov good to go

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 18, 2017

What is the point of readline-001-shlib_libs.patch for readline? Because variable SHLIB_LIBS is used only for shared library (which we don't build).

@CvH
Copy link
Member Author

CvH commented Aug 18, 2017

Iirc it didn't build without, maybe you could build it without patch (I need to redo whole build)?

@vpeter4
Copy link
Contributor

vpeter4 commented Aug 18, 2017

It builds fine here without patch. But I only changes readline package - everything else was old.
Also I don't see any linkage with curses library anyway.
Also I think SHLIB_LIBS could be supplied from environment - meaning patch is double useless :)

Will do clean build over night without this patch and see.

@CvH
Copy link
Member Author

CvH commented Aug 18, 2017

@vpeter4 I guess this problem was fixed with the removal of --disable-overwrite , hadn't tested afterwards if it works without the patch. It build (dirty) for me too. A clean build would not hurt. I push the removal of the patch so it could be merged (I am away till monday).

Copy link
Contributor

@vpeter4 vpeter4 left a comment

Choose a reason for hiding this comment

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

All good at my side @12:23, Saturday, 19 August 2017 (CEST) :)

@MilhouseVH MilhouseVH merged commit f694147 into LibreELEC:master Aug 21, 2017
@MilhouseVH
Copy link
Contributor

Thanks @CvH

@CvH CvH deleted the 9.0-ncurses branch August 21, 2017 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants