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

Make linking against ncursesw opt-in #167

Merged
merged 1 commit into from Oct 17, 2016
Merged

Make linking against ncursesw opt-in #167

merged 1 commit into from Oct 17, 2016

Conversation

mptre
Copy link
Owner

@mptre mptre commented Oct 14, 2016

I've always found it odd that pick favors linking against ncursesw if
present instead of curses. This gets even more interesting on my machine
where the ncursesw library is present but not the headers. This means in
practice: the curses headers are used but later linked against ncursesw.
However, it does work since ncursesw implements the same API as curses.

My proposal is to make linking against ncursesw a opt-in feature since
the more common curses library should be favored if available.

This patch adds a --{with,without}-ncursesw option to the configure
command, which defaults to false. The change is therefore breaking in
sense that users with only ncursesw available has to pass the new option
while running configure. A reasonable compromise in my opinion since
less implicit dependency resolving is a good thing. Worth mention: the
major of the users will not notice this change if curses is available
(most likely).

If I where to decide I would go even further and trust the user: if the
ncursesw option is given, assume the both headers and libraries are
present. Much of the autoconf verification could then be dropped -> less
code to maintain.

Left-over notes:

  • I'm planning on running Travis with and without ncursesw
  • Any advice or convention on how the format all the parenthesis and
    brackets inside configure.ac would be appreciated

Thoughts @calleerlandsson and @mike-burns?

@mike-burns
Copy link
Collaborator

For history, see #49.

Copy link
Collaborator

@calleluks calleluks left a comment

Choose a reason for hiding this comment

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

Great initiative, @mptre! Thanks for always looking for ways to simplify and improve the code base.

I'm not entirely sure why one would favor linking against any of curses, ncurses, or ncursesw over the other, other than it being the only one available on the system. Are there other differences than the fact that ncursesw supports wide chars?

My understanding of the changes in #49 is that they were intended to support systems that only had ncursesw installed.

Unless there are reasons to link against a specific version, what do you think about try linking to any of them available favoring the simplest one if multiple are available? I'm guessing this would be curses > ncurses, ncursesw?

I think the formatting looks fine.

@eavgerinos what are your thoughts on these changes and ideas?

[],
[with_ncursesw=no],
)
AS_IF([test "x$with_ncursesw" != xno],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious: why "x$with_ncursesw" != xno and not "$with_ncursesw" != no?

],
[AC_MSG_ERROR([unable to find setupterm function using ncursesw])]
),
AC_SEARCH_LIBS([setupterm], [curses], [],
Copy link
Collaborator

Choose a reason for hiding this comment

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

The old AC_SEARCH_LIBS mentioned ncurses instead of curses. I'm not sure what the differences between them are.

@mptre
Copy link
Owner Author

mptre commented Oct 15, 2016

I'm not entirely sure why one would favor linking against any of
curses, ncurses, or ncursesw over the other, other than it being
the only one available on the system. Are there other differences than
the fact that ncursesw supports wide chars?

We don't use any features of ncurses at this point. But if curses is not
available, the ncurses implementation of curses is used.

Unless there are reasons to link against a specific version, what do
you think about try linking to any of them available favoring the
simplest one if multiple are available? I'm guessing this would be
curses > ncurses, ncursesw?

I like this idea. Less knobs to fiddle with is a good thing. See the
updated commit. We still need to define HAVE_NCURSESW_H in order to
include the correct headers.

Just curious: why "x$with_ncursesw" != xno and not "$with_ncursesw" != no?

Adopted the style in the autoconf manual.

The old AC_SEARCH_LIBS mentioned ncurses instead of curses. I'm
not sure what the differences between them are.

Using curses is sufficient for pick. We could add ncurses as a fallback
prior to ncursesw if people can't build on some platforms.

Copy link
Collaborator

@calleluks calleluks left a comment

Choose a reason for hiding this comment

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

Sounds and looks great to me!

@mptre mptre merged commit 85543a3 into master Oct 17, 2016
@mptre mptre deleted the ncursesw branch October 17, 2016 17:03
@mptre
Copy link
Owner Author

mptre commented Oct 17, 2016

Thanks, merged.

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.

None yet

3 participants