msys2 / MINGW-packages

Package scripts for MinGW-w64 targets to build under MSYS2.
https://packages.msys2.org
BSD 3-Clause "New" or "Revised" License
2.28k stars 1.21k forks source link

[pdcurses] #12322

Open netdoggy opened 2 years ago

netdoggy commented 2 years ago

Header file /mingw64/include/pdcurses/panel.h refers to curses.h via #include

what does searching in /mingw64/include/curses.h

It seems to me that this is an oversight.

Biswa96 commented 2 years ago

My apology. I can not understand your question. Is there any issue using the package? Can you provide any steps to reproduce the issue?

netdoggy commented 2 years ago

the problem is not critical, but if you use the code:

include <pdcurses/panel.h>

int main() { }

And I try to compile it, I get an error: $ gcc pdcurses_test.c In file included from pdcurses_test.c:1: C:/msys64/mingw64/include/pdcurses/panel.h:10:10: fatal error: curses.h: No such file or directory 10 | #include | ^~~~~~ compilation terminated.

It turns out that I have to manually specify the direct path to the pdcurses folder or change file panel.h to make the file refer to or <pdcurses/curses.h>

netdoggy commented 2 years ago

It may be customary to specify the path to -I/include/libname to gcc when compiling. I don't really know the rules and standards here. If so, sorry) It just seems to me that this is not very logical, so I created this issue.

Biswa96 commented 2 years ago

I think pdcurses.h has to be included instead of individual header files in pdcurses directory.

netdoggy commented 2 years ago

Well, panel.h is a separate component of the pdcurses/ncurses library and it is not included in pdcurses.h by default and the static library for it is located separately.

Biswa96 commented 2 years ago

According to this commit https://github.com/msys2/MINGW-packages/commit/3672e965b9a68277066c9df99145b82c55a7ed83, it was done to resolve the conflict with ncurses. @GitMensch may provide some info about this.

GitMensch commented 2 years ago

There are two options:

1 - let users explicit specify -I /mingw/include/pdcurses - and then just #include - with or withouth the ncurses/ folder prefix 2 - we (should likely) adjust the package to change the sub-headers to #include <pdcurses/curses.h.

What do you think?

Note. Just recognized there was a pending update, so created #12323.

netdoggy commented 2 years ago

I think the second option is more convenient for the end user. At the same time, it will not, in theory, conflict with the first option if you need to use ncurses instead of pdcurses

Biswa96 commented 2 years ago

Is it possible add a proper install mechanism in upstream? The pkgbuild installs everything manually. Instead, a make install command would be better. Also, if cmake can be a possible alternative.

GitMensch commented 2 years ago

I think the second option is more convenient for the end user.

I agree. So someone would have to create a patch for that, adding to the PKGBUILD...

Instead, a make install command would be better.

I totally agree, but as upstream's (PDCursesMod) upstream (PDCurses) dropped any install outside of the configure-built XCurses, those pieces were removed.

You can, of course try and create an issue about that (my full support is sure...).

Also, if cmake can be a possible alternative.

Someone would have to try - there is at least cmake support in PDcursesMod, but neither I nor - as far as I know - the maintainer uses this - it is only supported by the community (which added it in the first place). So please try and if there are missing pieces for installation with cmake add a PR upstream and it is likely to be included.

legends2k commented 1 year ago

@Biswa96 Using PDCurses (mingw-w64-x86_64-pdcurses) on a CMake-based project gives a linker error though:

cmd.exe /C "cd . && C:\apps\msys64\mingw64\bin\c++.exe -g  CMakeFiles/Puyo_exe.dir/src/main.cpp.obj CMakeFiles/Puyo_exe.dir/src/graphics.cpp.obj -o puyo.exe -Wl,--out-implib,libpuyo.dll.a -Wl,--major-image-version,0,--minor-image-version,0  C:/apps/msys64/mingw64/lib/libpdcurses.dll.a  -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 && cd ."
C:/apps/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: CMakeFiles/Puyo_exe.dir/src/graphics.cpp.obj: in function `Graphics::~Graphics()':
F:/work/tryouts/Puyo/src/graphics.cpp:83: undefined reference to `endwin_x64_4302'

Inspecting the library shows

nm /mingw64/lib/libpdcurses.a | grep endwin
0000000000000480 T endwin_u64_4302

mingw32-make-ing original PDCurses sources from upstream has no issues. I only see the symbol endwin with no suffixes on both the sides.

Biswa96 commented 1 year ago

Please provide steps to reproduce the issue.

GitMensch commented 1 year ago

That linker error means that the correct library but the wrong header is used, likely pdcurses/curses.h, but not the installed one "pdcurses.h" https://github.com/msys2/MINGW-packages/blob/031f5a4f8132040dbe4cb63fccb9e03ff08240f2/mingw-w64-pdcurses/PKGBUILD#L116-L120 as that installed one would re-define endwin as it is available.

legends2k commented 1 year ago

@GitMensch Thanks for the tip. Basically PDF_FORCE_UTF8 does the trick. However, two counter points:

  1. PDCurses, is essentially a curses library, most applications wanting a curses library include curses.h
    • This is the header provided by $MINGW_PACKAGE_PREFIX-ncurses too. Portable code will include this and can be linked to either PDCurses or ncurses
    • Special casing for PDCurses in application code seems needless
  2. Building PDCurses from source and using it doesn't require to include pdcurses.h; curses.h just works
    • Declaring PDC_FORCE_UTF8 leads to rendering in a separate window; behaviour without it is nicer
legends2k commented 1 year ago

Please provide steps to reproduce the issue.

@Biswa96 Here you go

cat curtest.cpp

#include <curses.h>
int main() {
  initscr();
  endwin();
}

g++ -I /mingw64/include/pdcurses curtest.cpp -lpdcurses

C:/apps/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\apps\msys64\tmp\ccMQijfa.o:curtest.cpp:(.text+0x17): undefined reference to `endwin_x64_4302'
collect2.exe: error: ld returned 1 exit status
GitMensch commented 1 year ago

Building PDCurses from source and using it doesn't require to include pdcurses.h; curses.h just works

No. Building PDCurses from source without defining any options lead to "just work", but PDCurses is build (and likely ever was) build here with enforced UTF8 encoding so you need the same defines when linking your program, too. The installed pdcurses header removes the need to know which defines are necessary (apart from the DLL build, but then your link would be different, too), if you want to just use curses.h and PDCurses then you need to always take care of the correct defines yourself.

Declaring PDC_FORCE_UTF8 leads to rendering in a separate window; behaviour without it is nicer

Adding this define leads to link correct, this isn't related to the separate window....

I'm not sure what you want to achieve, if you just want to build with curses it would be most reasonable to use ncurses, as this is the MSYS2 default. But you explicit specify "include from pdcurses" and "link to pdcurses" on the command line.

@Biswa96: there would be the option to patch the installed curses.h and have the installed pdcurses.h just forward to that header of course, I've just found the way I added the defines "more clear". Note: not copying but creating curses.h with the appropriate defines is on the TODO-List of PDCursesMod, so this whole issue may be "obsolete" somewhere in the next months, see https://github.com/Bill-Gray/PDCursesMod/issues/237.

If you want to use PDCurses, then you should know about its ports; MSYS2 distributes 2: wingui an the "traditional" wincon, one of its features is to either be in a separate GUI window (with the option to change the font and title from within the code) or to be in a traditional cmd (when used from non-console apps this would open a cmd window, if started from console apps it uses the existing cmd window). The WinGUI port is "full-featured" while the wincon one is limited by cmd's capabilities.

If you want the cmd one in MSYS2 you manually need to copy the "wincon" variant over (or link to the DLL which allows you to place whatever port you want to use as "pdcurses.dll" next to your application).

See the install procedure used here: https://github.com/msys2/MINGW-packages/blob/031f5a4f8132040dbe4cb63fccb9e03ff08240f2/mingw-w64-pdcurses/PKGBUILD#L101-L112

legends2k commented 1 year ago

I'm not sure what you want to achieve

I just want a curses library to use and build against in the MSYS2 ecosystem. Now most curses library come with a curses.h. Why do we special case with the case of PDCurses? Shouldn't PDCurses be a drop-in replacement for ncurses or any curses library for that matter? For instance, I'm able to build my curses-based application using both ncurses and PDCurses (downloaded from upstream). It's only in MSYS2 ecosystem I've to special case and say if __MINGW__ then #include "pdcurses.h which I don't think is the apt behaviour since an app only using the minimum common denominator of curses functions should work with any curses-based library (ncurses, pdcurses, etc.)

if you just want to build with curses it would be most reasonable to use ncurses

I used this first. It has its own issues on MSYS2. While it builds fine, when I run the executable on mintty/MINGW64 I get

Error opening terminal: xterm.

@GitMensch Thanks for the information about wincon and wingui though. I'll be mindful. I think I need wincon. I completely missed that point that PDC_FORCE_UTF8 has nothing to do with the separate GUI window I got. It would be because by default it links to wingui I guess.

GitMensch commented 1 year ago

I used this first. It has its own issues on MSYS2. While it builds fine, when I run the executable on mintty/MINGW64 I get

Error opening terminal: xterm.

TERM="" yourprog

would solve that, but this is something that always feels bad; maybe @Biswa96 @JPeterMugaas @lazka have a cleaner way or coul suggest a patch we may apply to msys2-ncurses.

Why do we special case with the case of PDCurses? Shouldn't PDCurses be a drop-in replacement for ncurses or any curses library for that matter?

Because this way you can also have an application that either uses curses.h or pdcurses.h and also have that checked via the available header (for example in autoconf based software). This way you don't need to explicit set CPPFLAGS for "the correct path" to use "the correct curses.h" - just #include <ncursesh.h> / #include <pdcursesh.h> as wanted and there's no need for a special include setting.

Also note that different curses implementations have different "own" functions (and likely also own bugs you may need to work around).

since an app only using the minimum common denominator of curses functions should work with any curses-based library (ncurses, pdcurses, etc.)

yes, that approach works, just ensure you use the correct set of CPPFLAGS (in this case include directory + defines [for windows this will often be needed when you use a shared library in any case to use the right declspec import attribute) and LDFLAGS (the name of the library, or in the case of MinGW based environments the path to target curses dll).