Stellarium / stellarium

Stellarium is a free GPL software which renders realistic skies in real time with OpenGL. It is available for Linux/Unix, Windows and macOS. With Stellarium, you really see what you can see with your eyes, binoculars or a small telescope.
https://stellarium.org
GNU General Public License v2.0
7.62k stars 818 forks source link

Compilation error: strnlen() #3848

Closed fancsali closed 1 week ago

fancsali commented 1 month ago

Downloaded source package fails to build on ARMv7 and/or pmOS/Alipine with musl libc.

System

Build steps

Trying to build with following commands:

cmake -B build -G Ninja \
    -DCMAKE_BUILD_TYPE=None \
    -DCMAKE_INSTALL_PREFIX=/usr \
    -DENABLE_TESTING=1 \
    -DENABLE_SHOWMYSKY=OFF

cmake --build build

Logfile

Build emits following error:

/[...]/_deps/nlopt-src/src/api/options.c:273:11: error: implicit declaration of function 'strnlen'; did you mean 'strlen'? [-Wimplicit-function-declaration]
  273 |     len = strnlen(name, 1024) + 1;
      |           ^~~~~~~
      |           strlen
ninja: subcommand failed

Context & Potential fix/workaround

Creating a test file:

#include <stdio.h>
#include <string.h>

char *s = "SPAM";

int main() {

    int len = strnlen(s, 100);
    printf("%d\n", len);
}

and trying to compile it with gcc -std=c11 test.c -o test reports the same error.

However, looking at the musl header source code reveals, that defining _POSIX-SOURCE is the way to go. And indeed, compiling with gcc -std=c11 -D_POSIX_SOURCE test.c -o test succeeds, and yields a correctly working binary.

10110111 commented 1 month ago

Does this reproduce if you try building nlopt separately? If yes, this should be reported to that project instead.

fancsali commented 1 month ago

Fair point, one that I've missed before.

I've just had a check though, and unfortunately(?) it seems to be working...

gzotti commented 1 month ago

-DCMAKE_BUILD_TYPE=None ?

Try Release

fancsali commented 1 month ago

I understand your confusion, seemed to be rather odd to me too, when I looked into the packaging script of my distro.

But figured, there must be a reason behind that.

Either way, I tried with -DCMAKE_BUILD_TYPE=Release and it makes no difference, unfortunately.

alex-w commented 1 month ago

I've just had a check though, and unfortunately(?) it seems to be working...

What about versions for nlopt within stellarium and in “standard” package?

10110111 commented 1 month ago

I've just had a check though, and unfortunately(?) it seems to be working...

I've just tried building nlopt, both 2.7.1 and current master, and I don't see any mention of POSIX string (I'm building on stock x86_64 gcc version 9.4.0-1ubuntu1~20.04.2). Does the _POSIX_SOURCE definition appear anywhere in your compilation commands (use --verbose option for the cmake build command)?

alex-w commented 1 month ago

@10110111 difference in standard library

10110111 commented 1 month ago

difference in standard library

There should be no difference regardless of whether nlopt is compiled standalone or via CPM. And there appears to exist a difference for the OP. I presume this is because of the presence of the _POSIX_SOURCE definition, but I wonder why it could appear in the standalone build but not appear in the CPM build.

satmandu commented 1 month ago

I'm seeing the same issue. Installing v2.8.0 of nlopt first alleviates the problem.

10110111 commented 1 month ago

@satmandu Does changing the version requirements in plugins/LensDistortionEstimator/src/CMakeLists.txt (without the external package installed) fix the issue?

satmandu commented 1 month ago

That does not resolve the issue.

/usr/local/tmp/crew/stellarium.20240822121513.dir/builddir/_deps/nlopt-src/src/api/options.c: In function ‘nlopt_set_param’:
/usr/local/tmp/crew/stellarium.20240822121513.dir/builddir/_deps/nlopt-src/src/api/options.c:273:11: error: implicit declaration of function ‘strnlen’; did you mean ‘strlen’? [-Wimplicit-function-declaration]
  273 |     len = strnlen(name, 1024) + 1;
      |           ^~~~~~~
      |           strlen
[3*5/996] Building C object _deps/nlopt-build/CMakeFiles/nlopt.dir/src/util/mt19937ar.c.o
gzotti commented 1 month ago

Missing a header? https://man7.org/linux/man-pages/man3/strnlen.3.html

10110111 commented 1 month ago

Missing a header?

The header is the same, <string.h>, and yet the compiler sees strlen but not strnlen. Apparently because of the lack of _POSIX_SOURCE definition, but I don't get why the compilation works standalone, since the sources never mention this symbol.

10110111 commented 1 month ago

@satmandu Do you, by any chance, have -Werror=implicit-function-declaration (or simply -Werror) enabled externally? I guess, in this case the compilation of standalone nlopt will result in the same warning/error, but, if the above mentioned option is disabled, it may not stop compilation.

satmandu commented 1 month ago

I am using GCC 14, which I believe is more strict about these things...

10110111 commented 1 month ago

I am using GCC 14, which I believe is more strict about these things...

Hmm, indeed you're right. So I suppose, if you try building nlopt manually, you'll run into the same problem. If so, you should report this to nlopt project. Meanwhile, your workaround would be to add -DCMAKE_C_FLAGS=-Wno-error=implicit-function-declaration to the cmake command line.

satmandu commented 1 month ago

I am using GCC 14, which I believe is more strict about these things...

Hmm, indeed you're right. So I suppose, if you try building nlopt manually, you'll run into the same problem. If so, you should report this to nlopt project. Meanwhile, your workaround would be to add -DCMAKE_C_FLAGS=-Wno-error=implicit-function-declaration to the cmake command line.

Thanks for that. I didn't have to add any special options to nlopt 2.8.0 to get it to build.

Maybe this flag should be added to the stellarium build settings if GCC >= 14?

10110111 commented 1 month ago

Maybe this flag should be added to the stellarium build settings if GCC >= 14?

It would be a kludge, which I wouldn't like to add, especially given that this error is generally useful.

The proper solution would be to find out how nlopt gets built separately, particularly whether _POSIX_SOURCE is defined in that file that you get error from. If it is, then I wonder where it gets defined, because the sources don't seem to mention it.

satmandu commented 1 month ago

Does the compile succeed on Linux when you build with GCC 14?

10110111 commented 1 month ago

Does the compile succeed on Linux when you build with GCC 14?

The problem is that 1) I only have libstdc++, and 2) _POSIX_SOURCE is defined by default on the machine I checked.

I don't have GCC 14 on my local machine, but given the above I expect that it would still build nlopt, both separately and from Stellarium (where the definition is also present).

Could you check that the definition is present in nlopt's src/api/options.c when you build nlopt separately?

satmandu commented 1 month ago

I do not see anything here:

https://github.com/stevengj/nlopt/blob/v2.8.0/src/api/options.c

The relevant C Flags I am building both stellarium and nlopt with are:

-DCMAKE_C_FLAGS='-O2 -pipe -ffat-lto-objects -fPIC -flto=auto'

For what it is worth, this has the options that nlopt uses for build testing:

https://github.com/stevengj/nlopt/blob/master/.github/workflows/build.yml

10110111 commented 1 month ago

The relevant C Flags I am building both stellarium and nlopt with

The question is not what you supply to the build system, but rather what the file being compiled gets. If the compiler somehow passes the necessary define, it will also result in a success. But we first need to figure out whether this is indeed true. You can check this by an #ifdef inside the C file that has this strnlen call (see how I did with Godbolt's Compiler Explorer linked to above, replace #warning with #error to make sure you'll not miss it).

satmandu commented 1 month ago

Compiling that code from the compiler explorer on my system:

