kou commented on a change in pull request #11067:
URL: https://github.com/apache/arrow/pull/11067#discussion_r726549132
##########
File path: ci/scripts/java_build.sh
##########
@@ -84,6 +85,10 @@ if [ "${ARROW_JAVA_SHADE_FLATBUFFERS}" == "ON" ]; then
${mvn} -Pshade-flatbuffers install
fi
+if [ "${ARROW_JAVA_FFI}" = "ON" ]; then
+ ${mvn} -Darrow.ffi.cpp.build.dir=${ffi_build_dir} -Parrow-ffi install
Review comment:
`cpp` confuses me with C++ implementation.
How about `jni` instead of `cpp` such as `-Darrow.ffi.jni.build.dir`?
##########
File path: java/ffi/CMakeLists.txt
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+#
+# arrow_ffi_java
+#
+
+cmake_minimum_required(VERSION 3.11)
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+project(arrow_ffi_java)
+
+# Find java/jni
+include(FindJava)
+include(UseJava)
+include(FindJNI)
Review comment:
Do we need this?
I think that `find_package(JNI)` read the file.
##########
File path: java/ffi/CMakeLists.txt
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+#
+# arrow_ffi_java
+#
+
+cmake_minimum_required(VERSION 3.11)
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+project(arrow_ffi_java)
+
+# Find java/jni
+include(FindJava)
Review comment:
Do we need this?
I think that `find_package(Java)` read the file.
##########
File path: docker-compose.yml
##########
@@ -1331,6 +1332,7 @@ services:
- ${DOCKER_VOLUME_PREFIX}debian-ccache:/ccache:delegated
command:
/bin/bash -c "
+ /arrow/ci/scripts/java_ffi_build.sh /arrow /build/java/ffi/build
/build/java/ffi &&
/arrow/ci/scripts/cpp_build.sh /arrow /build &&
Review comment:
```suggestion
/arrow/ci/scripts/cpp_build.sh /arrow /build &&
/arrow/ci/scripts/java_ffi_build.sh /arrow /build/java/ffi/build
/build/java/ffi &&
```
##########
File path: java/ffi/pom.xml
##########
@@ -0,0 +1,75 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more
contributor
+ license agreements. See the NOTICE file distributed with this work for
additional
+ information regarding copyright ownership. The ASF licenses this file to
+ You under the Apache License, Version 2.0 (the "License"); you may not use
+ this file except in compliance with the License. You may obtain a copy of
+ the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required
+ by applicable law or agreed to in writing, software distributed under the
+ License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
+ OF ANY KIND, either express or implied. See the License for the specific
+ language governing permissions and limitations under the License. -->
+<project xmlns="http://maven.apache.org/POM/4.0.0";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ <parent>
+ <artifactId>arrow-java-root</artifactId>
+ <groupId>org.apache.arrow</groupId>
+ <version>6.0.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>arrow-ffi</artifactId>
Review comment:
I also think that `arrow-ffi` may not be good. It seems that FFI is too
generic. If we use FFI, users may think that they can call any functions
implemented in not Java. (General FFI library can do them.)
How about `arrow-c-data` and `org.apache.arrow.c.Data`?
##########
File path: java/ffi/CMakeLists.txt
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+#
+# arrow_ffi_java
+#
+
+cmake_minimum_required(VERSION 3.11)
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+project(arrow_ffi_java)
+
+# Find java/jni
+include(FindJava)
+include(UseJava)
+include(FindJNI)
+
+find_package(Java REQUIRED)
+find_package(JNI REQUIRED)
+
+set(JNI_HEADERS_DIR "${CMAKE_CURRENT_BINARY_DIR}/generated")
+
+include_directories(${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR}
+ ${JNI_INCLUDE_DIRS} ${JNI_HEADERS_DIR})
+
+add_jar(${PROJECT_NAME}
+ src/main/java/org/apache/arrow/ffi/jni/JniLoader.java
+ src/main/java/org/apache/arrow/ffi/jni/JniWrapper.java
+ src/main/java/org/apache/arrow/ffi/jni/PrivateData.java
+ GENERATE_NATIVE_HEADERS
+ arrow_ffi_java-native
+ DESTINATION
+ ${JNI_HEADERS_DIR})
+
+set(SOURCES src/main/cpp/jni_wrapper.cc)
+add_library(arrow_ffi_jni SHARED ${SOURCES})
+target_link_libraries(arrow_ffi_jni ${JAVA_JVM_LIBRARY})
+add_dependencies(arrow_ffi_jni ${PROJECT_NAME})
+
+install(TARGETS arrow_ffi_jni DESTINATION
${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR})
Review comment:
I think that `${CMAKE_INSTALL_PREFIX}/` is needless here:
```suggestion
install(TARGETS arrow_ffi_jni DESTINATION ${CMAKE_INSTALL_LIBDIR})
```
##########
File path: java/ffi/README.md
##########
@@ -0,0 +1,56 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Java FFI (C Data Interface)
+
+## Setup Build Environment
+
+install:
+ - Java 8 or later
+ - Maven 3.3 or later
+ - A C++11-enabled compiler
+ - CMake 3.11 or later
+ - Make or ninja build utilities
+
+## Building JNI wrapper shared library
+
+```
+mkdir -p build
+pushd build
+cmake ..
+make
Review comment:
We can use `cmake --build .` here. It doesn't depend on `make` nor
`ninja`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
