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.

Sign up for GitHub

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

build: Make the build system work out of box #136

Merged
merged 2 commits into from
Mar 1, 2024
Merged

build: Make the build system work out of box #136

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32be12d
build: Make the build system work out of box
advancedxy Feb 29, 2024
de8822a
address comments
advancedxy Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions .github/actions/java-test/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,9 @@ runs:
- name: Run Maven compile
shell: bash
run: |
echo "JAVA_VERSION=${JAVA_VERSION}"
if [ $JAVA_VERSION == "1.8" ]; then
./mvnw -B compile test-compile scalafix:scalafix -Psemanticdb -Djava.version=${JAVA_VERSION} -Dspotless.version=2.29.0
else
./mvnw -B compile test-compile scalafix:scalafix -Psemanticdb -Djava.version=${JAVA_VERSION}
fi
./mvnw -B compile test-compile scalafix:scalafix -Psemanticdb

- name: Run tests
shell: bash
run: |
if [ $JAVA_VERSION == "1.8" ]; then
SPARK_HOME=`pwd` ./mvnw -B clean install -Djava.version=${JAVA_VERSION} -Dspotless.version=2.29.0
else
SPARK_HOME=`pwd` ./mvnw -B clean install -Djava.version=${JAVA_VERSION}
fi
SPARK_HOME=`pwd` ./mvnw -B clean install
9 changes: 1 addition & 8 deletions .github/actions/rust-test/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,7 @@ runs:
shell: bash
run: |
cd common
if [ $JAVA_VERSION == "1.8" ]; then
../mvnw -B clean compile -DskipTests -Djava.version=${JAVA_VERSION} -Dspotless.version=2.29.0
else
../mvnw -B clean compile -DskipTests -Djava.version=${JAVA_VERSION}
fi
../mvnw -B clean compile -DskipTests

- name: Run Cargo test
shell: bash
Expand All @@ -64,9 +60,6 @@ runs:
# This is required to run some JNI related tests on the Rust side
JAVA_LD_LIBRARY_PATH=$JAVA_HOME/lib/server:$JAVA_HOME/lib:$JAVA_HOME/lib/jli
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rationale mentions 1.8, but it only works for 11 and 17

make and mvn commands should just work out of box for JDK 1.8, 11 and 17

# special handing for java 1.8 for both linux and mac distributions
advancedxy marked this conversation as resolved.
Show resolved Hide resolved
if [ $JAVA_VERSION == "8" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this is no longer required

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some search. It looks like that upgrading to jni-rs 0.21 removes the need for explicit setting (DY)LD_LIBRARY_PATHs. See jni-rs/jni-rs#293 for related code change.

First thing first, this test condition is wrong as we have changed the JAVA_VERSION to 1.8 during the iteration of MR refinements. So the next lines are never executed.

This line was added when I'm adding support for JDK 1.8 support. At the time of make all the tests running, the jni-rs was upgraded from 0.19 to 0.21. So my PR with these wrong and unnecessary lines passes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Java11 the tests run without any LD_LIBRARY_PATH setting since the upgrade to jni-rs 0.21

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, good to know!

JAVA_LD_LIBRARY_PATH=$JAVA_HOME/jre/lib/amd64/server:$JAVA_HOME/jre/lib/amd64:$JAVA_HOME/jre/lib/amd64/jli:$JAVA_HOME/jre/lib/server:$JAVA_HOME/jre/lib:$JAVA_HOME/jre/lib/jli
fi
RUST_BACKTRACE=1 \
LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$JAVA_LD_LIBRARY_PATH \
DYLD_LIBRARY_PATH=$DYLD_LIBRARY_PATH:$JAVA_LD_LIBRARY_PATH \
Expand Down
6 changes: 0 additions & 6 deletions .github/workflows/pr_build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ jobs:
runs-on: ${{ matrix.os }}
container:
image: amd64/rust
env:
JAVA_VERSION: ${{ matrix.java_version == 8 && '1.8' || format('{0}', matrix.java_version) }}
steps:
- uses: actions/checkout@v4
- name: Setup Rust & Java toolchain
Expand All @@ -82,8 +80,6 @@ jobs:
if: github.event_name == 'push'
name: ${{ matrix.test-target }} test on ${{ matrix.os }} with java ${{ matrix.java_version }}
runs-on: ${{ matrix.os }}
env:
JAVA_VERSION: ${{ matrix.java_version == 8 && '1.8' || format('{0}', matrix.java_version) }}
steps:
- uses: actions/checkout@v4
- name: Setup Rust & Java toolchain
Expand Down Expand Up @@ -113,8 +109,6 @@ jobs:
fail-fast: false
name: ${{ matrix.test-target }} test on macos-aarch64 with java ${{ matrix.java_version }}
runs-on: macos-14
env:
JAVA_VERSION: ${{ matrix.java_version == 8 && '1.8' || format('{0}', matrix.java_version) }}
steps:
- uses: actions/checkout@v4
- name: Setup Rust & Java toolchain
Expand Down
23 changes: 22 additions & 1 deletion DEVELOPMENT.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ under the License.

## Development Setup

1. Make sure `JAVA_HOME` is set and point to JDK 11 installation.
1. Make sure `JAVA_HOME` is set and point to JDK 8/11/17 installation.
2. Install Rust toolchain. The easiest way is to use
[rustup](https://rustup.rs).

Expand All @@ -39,6 +39,8 @@ under the License.
A few common commands are specified in project's `Makefile`:

- `make`: compile the entire project, but don't run tests
- `make test-rust`: compile the project and run tests in Rust side
- `make test-java`: compile the project and run tests in Java side
- `make test`: compile the project and run tests in both Rust and Java
side.
- `make release`: compile the project and creates a release build. This
Expand All @@ -47,6 +49,25 @@ A few common commands are specified in project's `Makefile`:
- `make clean`: clean up the workspace
- `bin/comet-spark-shell -d . -o spark/target/` run Comet spark shell for V1 datasources
- `bin/comet-spark-shell -d . -o spark/target/ --conf spark.sql.sources.useV1SourceList=""` run Comet spark shell for V2 datasources

## Opening Project in IDEs
advancedxy marked this conversation as resolved.
Show resolved Hide resolved
Comet is a multi-language project with native code written in Rust and JVM code written in Java and Scala.
For Rust code, the CLion IDE is recommended. For JVM code, IntelliJ IDEA is recommended.

Before opening the project in an IDE, make sure to run `make` first to generate the necessary files for the IDEs. Currently, it's mostly about
generating protobuf message classes for the JVM side. It's only required to run `make` once after cloning the repo.

### IntelliJ IDEA
First make sure to install the Scala plugin in IntelliJ IDEA.
After that, you can open the project in IntelliJ IDEA. The IDE should automatically detect the project structure and import as a Maven project.

### CLion
First make sure to install the Rust plugin in CLion or you can use the dedicated Rust IDE: RustRover.
After that you can open the project in CLion. The IDE should automatically detect the project structure and import as a Cargo project.

### Running Tests in IDEA
Like other Maven projects, you can run tests in IntelliJ IDEA by right-clicking on the test class or test method and selecting "Run" or "Debug".
However if the tests is related to the native side. Please make sure to run `make core` or `cd core && cargo build` before running the tests in IDEA.

## Benchmark

Expand Down
9 changes: 9 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,17 @@ all: core jvm

core:
cd core && cargo build
test-rust:
# We need to compile CometException so that the cargo test can pass
./mvnw compile -pl common -DskipTests $(PROFILES)
cd core && cargo build && \
LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+${LD_LIBRARY_PATH}:}${JAVA_HOME}/lib:${JAVA_HOME}/lib/server:${JAVA_HOME}/lib/jli && \
DYLD_LIBRARY_PATH=${DYLD_LIBRARY_PATH:+${DYLD_LIBRARY_PATH}:}${JAVA_HOME}/lib:${JAVA_HOME}/lib/server:${JAVA_HOME}/lib/jli \
RUST_BACKTRACE=1 cargo test
jvm:
./mvnw clean package -DskipTests $(PROFILES)
test-jvm: core
SPARK_HOME=`pwd` COMET_CONF_DIR=$(shell pwd)/conf RUST_BACKTRACE=1 ./mvnw verify $(PROFILES)
test:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can update test to use test-rust and test-jvm now.

./mvnw clean
# We need to compile CometException so that the cargo test can pass
Expand Down
46 changes: 37 additions & 9 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,9 +492,6 @@ under the License.
<spark.version>3.2.2</spark.version>
<spark.version.short>3.2</spark.version.short>
<parquet.version>1.12.0</parquet.version>
<java.version>1.8</java.version>
<maven.compiler.source>${java.version}</maven.compiler.source>
<maven.compiler.target>${java.version}</maven.compiler.target>
</properties>
</profile>

Expand All @@ -505,9 +502,6 @@ under the License.
<spark.version>3.3.2</spark.version>
<spark.version.short>3.3</spark.version.short>
<parquet.version>1.12.0</parquet.version>
<java.version>11</java.version>
<maven.compiler.source>${java.version}</maven.compiler.source>
<maven.compiler.target>${java.version}</maven.compiler.target>
</properties>
</profile>

Expand All @@ -517,9 +511,6 @@ under the License.
<scala.version>2.12.17</scala.version>
<spark.version.short>3.4</spark.version.short>
<parquet.version>1.13.1</parquet.version>
<java.version>11</java.version>
<maven.compiler.source>${java.version}</maven.compiler.source>
<maven.compiler.target>${java.version}</maven.compiler.target>
</properties>
</profile>

Expand All @@ -531,6 +522,43 @@ under the License.
</properties>
</profile>

<profile>
<id>jdk1.8</id>
<activation>
<jdk>1.8</jdk>
</activation>
<properties>
<java.version>1.8</java.version>
<maven.compiler.source>${java.version}</maven.compiler.source>
<maven.compiler.target>${java.version}</maven.compiler.target>
<spotless.version>2.29.0</spotless.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

</properties>
</profile>

<profile>
<id>jdk11</id>
<activation>
<jdk>11</jdk>
</activation>
<properties>
<java.version>11</java.version>
<maven.compiler.source>${java.version}</maven.compiler.source>
<maven.compiler.target>${java.version}</maven.compiler.target>
</properties>
</profile>

<profile>
<id>jdk17</id>
<activation>
<jdk>17</jdk>
</activation>
<properties>
<java.version>17</java.version>
<maven.compiler.source>${java.version}</maven.compiler.source>
<maven.compiler.target>${java.version}</maven.compiler.target>
</properties>
</profile>

<profile>
<id>semanticdb</id>
<properties>
Expand Down