cburstedde / libsc

The "sc" auxiliary library
www.p4est.org/
GNU Lesser General Public License v2.1
35 stars 34 forks source link

`make install` installs getopt.h (twice) #105

Closed StefanBruens closed 1 year ago

StefanBruens commented 1 year ago

Description

In case getopt.h (and getopt_int.h) is provided by the system, the header files should not be installed.

Actually, it is installed twice, once under <prefix>/include/, once under <prefix>/include/sc_builtin.

scivision commented 1 year ago

Is this as configured via Autotools vs. CMake?

cburstedde commented 1 year ago

Automake installs the following files:

include/sc_getopt.h
include/sc_builtin/getopt_int.h
include/sc_builtin/getopt.h

I'd be hoping the sc_builtin path, which is not needed for outside use, does not get in the way. Comments?

StefanBruens commented 1 year ago

This is a CMake build.

The problematic lines are here: https://github.com/cburstedde/libsc/blob/1d8ab1397cec48c71f86eaca726e1d56a2bc49a7/CMakeLists.txt#L60-L62

cburstedde commented 1 year ago

Thanks! I'd generally encourage to have the CMake install to look as much as possible like the autotools install.

cburstedde commented 1 year ago

I have an inkling this would be a breeze for @pkestene .

cburstedde commented 1 year ago

I have an inkling this would be a breeze for @pkestene .

Indeed I have merged a suitable PR. Does everything work for y'all?

cburstedde commented 1 year ago

Thanks; merged. Everything checks out?

StefanBruens commented 1 year ago

As far as I can see, this only fixes the "twice" part.

It still installs getopt.h, i.e. typically a system header file.

Actually, I can not see a reason to install getopt.h even if the system does not provide getopt.h. AFAICS none of SCs headers include getopt.h (directly or via sc_getopt.h). It is only include by sc_options.c (but not sc_options.h).

cburstedde commented 1 year ago

Good points; reopening.

cburstedde commented 1 year ago

Since @pkestene is on rampage, copying. :)

pkestene commented 1 year ago

What PR #126 did is just ensuring that autotools and cmake build actually align.

Two remarks :

and a question: maybe we can remove from installation procedure both sc_builtin/getopt.h and sc_getopt.h; do you agree ?

cburstedde commented 1 year ago

As long as these headers are installed under the sc_builtin subdirectory, which is the case with autoconf, I do not see this to be problematic. Of course it would be nicer to entirely rename these files, which I may look into.

For now, is CMake doing the same, installing them under a sc_buildint subdirectory?

StefanBruens commented 1 year ago

@cburstedde But why are these headers installed at all, if the files are completely unused?

Users may (errneously) add sc_builtin to their include dirs, and then these files suddenly become problematic.

cburstedde commented 1 year ago

@cburstedde But why are these headers installed at all, if the files are completely unused?

Users may (errneously) add sc_builtin to their include dirs, and then these files suddenly become problematic.

Yes, you are right. We might just make them no-install files. Let me look at it.

cburstedde commented 1 year ago

This has been done for autoconf in #131. Comments?

Can somebody please check that CMake does it the same way, i.e., depends on src/sc_builtin/sc_getopt.h for compiling, but not installing anything under src/sc_bulitin? These files should still be included by cpack.

cburstedde commented 1 year ago

Ok there was an update to address that. Any remaining issues @StefanBruens ?

cburstedde commented 1 year ago

Bump. Will merge soon if not commented.