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

Rewrite Mainframer in Rust. #191

Merged
merged 20 commits into from
Mar 1, 2018
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
16d3165
WIP: Rewrite in Rust.
artem-zinnatullin Feb 19, 2018
fff43b9
Parse config file yoooo.
artem-zinnatullin Feb 21, 2018
e718688
Add tests for Args.
artem-zinnatullin Feb 22, 2018
d4a6f85
Add sync_local_to_remote, execute_remote_command.
artem-zinnatullin Feb 22, 2018
aff1522
Pretty much it, need print duration, etc.
artem-zinnatullin Feb 23, 2018
c5d3760
WIP: Adapt integration tests to Rust version.
artem-zinnatullin Feb 23, 2018
412eb38
Format durations, more text otput, something is wrong with sync.
artem-zinnatullin Feb 24, 2018
6e287bd
Pretty much it! Passed all tests.
artem-zinnatullin Feb 25, 2018
12550ec
Nits.
artem-zinnatullin Feb 25, 2018
5f109d2
Add support for spaces/etc in working dir.
artem-zinnatullin Feb 25, 2018
601297a
Build debug and release versions of Mainframer as a test, run unit te…
artem-zinnatullin Feb 25, 2018
b836e2d
Reformat code, remove Bash version.
artem-zinnatullin Feb 25, 2018
0e212a6
Revert change in Buck sample.
artem-zinnatullin Feb 25, 2018
3bef0c7
Remove shellcheck.
artem-zinnatullin Feb 25, 2018
a49634f
Reduce noise during Docker build, rm unneded step from test/common.sh.
artem-zinnatullin Feb 25, 2018
d06f667
Add Artur as author even though he only wrote Bash :troll:.
artem-zinnatullin Feb 25, 2018
26c2b41
Inline start message.
artem-zinnatullin Feb 25, 2018
907efe5
Fix code review comments.
artem-zinnatullin Feb 26, 2018
c333da1
Copy Duration.
artem-zinnatullin Mar 1, 2018
07c1c7e
Address more feedback.
artem-zinnatullin Mar 1, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,8 @@
.DS_Store
build/

# Rust
target

# Share code style.
!.idea/codeStyleSettings.xml
4 changes: 4 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
name = "mainframer"
version = "2.1.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In future I'd like to detach version from source code and get it from current git tag, but we didn't do it with Bash so for simplicity I didn't do it in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to open an issue about 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.

Yeeep, here you go #192

authors = ["Artem Zinnatullin <artem.zinnatullin@gmail.com>", "Artur Dryomov <artur.dryomov@gmail.com>", "Mainframer Developers and Contributors"]

[dependencies]
3 changes: 0 additions & 3 deletions ci/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ if [ "$USER_ID" == "0" ]; then
echo "Warning: running as r00t."
fi

# Run shellcheck.
docker run --rm --env SHELLCHECK_OPTS="--exclude SC2088" --volume `"pwd"`:/scripts:ro koalaman/shellcheck:v0.4.6 /scripts/mainframer
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank God.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, actually I'm planning to add it back later to check our integration tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #194 to track that


docker build -t mainframer:latest .

# Command will run inside a container.
Expand Down
18 changes: 11 additions & 7 deletions ci/docker/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ FROM ubuntu:16.04
MAINTAINER Mainframer Team

# "sudo": switch user in entrypoint.
# "curl rustup": build Mainframer and sample Rust project.
# "openssh-server": testing.
# "openjdk-8-jdk": build sample Gradle, Maven, Buck projects.
# "golang": build sample Go project.
# "clang": build sample Clang project.
# "build-essential": build sample GCC project.
# "lib32stdc++6 lib32z1 unzip": build sample Android project.
# "ant python git": build sample Buck project.
# "curl rustup": build sample Rust project.
RUN apt-get update --quiet && \
RUN apt-get update --quiet > /dev/null && \
apt-get --assume-yes --quiet install sudo openssh-server openjdk-8-jdk \
golang clang build-essential lib32stdc++6 lib32z1 unzip ant python git curl && \
curl -sf -L https://static.rust-lang.org/rustup.sh | sh
Expand All @@ -22,14 +22,18 @@ ENV ANDROID_HOME /opt/android-sdk-linux
ENV PATH ${PATH}:${ANDROID_HOME}/tools:${ANDROID_HOME}/tools/bin:${ANDROID_HOME}/platform-tools
ENV ANDROID_SDK_INSTALL_COMPONENT "echo \"y\" | \"$ANDROID_HOME\"/tools/bin/sdkmanager --verbose"

# Give all users access to Android SDK (otherwise build_user won't be able to access it).
RUN mkdir -p $ANDROID_HOME && \
curl https://dl.google.com/android/repository/$ANDROID_SDK_FILE_NAME --progress-bar --location --output $ANDROID_SDK_FILE_NAME && \
unzip $ANDROID_SDK_FILE_NAME -d $ANDROID_HOME && \
echo "Unzipping Android SDK" && \
unzip -qq $ANDROID_SDK_FILE_NAME -d $ANDROID_HOME && \
rm $ANDROID_SDK_FILE_NAME && \
eval $ANDROID_SDK_INSTALL_COMPONENT '"tools"' && \
eval $ANDROID_SDK_INSTALL_COMPONENT '"platform-tools"' && \
eval $ANDROID_SDK_INSTALL_COMPONENT '"build-tools;25.0.2"' && \
eval $ANDROID_SDK_INSTALL_COMPONENT '"platforms;android-25"'
echo "Installing Android SDK components" && \
eval $ANDROID_SDK_INSTALL_COMPONENT '"tools"' > /dev/null && \
eval $ANDROID_SDK_INSTALL_COMPONENT '"platform-tools"' > /dev/null && \
eval $ANDROID_SDK_INSTALL_COMPONENT '"build-tools;25.0.2"' > /dev/null && \
eval $ANDROID_SDK_INSTALL_COMPONENT '"platforms;android-25"' > /dev/null && \
chmod -R 777 "$ANDROID_HOME"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it here, previously we gave ownership of this folder to the build user in entrypoint script which was slowing down iterative testing as it was running every time on container startup

Copy link
Contributor

Choose a reason for hiding this comment

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

chmod a+rwx, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

777, ruly?

@nostra13 sing it to @ming13:

Chmod777:

Podnyal chmoda, stali drugimi prava
Stali schitatsya so mnoy
Znayut kto teper chown

Podnyal chmoda, filesystem v liniu dala
U tebya talant bratan
Kkakoy?
Nastraivat linuxa

https://www.youtube.com/watch?v=qP303vxTLS8


# Entrypoint script will allow us run as non-root in the container.
COPY ci/docker/entrypoint.sh /usr/local/bin/entrypoint.sh
Expand Down
3 changes: 0 additions & 3 deletions ci/docker/entrypoint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@ echo "Starting with UID : $USER_ID"
groupadd --gid $USER_ID build_user
useradd --shell /bin/bash --uid $USER_ID --gid $USER_ID --comment "User for container" --create-home build_user

# Grant build_user access to Android SDK.
chown -R build_user:build_user $ANDROID_HOME

# Start ssh server for tests.
service ssh start

Expand Down
176 changes: 0 additions & 176 deletions mainframer

This file was deleted.

47 changes: 47 additions & 0 deletions src/args.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#[derive(Debug, PartialEq, Eq)]
pub struct Args {
pub command: String
}

impl Args {
pub fn parse(raw_args: Vec<String>) -> Result<Args, String> {
match raw_args.len() {
0 => Err(String::from("Please pass remote command.")), // TODO more user friendly message, for now it's consistent with Bash version.

Choose a reason for hiding this comment

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

FWIW, String::from(x) is equivalent to x.into().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I see the pattern here

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 still like the String::from though, what do you think is more idiomatic?

String docs use from() heavily, that's why I used it https://doc.rust-lang.org/book/second-edition/ch08-02-strings.html

But into() seems to be a common function across different types as well, maybe I should use from() for "static" values and into() for dynamic?

Choose a reason for hiding this comment

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

I'm not aware of any convention here. From and Into are duals to each other in some sense.
You are free to use either String::from(x), x.to_string(), x.to_owned() and x.into() and they will do the same thing, but I have an impression that x.into() is more common.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn Rust, String is most confusing type so far, so many options jeez (and I haven't really used Cow yet btw)

Let's stick to from() for now since docs use it? I'd probably get a sense of convention as I read/write more code, googling didn't help much, I might also ask this later on /r/rust sub to get a sense of what community thinks about it :)

_ => Ok(Args {
command: {
let str: String = raw_args
.iter()
.cloned()

Choose a reason for hiding this comment

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

Because you own raw_args you can avoid cloning by calling .into_iter(). This will consume raw_args and turn it into an iterator that yields owned Strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice! didn't know about this option, thx!

// Add space between parameters.
.map(|arg| format!("{} ", arg))

Choose a reason for hiding this comment

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

I guess, this isn't important, but this code will allocate a new String for result. This is sorta wasteful given that you already have a owned String that you can modify here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, I would thought that Compiler will be able to optimize it here

.collect();
Copy link

@pepyakin pepyakin Feb 25, 2018

Choose a reason for hiding this comment

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

... but better, I guess you are looking for something like Kotlin's joinToString. Rust also have similar method.
It is unfortunate this function is not so discoverable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

holy moly, this is now legit oneliner, THX!


String::from(str.trim())
}
})
}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn parse_command_passed_as_single_parameter() {
let raw_args = vec![String::from("test command")];
assert_eq!(Args::parse(raw_args), Ok(Args { command: String::from("test command") }));
}

#[test]
fn parse_empty() {
let raw_args: Vec<String> = vec![];
assert_eq!(Args::parse(raw_args), Err(String::from("Please pass remote command.")));
}

#[test]
fn parse_command_passed_as_multiple_parameters() {
let raw_args = vec![String::from("test"), String::from("command")];
assert_eq!(Args::parse(raw_args), Ok(Args { command: String::from("test command") }));
}
}
Loading