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

Specify terminfo dirs #23

Closed
wants to merge 5 commits into from
Closed

Specify terminfo dirs #23

wants to merge 5 commits into from

Conversation

keuv-grvl
Copy link

@keuv-grvl keuv-grvl commented Sep 19, 2016

@@ -21,6 +21,8 @@ do
--enable-termcap \
--enable-pc-files \
--with-termlib \
--with-terminfo-dirs="/etc/terminfo:/lib/terminfo:/usr/share/terminfo:$PREFIX/share/terminfo" \
--with-default-terminfo-dir="/usr/share/terminfo" \
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the default should be ours, no?

Copy link
Member

Choose a reason for hiding this comment

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

Done.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jakirkham
Copy link
Member

Please bump the build number as well.

@jakirkham
Copy link
Member

@mingwandroid, could you please take a look at this and share your thoughts?

@mingwandroid
Copy link
Contributor

mingwandroid commented Sep 19, 2016

If you build --with-termlib (which I do not since it caused a lot of pain for me) then you probably want the ones in $PREFIX first. If you don't build --with-termlib then you probably want to have the ones in $PREFIX last.

Perhaps both building --with-termlib and setting the terminfo-dirs appropriately would fix my issues but I'm not really convinced. ncurses is fairly complicated, reading on and also setting various env. vars.

My take was to defer to the system as much as I could for everything above ncurses itself, so my preference was to remove --with-termlib and not pass --with-terminfo-dirs at all.

The only useful advice I can give is to test on a multitude of different Linux installations and if it works on them all, then I'd call that good enough, but my instinct would be for you guys to use my recipe as reference here:

https://github.com/mingwandroid/conda-recipes/tree/master/ncurses

.. because if it isn't broke (and is well tested) then don't fix it.

@jakirkham
Copy link
Member

However we can't use your recipe as dropping --with-termlib will break tons of things at this point. Plus this breaks with the general trend of using that option on Linux distros. It would be great if we could figure out why things on your end won't work with that option honestly.

@@ -21,6 +21,8 @@ do
--enable-termcap \
--enable-pc-files \
--with-termlib \
--with-terminfo-dirs="/etc/terminfo:/lib/terminfo:/usr/share/terminfo:$PREFIX/share/terminfo" \
Copy link
Member

Choose a reason for hiding this comment

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

So the suggestion is to move $PREFIX/share/terminfo to the front of the list, which seems reasonable.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@mingwandroid
Copy link
Contributor

What will break? Are you sure anything will? These changes to ncurses on conda-forge broke compatibility with defaults.

What Linux distros do is not always instructive for what conda/conda-forge should do because we do not provide a complete software stack down to glibc and the kernel so accommodations like this are sometimes necessary; you can call out to some conda-forge package from something linked to and running the system's ncurses, having already set a bunch of env. vars and this is where you'll probably have difficulties. My change to remove --with-termlib was an accommodation for co-existing on the system, and I tested it on a vast range of Linux distributions.

My suggestion is to unify your package with ours.

@jakirkham
Copy link
Member

No we had fixed that in PR ( #19 ). AFAICT the previous build and this build of ncurses work fine in the continuumio/miniconda image.

Please see issue ( ContinuumIO/anaconda-issues#455 ) for a bit more history on why we have our own termlib.

@mingwandroid
Copy link
Contributor

mingwandroid commented Sep 19, 2016

I find it hard to follow these multiple threads of discussion but what I did find seemed to circle back to what you already said "We did it because that's what Linux distros do".

The fact is, since I removed --with-termlib (and the other fixes), ncurses in defaults has had no reported problems against it, the Python curses module works, the R interpreter works and nano works, all on large array of Linux distros and OS X 10.7 and above (most of which was previously untrue). conda-forge and conda defaults need to be as compatible as possible, esp. for somewhat important system libraries like ncurses, and making breaking changes like this because other Linux distros do it isn't a way to achieve that goal.

I would urge you to push changes to conda-recipes where I will test them with defaults Python and R packages when such big changes are being made to system-facing libraries.

@jakirkham
Copy link
Member

jakirkham commented Sep 19, 2016

I find it hard to follow these multiple threads of discussion but what I did find seemed to circle back to what you already said "We did it because that's what Linux distros do".

While there is some of that. This is not the crux of the problem.

The problem is the system may supply a newer copy of the terminfo db from a newer ncurses than we are using (e.g. 6 vs. 5). So we do need to supply our own terminfo db and we definitely want to use ours first (if not only ours).

If we must, we can try to fallback to the system as proposed here. Though it may causes issues if the system has a newer version.

@jakirkham jakirkham mentioned this pull request Sep 19, 2016
@mingwandroid
Copy link
Contributor

mingwandroid commented Sep 19, 2016

I build on CentOS 5.11 then test on that, then I test on a fully up to date ArchLinux system (which uses ncurses-6). This procedure gives me a good level of confidence about the compatibility of my packages.

It seems to me, from #19, that an incorrect partial fix was applied (after I had shared my experience that removing --with-termlib worked well) and now another attempt is being made to do something in the same vein.

However the important thing is that if your way proves itself to work well, and you run with it, then your ncurses package will break everything in defaults that is linked to our ncurses, so as soon as someone installs htop, it becomes a support issue for me with defaults' Python and R language (and also for conda-forge and looks pretty bad overall).

Whatever we do for ncurses should be done in common so that we are not incompatible, and in this case, I got there first and tried to advise.

Note, with this, if you find an actual bug with my way and your way fixes it then great, I will adopt your way and rebuild everything that I need to as soon as I can. This isn't about trying to prove who is right or wrong, it's about being compatible and building working software.

@jakirkham
Copy link
Member

We have been using --with-termlib since this package was added back in April. We also discussed dropping it in PR ( #10 ) before packaging python and other things that relied on it. The decision was not to do so. We worked with other user groups downstream (like bioconda) to add .pc files to make it easier to work with in PR ( #11 ).

As everyone is reliant on ncurses here being built using --with-termlib, switching will causes problems not just at conda-forge, but for other communities reliant on us and this way of doing things. I fail to see how switching it now is a good plan that avoids breakage for the larger conda community.

The proposal here seems like a simple reasonable change that should help out in a few other cases by searching some system paths. Feel free to test other things against this change. That being said, unless there are some clear blockers, I'm in favor of going forward with this strategy.

@jakirkham
Copy link
Member

Going to put a hold on this while we work on htop some more.

@keuv-grvl
Copy link
Author

I admit it has been a bit technical for me here :)

Anyway, we worked on my PR to add htop to the conda-forge. Now it builds successfully (locally and within a Docker container) without changing the ncurses recipe, which is nice.

So I would close this PR.
Thank you for your help!

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.

4 participants