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.

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

Add project structure and template for native components #18164

Merged
merged 11 commits into from
Sep 26, 2023

Conversation

dbw9580
Copy link
Contributor

@dbw9580 dbw9580 commented Sep 18, 2023

What changes are proposed in this pull request?

Prepare project structure for native components.
Add a maven archetype for Rust-based native components.
The native module is behind a profile native-components and is deactivated by default.

Why are the changes needed?

Add support for native components

Does this PR introduce any user facing changes?

No.

@dbw9580 dbw9580 requested a review from beinan September 18, 2023 07:37
@jja725
Copy link
Contributor

jja725 commented Sep 18, 2023

This is a great first step for introducing Rust in Alluxio!


## Building the modules

To build Rust sources, you will need the Rust toolchains. The recommended way is to use
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be good to update the alluxio-maven image that is used as the build environment to include the Rust toolchains. the dockerfile for the image is located in dev/github/Dockerfile-jdk8. there are also dockerfiles for jdk11 and jdk17; it would be nice to update those too but they are currently not in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added rust toolchain to the dockerfile:

# Rust toolchains for native components
RUN apt-get install -y rust-all

but it does not seem to work:

2023-09-25T17:54:35.0392570Z 17:54:35.038 [INFO] 17:54:35.037 [ERROR] -> [Help 1]
2023-09-25T17:54:35.0398031Z 17:54:35.039 [INFO] org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.questdb:rust-maven-plugin:1.1.1:build (release-build) on project alluxio-rust-example: Rust's `cargo` not found in PATH=/usr/local/openjdk-8/bin:/usr/local/go/bin:/usr/local/openjdk-8/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin.
2023-09-25T17:54:35.0608292Z 17:54:35.060 [INFO] 
2023-09-25T17:54:35.0620795Z 17:54:35.061 [INFO] See https://www.rust-lang.org/tools/install

looks like the rust toolchain is not properly installed. Does it require re-building the image so that github CI can pick up the change?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes requires rebuilding the image and subsequently bumping up the image version in dev/github/run_docker.sh

by `artifactId`. The archetype generation command will also edit the pom file of the
parent module, which is `native/rust`, to add the new submodule to the module list.

> Note: The archetype plugin modifies the pom file in a way that is not compatible with
Copy link
Contributor

Choose a reason for hiding this comment

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

it may be possible to override the plugin settings for the rust module by explicitly declaring them in the pom? just a guess, i'm not confident that this is the way to do so, but i imagine there must be some way to achieve it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I digged into archetype's source code but unfortunately was unable to find a configuration option to stop it from editing the parent pom file.

One workaround is to generate the submodule outside the directory of a parent module, and then move the generated module back into native/rust, and manually add the module to the list. It's still a lot of hassle and kind of defeats the purpose of using a archetype.

pom.xml Outdated Show resolved Hide resolved
<version>${project.version}</version>
</parent>

<artifactId>\${artifactId}</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

are the /$ intentional in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes indeed. there are two passes of string interpolation:

  1. the first is by maven resource plugin, when copying the resource files from the source directory to the output directory for packaging. Things like ${project.version} should be kept in sync with the whole project and should not be customized when a user generates a submodule.
  2. the second is by the archetype plugin, when a user actually uses the archetype to generate a submodule. Things like ${artifactId} are meant to be different for different modules, so must be customized.

The escaping is used to make sure templates for the second interpolation are not eagerly processed by the resource plugin.

@@ -493,6 +494,11 @@
<artifactId>jets3t</artifactId>
<version>0.8.1</version>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

double check that the new dependencies are compatible with apache 2.0 license and not introducing any forbidden dependencies as described in https://www.apache.org/legal/resolved.html#category-x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New dependencies;

  1. JNA (net.java.dev.jna:jna-jpms): LGPL and Apache 2.0 dual licensed.
  2. Maven Archetype Plugin: Apache 2.0 as it's part of the Maven project
  3. Maven plugin for Rust(org.questdb:rust-maven-plugin): Apache 2.0.

@zuston
Copy link
Contributor

zuston commented Sep 20, 2023

Do you have any roadmap of rust in Alluxio ? I'm interested

Copy link
Contributor

@beinan beinan left a comment

Choose a reason for hiding this comment

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

Great contribution, we can start to integrate something really fun with native code now

<profile>
<id>native-components</id>
<modules>
<module>native</module>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to hide the module behind a profile because before the infrastructure for native components is fully sorted out, we don't want to block other people's builds

@@ -153,3 +153,5 @@ RUN ARCH=$(dpkg --print-architecture) && \
wget --quiet https://releases.hashicorp.com/terraform/1.0.1/terraform_1.0.1_linux_${ARCH}.zip && \
unzip -o ./terraform_1.0.1_linux_${ARCH}.zip -d /usr/local/bin/ && \
rm terraform_1.0.1_linux_${ARCH}.zip
# Rust toolchains for native components
RUN apt-get install -y rust-all
Copy link
Contributor

Choose a reason for hiding this comment

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

append rust-all to the apt-get install on line 141. reduces the number of image layers that are cached. similar for other dockerfiles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. But let me remove this change from this PR and move them into a dedicated PR that updates the docker images.

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm

@dbw9580 dbw9580 added type-feature This issue is a feature request POM Change area-build Build (maven, tarball) and tests area-native Non-Java native components labels Sep 26, 2023
@dbw9580
Copy link
Contributor Author

dbw9580 commented Sep 26, 2023

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit dc0849a into Alluxio:main Sep 26, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-build Build (maven, tarball) and tests area-native Non-Java native components POM Change type-feature This issue is a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants