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

deps: new dep "proj" fails to compile #49749

Closed
knz opened this issue Jun 1, 2020 · 6 comments · Fixed by #49755
Closed

deps: new dep "proj" fails to compile #49749

knz opened this issue Jun 1, 2020 · 6 comments · Fixed by #49755
Assignees
Labels
A-build-system A-spatial Spatial work that is *not* related to builtins. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@knz
Copy link
Contributor

knz commented Jun 1, 2020

/data/home/kena/src/go/src/github.com/cockroachdb/cockroach/c-deps/proj/src/pj_mutex.c:130:48: error: use of undeclared identifier 'PTHREAD_MUTEX_RECURSIVE_NP'; did you mean 'PTHREAD_MUTEX_RECURSIVE'?
        pthread_mutexattr_settype(&mutex_attr, PTHREAD_MUTEX_RECURSIVE_NP);
                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
                                               PTHREAD_MUTEX_RECURSIVE
/usr/include/pthread.h:132:2: note: 'PTHREAD_MUTEX_RECURSIVE' declared here
        PTHREAD_MUTEX_RECURSIVE         = 2,    /* Recursive mutex */
        ^

nb: the _NP variants are obsolete as far as I can see

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-build-system A-spatial Spatial work that is *not* related to builtins. labels Jun 1, 2020
@knz knz assigned otan Jun 1, 2020
@knz
Copy link
Contributor Author

knz commented Jun 1, 2020

I'm confused because there's a conditional just before

#ifdef HAVE_PTHREAD_MUTEX_RECURSIVE

and my system most definitely has PTHREAD_MUTEX_RECURSIVE

Is the build perhaps forgetting to run the configure script?

@knz
Copy link
Contributor Author

knz commented Jun 1, 2020

ok I found what's amiss

on my system, PTHREAD_MUTEX_RECURSIVE is an enum tag,

and CMake mistakenly tries to identify its availability using the following C code:

int main(int argc, char** argv)
{
  (void)argv;
#ifndef PTHREAD_MUTEX_RECURSIVE
  return ((int*)(&PTHREAD_MUTEX_RECURSIVE))[argc];
#else
  (void)argc;
  return 0;
#endif 
}

ie it assumes that something is either a macro or an lvalue, and it doesn't consider enum tags.

This is a bug in the CMake implementation, or perhaps even the CMakefile itself.

For comparison, here's how RocksDB does it:

CHECK_CXX_SOURCE_COMPILES("
#include <pthread.h>
int main() {
  (void) PTHREAD_MUTEX_ADAPTIVE_NP;
}
" HAVE_PTHREAD_MUTEX_ADAPTIVE_NP)
if(HAVE_PTHREAD_MUTEX_ADAPTIVE_NP)
  add_definitions(-DROCKSDB_PTHREAD_ADAPTIVE_MUTEX)
endif()

which is much better

@knz
Copy link
Contributor Author

knz commented Jun 1, 2020

I could send a PR to manually edit CMakeLists.txt with the fix, however how do I guarantee that the fix remains if we ever bump the dependency?

@knz
Copy link
Contributor Author

knz commented Jun 1, 2020

(Instructions as to where's the upstream bug tracker so I can file the upstream issue are welcome)

@knz
Copy link
Contributor Author

knz commented Jun 1, 2020

For reference the following diff fixes it:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index cde21f0c..5c7e7b51 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -60,9 +60,15 @@ find_package (Threads)

 include(CheckIncludeFiles)
 include(CheckSymbolExists)
-CHECK_SYMBOL_EXISTS(PTHREAD_MUTEX_RECURSIVE pthread.h HAVE_PTHREAD_MUTEX_RECURSIVE_DEFN)
-if (HAVE_PTHREAD_MUTEX_RECURSIVE_DEFN)
-    add_definitions(-DHAVE_PTHREAD_MUTEX_RECURSIVE=1)
+
+CHECK_C_SOURCE_COMPILES("
+#include <pthread.h>
+int main() {
+  (void) PTHREAD_MUTEX_RECURSIVE;
+}
+" HAVE_PTHREAD_MUTEX_RECURSIVE)
+if(HAVE_PTHREAD_MUTEX_RECURSIVE)
+  add_definitions(-DHAVE_PTHREAD_MUTEX_RECURSIVE=1)
 endif()

 boost_report_value(PROJ_PLATFORM_NAME)

@otan
Copy link
Contributor

otan commented Jun 1, 2020

we're pinned to an older version of PROJ (tracker here: https://github.com/OSGeo/PROJ) at 4.9.3, so not sure if they'll care. you can file a bug there.

we'll probably have to fork this to make this work for you. i'll set that up shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-system A-spatial Spatial work that is *not* related to builtins. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants