Copilot commented on code in PR #11332:
URL:
https://github.com/apache/incubator-gluten/pull/11332#discussion_r2653119388
##########
.github/workflows/velox_backend_x86.yml:
##########
@@ -188,8 +188,8 @@ jobs:
$MVN_CMD clean install -P${{ matrix.spark }} -P${{ matrix.java }}
-Pbackends-velox -DskipTests
fi
cd $GITHUB_WORKSPACE/tools/gluten-it
- $MVN_CMD clean install -P${{ matrix.spark }} -P${{ matrix.java }} \
- && GLUTEN_IT_JVM_ARGS=-Xmx5G sbin/gluten-it.sh queries-compare \
+ $GITHUB_WORKSPACE/$MVN_CMD clean install -P${{ matrix.spark }} -P${{
matrix.java }}
Review Comment:
The Maven wrapper invocation is inconsistent. On line 191, the path is
prefixed with `$GITHUB_WORKSPACE/` while the same command on line 188 uses
`$MVN_CMD` which already resolves to `build/mvn -ntp`. These should both use
the same approach for consistency. Either use `$MVN_CMD` consistently, or use
the full path consistently.
##########
.github/workflows/velox_backend_x86.yml:
##########
@@ -505,7 +503,7 @@ jobs:
cd $GITHUB_WORKSPACE/
$MVN_CMD clean install -P${{ matrix.spark }} -Pbackends-velox
-DskipTests
cd $GITHUB_WORKSPACE/tools/gluten-it
- $MVN_CMD clean install -P${{ matrix.spark }}
+ $GITHUB_WORKSPACE/build/mvn clean install -P${{ matrix.spark }}
Review Comment:
The Maven wrapper invocation is inconsistent. This uses
`$GITHUB_WORKSPACE/build/mvn` instead of the `$MVN_CMD` environment variable.
For consistency and maintainability, this should use `$MVN_CMD` instead.
```suggestion
$MVN_CMD clean install -P${{ matrix.spark }}
```
##########
.github/workflows/velox_backend_x86.yml:
##########
@@ -270,7 +269,7 @@ jobs:
$MVN_CMD clean install -P${{ matrix.spark }} -P${{ matrix.java }}
-Pbackends-velox -DskipTests
fi
cd $GITHUB_WORKSPACE/tools/gluten-it
- $MVN_CMD clean install -P${{ matrix.spark }} -P${{ matrix.java }}
+ $GITHUB_WORKSPACE/build/mvn clean install -P${{ matrix.spark }}
-P${{ matrix.java }}
Review Comment:
The Maven wrapper invocation is inconsistent. This uses a hardcoded path
`build/mvn` instead of the `$MVN_CMD` environment variable that was set at the
top of the workflow. For consistency and maintainability, this should use
`$MVN_CMD` instead, which would also include the `-ntp` flag.
```suggestion
$MVN_CMD clean install -P${{ matrix.spark }} -P${{ matrix.java }}
```
##########
.github/workflows/velox_backend_x86.yml:
##########
@@ -390,7 +388,7 @@ jobs:
cd $GITHUB_WORKSPACE/
$MVN_CMD clean install -P${{ matrix.spark }} -Pbackends-velox
-DskipTests
cd $GITHUB_WORKSPACE/tools/gluten-it
- $MVN_CMD clean install -P${{ matrix.spark }}
+ $GITHUB_WORKSPACE/build/mvn clean install -P${{ matrix.spark }}
Review Comment:
The Maven wrapper invocation is inconsistent. This uses
`$GITHUB_WORKSPACE/build/mvn` instead of the `$MVN_CMD` environment variable.
For consistency and maintainability, this should use `$MVN_CMD` instead.
```suggestion
$MVN_CMD clean install -P${{ matrix.spark }}
```
##########
.github/workflows/velox_backend_x86.yml:
##########
@@ -621,7 +618,8 @@ jobs:
bash -c "echo -e
'CELEBORN_MASTER_MEMORY=8g\nCELEBORN_WORKER_MEMORY=8g\nCELEBORN_WORKER_OFFHEAP_MEMORY=16g'
> ./conf/celeborn-env.sh" && \
bash -c "echo -e 'celeborn.worker.commitFiles.threads
32\nceleborn.worker.sortPartition.threads 16' > ./conf/celeborn-defaults.conf"
&& \
bash ./sbin/start-master.sh && bash ./sbin/start-worker.sh && \
- cd $GITHUB_WORKSPACE/tools/gluten-it && $MVN_CMD clean install
-Pspark-3.2 -Pceleborn ${EXTRA_PROFILE} && \
+ cd $GITHUB_WORKSPACE && $MVN_CMD clean install -Pspark-3.2
-Pceleborn ${EXTRA_PROFILE} -Pbackends-velox -DskipTests && \
+ cd $GITHUB_WORKSPACE/tools/gluten-it && $GITHUB_WORKSPACE/$MVN_CMD
clean install -Pspark-3.2 -Pceleborn ${EXTRA_PROFILE} && \
Review Comment:
The Maven wrapper invocation is inconsistent. Line 622 uses
`$GITHUB_WORKSPACE/$MVN_CMD` which will expand to something like
`$GITHUB_WORKSPACE/build/mvn -ntp`, creating an incorrect path. This should use
just `$MVN_CMD` since it already contains the correct relative path.
```suggestion
cd $GITHUB_WORKSPACE/tools/gluten-it && $MVN_CMD clean install
-Pspark-3.2 -Pceleborn ${EXTRA_PROFILE} && \
```
##########
.github/workflows/velox_backend_x86.yml:
##########
@@ -561,7 +559,7 @@ jobs:
run: |
export JAVA_HOME=/usr/lib/jvm/java-1.8.0-openjdk && \
cd $GITHUB_WORKSPACE/tools/gluten-it && \
- $MVN_CMD clean install -P${{ matrix.spark }} -Puniffle && \
+ $GITHUB_WORKSPACE/build/mvn clean install -P${{ matrix.spark }}
-Puniffle && \
Review Comment:
The Maven wrapper invocation is inconsistent. This uses
`$GITHUB_WORKSPACE/build/mvn` instead of the `$MVN_CMD` environment variable.
For consistency and maintainability, this should use `$MVN_CMD` instead.
```suggestion
$MVN_CMD clean install -P${{ matrix.spark }} -Puniffle && \
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
