potassco / clingo

🤔 A grounder and solver for logic programs.
https://potassco.org/clingo
MIT License
589 stars 79 forks source link

Building a library that depends on clingo with `DCLASP_BUILD_WITH_THREADS=Off` #489

Closed nuernbergk closed 3 months ago

nuernbergk commented 4 months ago

I am trying to build an external application to wasm that uses clingo as a library (simply using the web target doesn't suffice for my use case, unfortunately). So far, the following setup works: test.c:

#include <libclingo/clingo.h>
#include <stdio.h>

int main()
{
  int major, minor, revision;
  clingo_version(&major, &minor, &revision);
  printf("Version is %d.%d.%d\n", major, minor, revision);
  return 0;
}

CMakeLists.txt

cmake_minimum_required(VERSION 3.1)

project(hello_world)

add_subdirectory(clingo)

add_executable(myapp test.c)
target_include_directories(myapp PRIVATE
    "clingo/")

target_link_libraries(myapp libclingo libgringo libclasp)

where the clingo source files are provided in a subdirectory.

I can build this using

emcmake cmake \
        -DCLINGO_BUILD_WITH_PYTHON=Off \
        -DCLINGO_BUILD_WITH_LUA=Off \
        -DCLINGO_REQUIRE_LUA=Off \
        -DCLINGO_BUILD_SHARED=Off \
        -DCMAKE_VERBOSE_MAKEFILE=On \
        -DCMAKE_BUILD_TYPE=release \
        -DCMAKE_CXX_FLAGS="-s ALLOW_MEMORY_GROWTH=1 -s MODULARIZE=1" \
        .

emmake make -C . myapp -j 8

This compiles and does what it should do. Actually solving ASPs works as well. This surprises me, because the repository clingo-wasm makes sure to use the DCLASP_BUILD_WITH_THREADS=Off option. However, when passing this option, I get linker errors:

In file included from /Users/nn/clingo-test/clingo/clasp/libpotassco/src/convert.cpp:25:
In file included from /Users/nn/clingo-test/clingo/clasp/libpotassco/potassco/string_convert.h:27:
In file included from /opt/homebrew/Cellar/emscripten/3.1.55/libexec/cache/sysroot/include/c++/v1/string:568:
In file included from /opt/homebrew/Cellar/emscripten/3.1.55/libexec/cache/sysroot/include/c++/v1/__algorithm/min.h:14:
/opt/homebrew/Cellar/emscripten/3.1.55/libexec/cache/sysroot/include/c++/v1/__algorithm/min_element.h:57:61: error: too many arguments provided to function-like macro invocation
   57 |   static_assert(__is_callable<_Compare, decltype(*__first), decltype(*__first)>::value,
      |                                                             ^
/Users/nn/schulverwalter/clingo-test/clingo/clasp/libpotassco/potassco/platform.h:103:9: note: macro 'static_assert' defined here
  103 | #define static_assert(x, message) typedef bool POTASSCO_CONCAT(potassco_static_assertion,__LINE__)[sizeof(static_assertion< (x) >)] POTASSCO_ATTR_UNUSED
      |         ^
In file included from /Users/nn/clingo-test/clingo/clasp/libpotassco/src/convert.cpp:25:
In file included from /Users/nn/clingo-test/clingo/clasp/libpotassco/potassco/string_convert.h:27:
In file included from /opt/homebrew/Cellar/emscripten/3.1.55/libexec/cache/sysroot/include/c++/v1/string:568:
In file included from /opt/homebrew/Cellar/emscripten/3.1.55/libexec/cache/sysroot/include/c++/v1/__algorithm/min.h:14:
/opt/homebrew/Cellar/emscripten/3.1.55/libexec/cache/sysroot/include/c++/v1/__algorithm/min_element.h:57:3: error: use of undeclared identifier 'static_assert'; did you mean 'static_assertion'?
   57 |   static_assert(__is_callable<_Compare, decltype(*__first), decltype(*__first)>::value,
      |   ^

and a bunch more of this sort. In fact, even compiling to native code with this flag using the normal cmake,make process gives me very similar errors.

Building clingo itself (and its web target) with this flag works fine. It only happens in the case where I try to use clingo as a dependency from somewhere else. Maybe this is an issue with my CMakeLists.txt.

Now, I don't know how much of a problem this actually is. Maybe building clasp with threading support is fine, although this would surprise me, since wasm threading isn't quite there yet.

rkaminsk commented 4 months ago

Enabling threads also enables C++11 in clasp and static_assert comes along with it. It seems like there is some cmake configuration that is wrong. I would recommend to build without threads and enable C++14 or higher project wide.

nuernbergk commented 4 months ago

Wouldn't that mean I should get this issue only when building with threads? It seems odd that it happens when I try to build without threads. I tried to set C++14 project wide, but it doesn't help. In any case, this shouldn't be the problem, since the clingo CMakeLists.txt requires the C++14 standard as well.

rkaminsk commented 4 months ago

This message shows that the static_assert of libpotassco is used somewhere within the STL:

min_element.h:57:61: error: too many arguments provided to function-like macro invocation

It should not happen if everything is compiled with C++14. I cannot really tell you more. I would compile in verbose mode and have a look at the compiler commands to check if compiler options are used consistently.

nuernbergk commented 4 months ago

Thank you so much. Indeed, the problem is in the CMakeLists.txt for Clasp, lines L98-L100, where the standard is set to 98. Simply changing this to be 11 works.

Is this desired behaviour? It seems like it would make more sense to not set the standard at all in the case that threads aren't used. For my use case, it is of course fine to simply patch this file, but this does seem like an odd design choice.

rkaminsk commented 4 months ago

Thank you so much. Indeed, the problem is in the CMakeLists.txt for Clasp, lines L98-L100, where the standard is set to 98. Simply changing this to be 11 works.

Is this desired behaviour? It seems like it would make more sense to not set the standard at all in the case that threads aren't used. For my use case, it is of course fine to simply patch this file, but this does seem like an odd design choice.

Clasp is written in C++98. Setting the standard to C++98 is the right thing to do.

@BenKaufmann I think there is a problem how libc++ uses static_assert. Maybe we should patch this in libpotassco by simply using another name? We could also add an option to not set the standard at all and use whatever is the default or configured by projects like clingo using clasp.

BenKaufmann commented 4 months ago

I think there is a problem how libc++ uses static_assert. Maybe we should patch this in libpotassco by simply using another name? We could also add an option to not set the standard at all and use whatever is the default or configured by projects like clingo using clasp.

@rkaminsk I'd be fine with either option. However, I'd first like to understand the underlying problem better - i.e. why is the detection code in libpotassco's platform.h not working and/or written like it is?

From my current understanding, I'd say that our check in platform.h is broken and we should either just drop the && !defined(_LIBCPP_VERSION) part completely or at least remove the !.

BenKaufmann commented 4 months ago

@nuernbergk Could you maybe check whether compiling with DCLASP_BUILD_WITH_THREADS=Off works if you change the defined(static_assert) && !defined(_LIBCPP_VERSION) check in line 31 of potassco/platform.h to defined(static_assert) && defined(_LIBCPP_VERSION) (i.e. just drop the !)?

nuernbergk commented 4 months ago

@BenKaufmann Yep, that fixes it.

rkaminsk commented 3 months ago

@nuernbergk this is to let you know that @BenKaufmann added a second patch that addresses the problem at the root. With this it should be possible to build clasp using C++98 and still use it in code requiring later standards.

domoritz commented 3 months ago

I suspect this also causes my build issues in https://github.com/domoritz/clingo-wasm/pull/348. Do you plan to release a new version that can compile without threads (which is what makes sense for WASM at the moment)?