You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@orc.apache.org by GitBox <gi...@apache.org> on 2020/11/06 06:49:22 UTC
[GitHub] [orc] dongjoon-hyun opened a new pull request #566: ORC-503. Add Maven Wrapper
dongjoon-hyun opened a new pull request #566:
URL: https://github.com/apache/orc/pull/566
### What changes were proposed in this pull request?
This PR aims to add `Maven Wrapper`.
### Why are the changes needed?
This will help ORC build and testing at the environment without Maven installation.
### How was this patch tested?
Manual.
```
cd java
./mvnw clean
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #566: ORC-503. Add Maven Wrapper
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #566:
URL: https://github.com/apache/orc/pull/566#issuecomment-723178703
Merged to master!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pgaref commented on pull request #566: ORC-503. Add Maven Wrapper
Posted by GitBox <gi...@apache.org>.
pgaref commented on pull request #566:
URL: https://github.com/apache/orc/pull/566#issuecomment-723089596
Hey @dongjoon-hyun thanks for looking at this!
Wrapper script LGTM -- however I believe we still have to replace the MVN build/test calls with MVNW right?
For instance:
- https://github.com/apache/orc/blob/master/.github/workflows/master.yml#L46
- https://github.com/apache/orc/blob/master/java/CMakeLists.txt#L49
And as we are only now going to depend on MVNW maybe even remove maven plugin installation from Dockerfiles completely?
- https://github.com/apache/orc/blob/master/docker/debian10/Dockerfile#L33
Another thing we have to check is if the MAVEN_OPTS are actually propagated to the wrapper (which I believe they should):
https://github.com/apache/orc/blob/master/.github/workflows/master.yml#L22
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #566: ORC-503. Add Maven Wrapper
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #566:
URL: https://github.com/apache/orc/pull/566#issuecomment-722917904
cc @omalley , @wgtmac , @pgaref
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #566: ORC-503. Add Maven Wrapper
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #566:
URL: https://github.com/apache/orc/pull/566#issuecomment-723178292
Thank you so much for review, @pgaref .
For the following, we don't need to use this in our CI or testing tool. This is for new environment.
> however I believe we still have to replace the MVN build/test calls with MVNW right?
For the following, that is not a scope of this PR. If some Docker images has a problem with old Maven versions, we had better migrate it step by step.
> And as we are only now going to depend on MVNW maybe even remove maven plugin installation from Dockerfiles completely?
The Apache Spark script already handles it like the following and uses it in Apache Spark GitHub Action job.
> Another thing we have to check is if the MAVEN_OPTS are actually propagated to the wrapper (which I believe they should):
```bash
# Set any `mvn` options if not already present
export MAVEN_OPTS=${MAVEN_OPTS:-"$_COMPILE_JVM_OPTS"}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun merged pull request #566: ORC-503. Add Maven Wrapper
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun merged pull request #566:
URL: https://github.com/apache/orc/pull/566
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #566: ORC-503. Add Maven Wrapper
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #566:
URL: https://github.com/apache/orc/pull/566#discussion_r518559909
##########
File path: java/mvnw
##########
@@ -0,0 +1,102 @@
+#!/usr/bin/env bash
Review comment:
This file is a simplified version of Apache Spark mvn wrapper.
- https://github.com/apache/spark/blob/master/build/mvn
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org