Skip to content

Commit

Permalink
Use C++11 thread local and Windows build update (#41)
Browse files Browse the repository at this point in the history
This PR fixes #32 and 75% of #40 

* Use standard thread_local since we expect todays compiler to support that standard feature.
* Fix some issues in the build system for python
* Add some documentation about the current state on Window
  • Loading branch information
a4z authored Jul 5, 2021
1 parent 755d165 commit 16d09e7
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 162 deletions.
23 changes: 21 additions & 2 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: build-all-configs
on: [push, pull_request]
jobs:
build-on-osx-for-osx:
build-on-osx-for-objectiveC:
runs-on: macos-10.15
steps:
- uses: actions/checkout@v2
Expand All @@ -26,7 +26,7 @@ jobs:
runs-on: macos-10.15
strategy:
matrix:
python-version: [3.6, 3.7, 3.8, 3.9]
python-version: [3.6, 3.9] # oldest and newest, rest assumed to work
steps:
- uses: actions/checkout@v2
- uses: asdf-vm/actions/install@v1
Expand Down Expand Up @@ -67,6 +67,25 @@ jobs:
working-directory: build/check_install_root
run: diff -u ../../test/jni_list.txt <(du -a | tac | awk -F ' ' '{print $2}' | sort)


build-on-ubuntu-for-python:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: asdf-vm/actions/install@v1
- name: Install dependencies
run: |
python3 -m pip install --upgrade pip
if [ -f requirements.txt ]; then pip3 install -r requirements.txt; fi
- name: Configure cmake
run: cmake -S . -B build -DDJINNI_WITH_PYTHON=ON
- name: Build release
run: cmake --build build --parallel $(nproc) --config Release
- name: Run tests
working-directory: build/test-suite
run: ctest -C Release -V


build-on-windows-for-cppcli:
runs-on: windows-latest
steps:
Expand Down
4 changes: 2 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ set(SRC_OBJC
)

set(SRC_C_WRAPPER
"djinni/cwrapper/thread_local.cpp"
"djinni/cwrapper/thread_local.hpp"
"djinni/cwrapper/wrapper_marshal.cpp"
"djinni/cwrapper/wrapper_marshal.h"
"djinni/cwrapper/wrapper_marshal.hpp"
Expand Down Expand Up @@ -136,6 +134,8 @@ endif()
# Python support
option(DJINNI_WITH_PYTHON "Include the Python support code in Djinni support library." OFF)
if(DJINNI_WITH_PYTHON)
set_target_properties(djinni_support_lib PROPERTIES
POSITION_INDEPENDENT_CODE ON)
target_include_directories(djinni_support_lib PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/djinni/cwrapper/>")
target_sources(djinni_support_lib PRIVATE ${SRC_C_WRAPPER})
source_group("cwrapper" FILES ${SRC_C_WRAPPER})
Expand Down
58 changes: 0 additions & 58 deletions djinni/cwrapper/thread_local.cpp

This file was deleted.

83 changes: 0 additions & 83 deletions djinni/cwrapper/thread_local.hpp

This file was deleted.

15 changes: 8 additions & 7 deletions djinni/cwrapper/wrapper_marshal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@

#include <stdio.h> // for debugging
#include "wrapper_marshal.hpp"
#include "thread_local.hpp"

#include "../djinni_common.hpp"
#include <string>
#include <iostream> // for debugging
#include <thread>

using namespace djinni;
using namespace djinni::optionals;
Expand Down Expand Up @@ -57,11 +58,11 @@ struct ExceptionState {
};

// Current exception state
static djinni::support_lib::ThreadLocal<ExceptionState> s_exception_state;
static thread_local ExceptionState s_exception_state;

// to be called from Python
void djinni_create_and_set_cpp_from_py_exception(DjinniPythonExceptionHandle * py_e_handle) {
s_exception_state.get().handle = ExceptionState::newHandle(py_e_handle);
s_exception_state.handle = ExceptionState::newHandle(py_e_handle);
}

void _djinni_add_callback_create_py_from_cpp_exception(CreateExceptionFnPtr fct_ptr) {
Expand All @@ -82,7 +83,7 @@ void djinni::cw_throw_exception(djinni::Handle<DjinniPythonExceptionHandle> e_ha

// to be called from cpp to allow cpp impl to know about pyhton exception
void djinni::cw_throw_if_pending() {
auto e_handle = s_exception_state.get().takeException();
auto e_handle = s_exception_state.takeException();

if (e_handle) {
cw_throw_exception(std::move(e_handle));
Expand Down Expand Up @@ -115,12 +116,12 @@ djinni::Handle<DjinniPythonExceptionHandle> djinni::cw_get_py_exception(const st
// notice an exception was thrown
// The argument is an exception_ptr for either a std::exception or a subclass of std::exception called py_exception
void djinni::cw_set_pending_exception(const std::exception_ptr & e_ptr) {
s_exception_state.get().handle = cw_get_py_exception(e_ptr);
s_exception_state.handle = cw_get_py_exception(e_ptr);
}

DjinniPythonExceptionHandle * djinni_from_python_check_and_clear_exception() {
if (s_exception_state.get().handle) {
return s_exception_state.get().takeException().release();
if (s_exception_state.handle) {
return s_exception_state.takeException().release();
}
return nullptr;
}
Expand Down
19 changes: 19 additions & 0 deletions docs/python-support.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,22 @@ the other side.
* The support library code makes direct reference to <optional> types rather than respecting the
command-line choice of a different optional library.


## Python support on Windows not complete yet

If you want help development of that feature, please download the current generator for Windows and place it in the root folder.

Generate the cmake project:

```
cmake -S . -B build/py -DDJINNI_WITH_PYTHON=ON -DDJINNI_EXECUTABLE="$(((Get-Location).Path) -replace "\\","/")/djinni.bat" -G "Visual Studio 16 2019"
```


Run the build

```
cmake --build build/py
```

Explore the error. Help will be welcome!
22 changes: 12 additions & 10 deletions test-suite/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ add_djinni_target(DjinniAllTests
CPP_OUT "${CMAKE_CURRENT_BINARY_DIR}/generated-src/cpp"
CPP_NAMESPACE "testsuite"
IDENT_CPP_ENUM_TYPE "foo_bar"
CPP_OPTIONAL_TEMPLATE "std::optional"
CPP_OPTIONAL_HEADER "\"optional\""
CPP_EXTENDED_RECORD_INCLUDE_PREFIX "test-suite/handwritten-src/cpp/"
CPP_ENUM_HASH_WORKAROUND
JAVA_OUT "${CMAKE_CURRENT_BINARY_DIR}/generated-src/java"
Expand Down Expand Up @@ -54,8 +52,6 @@ add_djinni_target(DjinniWCharTests
CPP_OUT "${CMAKE_CURRENT_BINARY_DIR}/generated-src/cpp"
CPP_NAMESPACE "testsuite"
IDENT_CPP_ENUM_TYPE "foo_bar"
CPP_OPTIONAL_TEMPLATE "std::optional"
CPP_OPTIONAL_HEADER "\"optional\""
CPP_EXTENDED_RECORD_INCLUDE_PREFIX "test-suite/handwritten-src/cpp/"
CPP_ENUM_HASH_WORKAROUND
CPP_USE_WIDE_STRINGS
Expand Down Expand Up @@ -98,9 +94,6 @@ add_djinni_target(DjinniPyTests
CPP_OUT "${CMAKE_CURRENT_BINARY_DIR}/generated-src/pycpp"
CPP_NAMESPACE "testsuite"
IDENT_CPP_ENUM_TYPE "foo_bar"
CPP_OPTIONAL_TEMPLATE "std::optional"
CPP_OPTIONAL_HEADER "\"optional\""

PY_OUT_FILES PYTHON_GENERATED_SRCS
PYCFFI_OUT_FILES PYCFFI_GENERATED_SRCS
C_WRAPPER_OUT_FILES C_WRAPPER_GENERATED_SRCS
Expand Down Expand Up @@ -236,23 +229,32 @@ if(DJINNI_WITH_PYTHON)

# Prepare Python 3 environment
find_package(PythonInterp "3" REQUIRED)
string(JOIN ":" PYTHONPATH
if (WIN32)
message(WARNING "Python support on Windwos is not fully implemented, for more details visit https://github.com/cross-language-cpp/djinni-support-lib/issues")
string(JOIN $<SEMICOLON> PYTHONPATH
"${CMAKE_SOURCE_DIR}/djinni/py"
"${CMAKE_CURRENT_BINARY_DIR}/generated-src/python"
$ENV{PYTHONPATH})

else()
string(JOIN ":" PYTHONPATH
"${CMAKE_SOURCE_DIR}/djinni/py"
"${CMAKE_CURRENT_BINARY_DIR}/generated-src/python"
$ENV{PYTHONPATH})
endif()

# Compile Python extension module (CFFI)
set(C_WRAPPER_GENERATED_HEADERS ${C_WRAPPER_GENERATED_SRCS})
list(FILTER C_WRAPPER_GENERATED_HEADERS INCLUDE REGEX "\\.h$")
add_custom_target(PyCFFIlib_cffi ALL
${CMAKE_COMMAND} -E env PYTHONPATH=${PYTHONPATH} ${PYTHON_EXECUTABLE}
${CMAKE_COMMAND} -E env PYTHONPATH=${PYTHONPATH} LIBRARY_PATH=$<TARGET_FILE_DIR:mylib> ${PYTHON_EXECUTABLE}
${PYCFFI_GENERATED_SRCS}
${CMAKE_SOURCE_DIR}/djinni/cwrapper/wrapper_marshal.h
${C_WRAPPER_GENERATED_HEADERS}
${CMAKE_CURRENT_SOURCE_DIR}/handwritten-src/cwrapper/limits_helper.h
DEPENDS mylib ${PYCFFI_GENERATED_SRCS} ${PYTHON_GENERATED_SRCS} ${C_WRAPPER_GENERATED_HEADERS}
COMMENT "Building CFFI lib"
VERBATIM
WORKING_DIRECTORY $<TARGET_FILE_DIR:mylib>
)

add_test(NAME PythonTests COMMAND ${CMAKE_COMMAND} -E env PYTHONPATH=${PYTHONPATH}
Expand Down

0 comments on commit 16d09e7

Please sign in to comment.