-
Notifications
You must be signed in to change notification settings - Fork 29
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
GH-14: Add JNI CI test #441
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lidavidm ! LGTM a couple of minor things
ci/docker/conda-java-jni.dockerfile
Outdated
# specific language governing permissions and limitations | ||
# under the License. | ||
|
||
FROM ghcr.io/mamba-org/micromamba:ubuntu24.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm new to the process so this question might be dumb. Do we only build JNI libraries for ubuntu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the resulting libraries shouldn't be tied to Ubuntu. But perhaps we should use an older system as a base to avoid potential glibc incompatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, this is for testing and not for building the actual binaries, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! Let me investigate how the actual binaries (for different platforms) are built for a release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
.github/workflows/test.yml
Outdated
@@ -38,16 +38,21 @@ env: | |||
|
|||
jobs: | |||
ubuntu: | |||
name: AMD64 Ubuntu 22.04 JDK ${{ matrix.jdk }} Maven ${{ matrix.maven }} | |||
name: ${{ matrix.name }} AMD64 Ubuntu 22.04 JDK ${{ matrix.jdk }} Maven ${{ matrix.maven }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use "Ubuntu 24.04" for conda-java-jni-cdata?
(Or we may want to use just "Ubuntu".)
ci/scripts/java_jni_build.sh
Outdated
n_jobs=$(nproc) | ||
;; | ||
Darwin) | ||
n_jobs=$(sysctl -n hw.ncpu) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, hw.logicalcpu
is better than hw.ncpu
:
n_jobs=$(sysctl -n hw.ncpu) | |
n_jobs=$(sysctl -n hw.logicalcpu) |
hw.activecpu
The number of enabled logical processor cores in the SoC. This is an alias of hw.logicalcpu.
hw.ncpu
The number of logical processor cores in the SoC. This is an alias of hw.logicalcpu_max.
hw.logicalcpu
The number of enabled logical processor cores in the SoC. This is an alias of hw.activecpu.
hw.logicalcpu_max
The number of logical processor cores in the SoC. This is an alias of hw.ncpu.
hw.physicalcpu
The number of enabled physical processor cores in the SoC.
hw.physicalcpu_max
The number of physical processor cores in the SoC.
docker-compose.yml
Outdated
# docker compose build java-jni-cdata | ||
# docker compose run java-jni-cdata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# docker compose build java-jni-cdata | |
# docker compose run java-jni-cdata | |
# docker compose build conda-java-jni-cdata | |
# docker compose run conda-java-jni-cdata |
docker-compose.yml
Outdated
/bin/bash -c " | ||
/arrow-java/ci/scripts/java_build.sh /arrow-java /build && | ||
/arrow-java/ci/scripts/java_test.sh /arrow-java /build" | ||
|
||
conda-java-jni-cdata: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to remove the -java
part because this repository is only for the Java implementation.
Updated. |
.github/workflows/test.yml
Outdated
maven: [3.9.9] | ||
image: [java, conda-jni-cdata] | ||
include: | ||
- image: java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this "java" too?
"ubuntu"? Because maven
image uses Ubuntu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, I also tweaked the CI job names a bit so that "Conda" is in the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Fixes #14.