test.c:3:2: warning: #warning the definition is present [-Wcpp]                 
    3 | #warning the definition is present                                      
      |  ^~~~~~~

I can patch the nlopt source easily at build time when building it standalone... do you have an easy way to do that with the cmake CPMFindPackage function in plugins/LensDistortionEstimator/src/CMakeLists.txt where it pulls in the tar?

diff -Npaur a/src/api/options.c b/src/api/options.c
--- a/src/api/options.c 2024-08-22 12:00:01.733064449 -0400
+++ b/src/api/options.c 2024-08-22 12:00:30.804740586 -0400
@@ -24,6 +24,9 @@
 #include <stdlib.h>
 #include <math.h>
 #include <string.h>
+#ifdef _POSIX_SOURCE
+#warning the definition is present
+#endif
 #include <float.h>
 #include <stdarg.h>
satmandu commented 1 month ago

FYI for 2.8.0 I have this change in plugins/LensDistortionEstimator/src/CMakeLists.txt

CPMFindPackage(NAME NLopt
               URL https://github.com/stevengj/nlopt/archive/refs/tags/v2.8.0.tar.gz
               URL_HASH SHA256=e02a4956a69d323775d79fdaec7ba7a23ed912c7d45e439bc933d991ea3193fd
               EXCLUDE_FROM_ALL yes
              )
10110111 commented 1 month ago

do you have an easy way to do that with the cmake CPMFindPackage

I'd try editing the file under _deps/nlopt-src inside the build directory. I don't think it would be overwritten on the rebuild of Stellarium.

Anyway, it's obvious that the definition is lacking. And actually, I now realize what the problem may be. Does it work if you apply this?

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 53dd835c11..f2dc19af64 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -306,7 +306,6 @@ SET(CMAKE_CXX_EXTENSIONS OFF)
 # Ubuntu 18.04 have GCC 7.5 - so, C11 only
 SET(CMAKE_C_STANDARD 11)
 SET(CMAKE_C_STANDARD_REQUIRED ON)
-SET(CMAKE_C_EXTENSIONS OFF)

 IF(WIN32)
      # We don't need the extra Windows.h stuff, this may speed up compilation a tiny bit
satmandu commented 1 month ago

That fixes the build.

10110111 commented 1 month ago

@alex-w Is there any serious reason for this setting? It was added in 6169a3880a11f48b5d67be74e296a8a940bb012d.

alex-w commented 1 month ago

@alex-w Is there any serious reason for this setting? It was added in 6169a38.

I don’t remember details, but we catch some puzzling for some compilers when extensions are enabled

10110111 commented 3 weeks ago

Ideally, for simplicity, I'd remove that line, adding a comment explaining why we want the extensions to be enabled. If we can't, @fancsali please check if the following works:

diff --git a/plugins/LensDistortionEstimator/src/CMakeLists.txt b/plugins/LensDistortionEstimator/src/CMakeLists.txt
index ce4f21bc5e..52f4aeec0c 100644
--- a/plugins/LensDistortionEstimator/src/CMakeLists.txt
+++ b/plugins/LensDistortionEstimator/src/CMakeLists.txt
@@ -54,6 +54,7 @@ endif()

 TARGET_COMPILE_OPTIONS(LensDistortionEstimator-static PRIVATE "-DHAVE_NLOPT")
 IF(NLopt_ADDED)
+    SET_TARGET_PROPERTIES(nlopt PROPERTIES C_EXTENSIONS ON)
     ADD_DEPENDENCIES(LensDistortionEstimator-static generate-cpp)
     TARGET_INCLUDE_DIRECTORIES(LensDistortionEstimator-static
                                PRIVATE ${CPM_PACKAGE_NLopt_BINARY_DIR} ${CPM_PACKAGE_NLopt_SOURCE_DIR}/src/api)
alex-w commented 2 weeks ago

@fancsali any news?