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

WIP: Combine libtinfo with libncurses #10

Closed
wants to merge 1 commit into from
Closed

WIP: Combine libtinfo with libncurses #10

wants to merge 1 commit into from

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented May 7, 2016

Needs #12 and #11 to be merged first.

Fixes #9

Some users expect to find all of libtinfo's symbols in libncurses. While not all builds of libncurses do this and the trend has been to have a separate libtinfo, this puts them back together in the same library. However, it does provides symlinks named after the libtinfo libraries that point back to libncurses. This way programs that expect to link to libtinfo still can, but will get those symbols from libncurses. Part of the reason this has been opted for is defaults does the same thing. We have included some tests to verify that some of the libtinfo symbols are in libncurses, which should be sufficient in verifying this works.

After this is merged, anything built with ncurses support is going to have to be rebuilt as this will break those packages. Currently, that is just erlang, which needs to be pinned anyways ( conda-forge/erlang-feedstock#5 ). Though it would be good to get this in before our own python is released so that we will have some time to test this new configuration with that build and release it using this new configuration. Thus, this is a good time to make this break if we are going to do it.

xref: bioconda/bioconda-recipes#1391

cc @pelson @ocefpaf @msarahan @jjhelmus @scopatz @cel4 @croth1 @johanneskoester

@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.

DYLIB_EXT=dylib
else
DYLIB_EXT=so
fi
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to have the right library extension for each platform.

@jakirkham
Copy link
Member Author

Should add that we welcome other maintainers on board this feedstock (as with the whole ecosystem). As this seems important to bioconda, maybe it would be good to have a few of your people on board. 🚢 Please let us know if you are interested, we are always happy to have other people join us. 😄

@jakirkham
Copy link
Member Author

Rebased on top of this PR ( #11 ) to provide pkg-config support.

@jakirkham
Copy link
Member Author

jakirkham commented May 7, 2016

This seems like it should work ok, but I would like someone to confirm that it works for them.

Also, I have tried very hard to ensure that from a build standpoint everything will still behave correctly whether or not one expects a separate tinfo. However, I may not have caught all such cases and would appreciate careful eyes looking for any such missing thing.

Even though we want this to be fine from a build standpoint, it will be a breaking change from the runtime standpoint. As a result, anything that is already built with ncurses will be broken and need to be rebuilt with an ncurses that includes this change.

Finally, it seems that the ncurses devs have been pushing for these to be two separate libraries (libncurses and libtinfo). Most Linux distros provide them as two separate libraries. Mac, which has a very old version of ncurses, provides them as one. While we can push them together into libncurses, it feels like a step backwards and it is unclear how long that behavior will be supported. It seems more reasonable that packages expecting this behavior get updated to handle the fact that these libraries may be separate. Adding support for pkg-config ( #11 ) will make that easier.

That being said, I don't have very strong feelings against this, but am just a little wary. If this is very desirable behavior from the community perspective, I'm willing to discuss this. Hence why I have generated a PR that implements behavior so we can discuss it more.

@jakirkham jakirkham changed the title Combine libtinfo with libncurses WIP: Combine libtinfo with libncurses May 7, 2016
@jakirkham
Copy link
Member Author

I have marked this as WIP so no one accidentally merges this. However, this is really a working, tested change at this point. People are welcome to play with it if they like. We just should not merge it until much more discussion has occurred and we decide we want to go in this direction. Personally, I'm not yet convinced.

@msarahan
Copy link
Member

msarahan commented May 8, 2016

+1 for following the ncurses team lead and not merging these. This might be some crappy maintenance work to add in the extra library in various makefiles, but it will be worth it, IMHO.

@jakirkham
Copy link
Member Author

jakirkham commented May 8, 2016

Agreed. Only proposing this for the convenience of downstream. If providing some pkg-config files ( #11 ) is enough, I would rather go that way and close this.

@croth1
Copy link

croth1 commented May 8, 2016

I am fine with keeping the status quo. Patching the Makefile and using ncurses-config and pkg-config to get the flags required for linking seems to work fine.

@@ -20,14 +27,23 @@ do
--enable-symlinks \
--enable-termcap \
--enable-pc-files \
--with-termlib \
Copy link
Member Author

Choose a reason for hiding this comment

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

Dropping this options gets rid of libtinfo and combines it in with libncurses.

@pelson
Copy link
Member

pelson commented May 10, 2016

No reason from me to not do this. 👍

@jakirkham
Copy link
Member Author

No reason from me to not do this. 👍

Personally, I don't love it. This seems to be opposite all of the Linux distros I have seen and is only really the same as Mac (which has an ancient copy).

Mainly, I put this together so we could see it is possible and discuss with downstream if this is something they would want. Thus far it seems like they are happy with the pkg-config additions ( #11 ) and patching Makefiles in some things that require ncurses to find libtinfo.

Honestly, I would be in favor of closing. Especially as it looks like they now don't need it.

@pelson pelson closed this May 11, 2016
@jakirkham jakirkham deleted the combine_libtinfo branch May 13, 2016 05:48
@jakirkham jakirkham mentioned this pull request Aug 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants