[ZEPPELIN-6060] Integrate Thrift code generation with Maven build and enhance genthrift.sh #4797
Conversation
|
Installation of packages via I also looked at the problem with My goal was just to have the thrift files in git and generate the thrift classes each time. But due to the installation process I don't see a way to do this. How do other Thrift projects implement it? |
|
Did you come up with |
| install_thrift_macos() { | ||
| echo "Installing Thrift ${THRIFT_VERSION} on macOS..." | ||
| if command -v brew &> /dev/null; then | ||
| brew install [email protected] |
There was a problem hiding this comment.
this does not work. and formulae with the suffix name@<version> won't be added into PATH automaticlly.
$ brew install [email protected]
Warning: No available formula with the name "[email protected]". Did you mean [email protected]?
==> Searching for similarly named formulae and casks...
==> Formulae
[email protected]
To install [email protected], run:
brew install [email protected]
|
integrating thrift codegen into the regular build process makes the developer hard to set up the environment, because thrift community does not publish the convenient given the generated thrift code won't change frequently, tracking the generated thrift code and providing a script to regenerate it is a common way. I think we can make some improvements in |
|
Thank you for the discussion. My first goal is to ask him to divide thrift definition and generated files with zeppelin-intepreter module to minimize itself and clean up the module. During this job, I thought it would be better to generate thrift files easier. By the way, in my past experience, I provided all binaries for thrift compilers into a repository. Can we choose a similar way? You also can use docker to build the thrift files. Current implementation looks good personally but as others said it might be a quite difficult. WDYT? |
…tory location using THRIFT_EXEC
| @@ -17,176 +17,25 @@ | |||
| # * limitations under the License. | |||
| # */ | |||
|
|
|||
| # Determine the base directory dynamically (directory where the script is located) | |||
| THRIFT_EXEC="$(dirname "$(realpath "$0")")" | |||
There was a problem hiding this comment.
@pan3793 It seems feasible to implement the configuration to operate independently of the working directory using THRIFT_EXEC="$(dirname "$(realpath "$0")")". This approach allows for determining the location of the thrift script file itself and executing the thrift operations from that location.
There was a problem hiding this comment.
I understand your approach. However, the path is not the Thrift.exe, but the Zeppelin Thrift folder. Please change the variable name, it has just confused me a lot.
There was a problem hiding this comment.
THRIFT_EXEC should point to the thrift command directly.
There was a problem hiding this comment.
why I prefer THRIFT_EXEC instead of THRIFT_HOME?
the thrift binary has no extra dependency and can be copied to any folder directly, for example, I put the thrift binaries under $HOME/bin
$ ll bin | grep thrift
lrwxr-xr-x@ 1 chengpan staff 13B Aug 19 19:35 thrift -> thrift-0.16.0
-rwxr-xr-x@ 1 chengpan staff 3.4M Aug 19 11:13 thrift-0.14.1
-r-xr-xr-x@ 1 chengpan staff 3.3M Aug 19 18:35 thrift-0.16.0
-r-xr-xr-x@ 1 chengpan staff 3.6M Aug 1 10:15 thrift-0.20.0
-rwxr-xr-x@ 1 chengpan staff 2.9M Aug 19 11:12 thrift-0.9.3
|
@jongyoul Installing thrift and helping with its setup could be quite complicated and challenging. Therefore, I considered using Docker, but the current Docker thrift image has been deprecated since version 0.12.0, and there is no support for the current version, 0.13.0. However, through @Reamer's review, I found an alternative approach by exploring the thrift-related code here: https://github.com/apache/hbase/blob/master/hbase-thrift/pom.xml Looking at this code, instead of supporting the installation directly, we could consider an approach where we simply check if thrift is installed and proceed only if it is. This method could reduce the build burden for users who don't have thrift installed while maintaining build consistency for those who do. |
| @@ -17,176 +17,25 @@ | |||
| # * limitations under the License. | |||
| # */ | |||
|
|
|||
| # Determine the base directory dynamically (directory where the script is located) | |||
| THRIFT_EXEC="$(dirname "$(realpath "$0")")" | |||
There was a problem hiding this comment.
I understand your approach. However, the path is not the Thrift.exe, but the Zeppelin Thrift folder. Please change the variable name, it has just confused me a lot.
| <version>3.0.0</version> | ||
| <executions> | ||
| <execution> | ||
| <!-- This execution runs during the compile phase to generate Java files from Thrift files --> |
There was a problem hiding this comment.
Thrift is not installed in the CI pipeline, so these steps can only be executed using profiles similar to hbase-thrift.
| <executable>bash</executable> | ||
| <arguments> | ||
| <argument>${basedir}/src/main/thrift/genthrift.sh</argument> | ||
| <argument>install</argument> |
There was a problem hiding this comment.
genthrift.sh does not support arguments, so the execution can be removed.
What is this PR for?
This PR aims to enhance the build process in Zeppelin by integrating Thrift Java file generation into the Maven build workflow. It also updates the
genthrift.shscript to automatically handle Thrift installation. These changes are intended to reduce manual errors and ensure consistent build environments.What type of PR is it?
Improvement
Todos
exec-maven-plugintopom.xmlto automate the generation of Java files from Thrift IDL during the Maven build process.genthrift.shto support automatic Thrift installation, including improved checks for Thrift being installed.What is the Jira issue?
How should this be tested?
Questions: