[#5626] feat (gvfs-fuse): Set up a submodule project for the Gvfs-fuse #5627
Conversation
clients/gvfs-fuse/build.gradle.kts
Outdated
There was a problem hiding this comment.
I think you need use a Gradle for Rust plugin to compile this module, just like client-python.
maybe we can use the Gradle Rust plugin to install Rust compile tools in the temp dir.
There was a problem hiding this comment.
I tried some rust plugins. I can't found a good one
|
suggest to do some auto check like |
clients/gvfs-fuse/check_rust_env.sh
Outdated
| exit 1 | ||
| fi | ||
|
|
||
| export PATH="$HOME/.cargo/bin:$PATH" |
There was a problem hiding this comment.
What if I run this script several times?
There was a problem hiding this comment.
If successful, the install branch can be skipped
There was a problem hiding this comment.
I'd suggest we don't do this in the script.
Most of the software installer scripts avoid doing this.
It may and may not be desirable for the user.
| if ! command -v cargo &> /dev/null; then | ||
| echo "Rust is not installed. Installing Rust..." | ||
|
|
||
| if command -v curl &> /dev/null; then | ||
| curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y | ||
| elif command -v wget &> /dev/null; then | ||
| wget -qO- https://sh.rustup.rs | sh -s -- -y | ||
| else | ||
| echo "Error: Neither curl nor wget is available. Please install one of them to proceed." | ||
| exit 1 | ||
| fi | ||
|
|
||
| export PATH="$HOME/.cargo/bin:$PATH" | ||
| if command -v cargo &> /dev/null; then | ||
| echo "Rust has been installed successfully." | ||
| else | ||
| echo "Error: Rust installation failed. Please check your setup." | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo "Rust is already installed: $(cargo --version)" | ||
| fi |
There was a problem hiding this comment.
I'd do this to 1) return early if possible 2) save some of the nested if- tests.
if command -v cargo &> /dev/null; then
echo "Rust is already installed: $(cargo --version)"
exit 0
fi
echo "Rust is not installed. Installing Rust..."
# the rest of the install and check
# ...
if ! command -v cargo &> /dev/null; then
echo "Error: Rust installation failed. Please check your setup."
exit 1
fi
echo "Rust has been installed successfully."
exit 0There was a problem hiding this comment.
The cargo command does not run after running the rust installation command. Unless you set the PATH environment variable or restart the bash session.
There was a problem hiding this comment.
well ... we are assuming that cargo is installed in a specific directory, i.e. $HOME/.cargo/bin, any way...
The only difference is that we manually invoke the binary (with full path) or let the "polluted" shell to find it. Right?
There was a problem hiding this comment.
Yes, the cargo install script will add logic to modify the PATH inn the .profile. If we restart the shell session, it will automatically add this environment variable.
Is modifying the PATH not allowed?
|
@FANNG1 can you please help to review? |
clients/filesystem-fuse/Cargo.toml
Outdated
| [package] | ||
| name = "filesystem-fuse" | ||
| version = "0.8.0-incubating-SNAPSHOT" | ||
| edition = "2021" |
There was a problem hiding this comment.
What is the usage of this "edition", shall we remove this?
There was a problem hiding this comment.
The version specify the minimum Rust compiler version required for this project.
|
suggest adding basic check for code style and common errors in ci |
clients/filesystem-fuse/Cargo.toml
Outdated
| path = "src/main.rs" | ||
|
|
||
| [dependencies] | ||
| fuse3 = { version = "0.8.1", "features" = ["tokio-runtime", "unprivileged"] } |
There was a problem hiding this comment.
There are many fuse implement in RUST, why you choose fuse3?
There was a problem hiding this comment.
seems fuser is more popular
There was a problem hiding this comment.
Yes, fuser is more popular, but FUSE3 is used by OpenDAL. We need to decide which one to use soon
|
@FANNG1 can you please help to review again? If there's no blocking issue, I think we can improve the building command in the following PRs, what do you think? |
Seems there're some basic issues to be addressed,
|
|
@FANNG1 |
|
LGTM, except minor comments |
| """ | ||
| set -e | ||
| echo "Checking the code format" | ||
| cargo fmt --all -- --check |
There was a problem hiding this comment.
Normally, cargo fmt and clippy aren't combined with build. do you plan to separate it ?
There was a problem hiding this comment.
What are the benefits of splitting it up?
There was a problem hiding this comment.
I could do the check seperately without building the code.
What changes were proposed in this pull request?
Set up a submodule project for the Gvfs-fuse, layout the rust module environment for gvfs-fuse
Why are the changes needed?
Fix: #5626
Does this PR introduce any user-facing change?
No
How was this patch tested?
No