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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mulle-nat
Copy link
Contributor

…USE_CXX is not set

@mulle-nat
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

confirmed USE_CXX=no results in no headers installed

@mulle-nat
Copy link
Contributor Author

mulle-nat commented Mar 2, 2024

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 😉

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.

2 participants