dankamongmen / notcurses

blingful character graphics/TUI library. definitely not curses.
https://nick-black.com/dankwiki/index.php/Notcurses
Other
3.4k stars 113 forks source link

* remove cpp headers and other c++ support file from installation if … #2765

Open mulle-nat opened 4 months ago

mulle-nat commented 4 months ago

…USE_CXX is not set

mulle-nat commented 4 months ago

I already had a pull request like this, but this one is better. Now pkgconfig and cmake files are also not installed, if there is no c++ support.

dankamongmen commented 4 months ago

I'm not sure I like this idea. What's your motivation here? USE_CXX is all about not requiring a C++ compiler when building Notcurses. Installing the C++ headers doesn't require a C++ compiler for people building against Notcurses who aren't using C++. What's the point of not installing the C++ headers?

I definitely don't think this ought disable pkgconfig/CMake support, independently of your answer to the previous question.

mulle-nat commented 4 months ago

If you don't have a C++ compiler on your system and you can't build the c++ library, why would you want to install the headers or cmake/pkgconfig files supporting that ? You'd rather fail at compile time than link time. I really fail to see your argument here.

Anway, this patch also corrects the C headers like notcurses/notcurses.h not being installed, when -DUSE_CXX=OFF. Here's the diff of two installs with your current github version once with -DUSE_CXX=OFF and once with -DUSE_CXX=ON.

1a2,42
> ├── include
> │   ├── ncpp
> │   │   ├── Cell.hh
> │   │   ├── CellStyle.hh
> │   │   ├── Direct.hh
> │   │   ├── _exceptions.hh
> │   │   ├── FDPlane.hh
> │   │   ├── _flag_enum_operator_helpers.hh
> │   │   ├── _helpers.hh
> │   │   ├── internal
> │   │   │   └── Helpers.hh
> │   │   ├── Menu.hh
> │   │   ├── MultiSelector.hh
> │   │   ├── NCAlign.hh
> │   │   ├── NCBox.hh
> │   │   ├── NCKey.hh
> │   │   ├── NCLogLevel.hh
> │   │   ├── ncpp.hh
> │   │   ├── NotCurses.hh
> │   │   ├── Palette.hh
> │   │   ├── Pile.hh
> │   │   ├── Plane.hh
> │   │   ├── Plot.hh
> │   │   ├── Progbar.hh
> │   │   ├── Reader.hh
> │   │   ├── Reel.hh
> │   │   ├── Root.hh
> │   │   ├── Selector.hh
> │   │   ├── Subproc.hh
> │   │   ├── TabletCallback.hh
> │   │   ├── Tablet.hh
> │   │   ├── Utilities.hh
> │   │   ├── Visual.hh
> │   │   └── Widget.hh
> │   └── notcurses
> │       ├── direct.h
> │       ├── nckeys.h
> │       ├── ncport.h
> │       ├── ncseqs.h
> │       ├── notcurses.h
> │       └── version.h
12a54
> │   ├── libnotcurses++.a
20a63
> │   ├── libnotcurses++.so -> libnotcurses++.so.3
21a65
> │   ├── libnotcurses++.so.3 -> libnotcurses++.so.3.0.9
22a67
> │   ├── libnotcurses++.so.3.0.9
38c83
< 9 directories, 26 files
---
> 13 directories, 67 files
dankamongmen commented 4 months ago

If you don't have a C++ compiler on your system and you can't build the c++ library, why would you want to install the headers or cmake/pkgconfig files supporting that ? You'd rather fail at compile time than link time. I really fail to see your argument here.

we're not saying we don't have a c++ compiler (or won't have one in the future) when we use USE_CXX=OFF. we're only saying we don't want notcurses built with a c++ compiler.

Anway, this patch also corrects the C headers like notcurses/notcurses.h not being installed, when -DUSE_CXX=OFF. Here's the diff of two installs with your current github version once with -DUSE_CXX=OFF and once with -DUSE_CXX=ON.

this is a definite issue, nice catch

dankamongmen commented 4 months ago

confirmed USE_CXX=no results in no headers installed

mulle-nat commented 4 months ago

Currently when USE_CXX is OFF your CMakeLists.txt takes great pains not to compile any C++ specific .cpp files. Therefore it won't produce or install libnotcurses++.a for example (as seen in above diff). Then I come in and say: Installing headers or other support files for a non-existing library is a bug.

Building the notcurses library as C++ (with mangling) wouldn't even work IMO, because you're exporting all your symbols as C with:

#ifdef __cplusplus
extern "C" {

The current CMakeLists.txt is fine, it just needs these fixes :wink: