Add rust support to CI #1966

yyyeerbo · 2019-09-24T10:43:54Z

This patch adds preliminary support to rust agent.

Fixes: #1965

Signed-off-by: Yang Bo bo@hyper.sh

.ci/install_rust.sh

.ci/static-checks.sh

jcvenegas

thanks for the initial integration added some comments

jcvenegas · 2019-09-25T15:25:07Z

.ci/install_rust.sh

+fi
+
+# rust specific packages
+sudo -E apt-get update && apt-get install -y build-essential g++ \


This are distro specific installs could you move this to https://github.com/kata-containers/tests/blob/master/.ci/setup_env_ubuntu.sh, and as bonus would be nice to add to all the distros.

cmake/musl-libc is missed/outdated in some other distro, that why ubuntu is used for rust agent.

maybe we will resolve cmake/musl-libc in other distros later, as what we do in osbuilder

some pakages(libstdc++-8-dev) are only in bionic. It is needed to build rust agent. move this into specific distro script error out.

jcvenegas · 2019-09-25T15:26:10Z

.ci/install_rust.sh

+rustup toolchain install ${release}-${rustarch}-unknown-linux-gnu
+rustup default ${release}-${rustarch}-unknown-linux-gnu
+rustup target install ${rustarch}-unknown-linux-musl
+ln -sf /usr/bin/g++ /bin/musl-g++


This looks distro dependent as well

no, it is not

jcvenegas · 2019-09-25T15:28:32Z

.ci/static-checks.sh

+	local -r libs=("rustjail" "oci")
+	local -r bins=(".")
+	for lib in ${libs[@]}; do
+		rustfmt --check ${lib}/src/lib.rs > /dev/null 2>&1


This depend on the repo this is run where is run and will work only in agent (as we expect but should not be this then in the agent repo

I don't understand it. directory problem?

The functions for go also assume that script 's work directory is the repo directory.

I think @jcvenegas is saying (and I agree) that you shouldn't be assuming $dir/src/lib.rs. Instead, why not do something like:

find . -type f -name "*.rs" | egrep -v "grpc-rs/|target/" | xargs rustfmt ...

Admittedly, that still hard-codes a small amount of rust agent knowledge (the grpc-rs), but that problem could be overcome easily by moving grpc-rs/ into vendor/grpc-rs/ and updating the new agents Cargo.toml to specify:

grpcio = { path = "vendor/grpc-rs" }

grpc-rs has dozens of submodules, it might be easier to use it as submodule instead of a vendor directory

lib crates have src/lib.rs, bin crates have src/main.rs. I don't understand what the problem is

I think @jcvenegas is saying (and I agree) that you shouldn't be assuming $dir/src/lib.rs. Instead, why not do something like:

find . -type f -name "*.rs" | egrep -v "grpc-rs/|target/" | xargs rustfmt ...

Admittedly, that still hard-codes a small amount of rust agent knowledge (the grpc-rs), but that problem could be overcome easily by moving grpc-rs/ into vendor/grpc-rs/ and updating the new agents Cargo.toml to specify:

grpcio = { path = "vendor/grpc-rs" }

I see. clone submodule into vendor/grpc-rs.

.ci/static-checks.sh

jcvenegas · 2019-09-25T15:47:20Z

.ci/static-checks.sh

@@ -1090,6 +1114,23 @@ main()
 			fi
 		fi

+		if [ "${RUST_AGENT:-}" == "yes" ]; then
+			# skip checks specific to go
+			if [[ "${func}" =~ "go_arch_specific" ]]; then


If someone modify this function names, the rust agent CI will be broken easily

jcvenegas · 2019-09-25T15:48:38Z

.ci/static-checks.sh

@@ -299,6 +299,27 @@ static_check_go_arch_specific()
 	eval "$linter" "${linter_args}" "$dirs"
 }

+static_check_rust_arch_specific()
+{
+	local -r libs=("rustjail" "oci")


I know this is only for rust agent but I wonder if would be good to make this a ENV variable that the script checks and in an repo we are free to set what libs we want to analyse or not.

yyyeerbo · 2019-09-26T03:17:46Z

fyi
kata-containers/osbuilder#361
https://github.com/alipay/kata-rust-agent/pull/20

.ci/install_rust.sh

.ci/static-checks.sh

jodh-intel · 2019-10-07T09:00:42Z

.ci/install_rust.sh

+release="nightly"
+
+if [ "${rustarch}" == "ppc64le" ]; then
+	rustarch="powerpc64le"


You could consider adding a --rust option to https://github.com/kata-containers/tests/blob/master/.ci/kata-arch.sh

jodh-intel · 2019-10-07T09:03:33Z

.ci/install_rust.sh

+# sudo -E apt-get -y install build-essential g++ make cmake automake autoconf m4 libc6-dev libstdc++-8-dev coreutils binutils debianutils gcc musl musl-dev musl-tools git
+
+rustup toolchain install ${release}-${rustarch}-unknown-linux-gnu
+rustup default ${release}-${rustarch}-unknown-linux-gnu


You might want to add a check for KATA_DEV_MODE to avoid surprising people running this "outside of the CI":

https://github.com/kata-containers/tests/#developer-mode

jodh-intel · 2019-10-07T09:05:01Z

.ci/setup_env_ubuntu.sh

@@ -43,6 +43,8 @@ declare -A packages=( \
 	[redis]="redis-server" \
 )

+rust_agent_pkgs="build-essential g++ make cmake automake autoconf m4 libc6-dev libstdc++-8-dev coreutils binutils debianutils gcc musl musl-dev musl-tools git"


How about:

rust_agent_pkgs=() rust_agent_pkgs+=(build-essential) rust_agent_pkgs+=(g++) rust_agent_pkgs+=(make)

That makes it easier to read the list, easier to diff and easier to sort alphabetically 😄

jodh-intel · 2019-10-07T09:12:45Z

.ci/setup_env_ubuntu.sh

@@ -63,6 +65,11 @@ main()
 		done
 	fi

+	# packages for rust agent, build on 18.04 or later
+	if [ "${VERSION_ID}" >= "18.04" ]; then


I don't understand - if building on an older version of Ubuntu, you don't need these packages? Do you mean we should exit the script with an error if version <= 18.04?

Also, this test doesn't work. You need to do something like one of the following:

result=$(echo "$VERSION_ID > 18.04" | bc)

or,

int_version=$(echo $VERSION_ID|tr -d \.) [ $int_version -ge 1804 ] && ...

Some packages are missed in older version of ubuntu. They are needed for build rust agent. I think the string comparison is ok

-_-. string comparison has no >=. The ! version < "18.04" works

jodh-intel · 2019-10-07T09:16:39Z

.ci/static-checks.sh

+	local -r libs=("rustjail" "oci")
+	local -r bins=(".")
+	for lib in ${libs[@]}; do
+		rustfmt --check ${lib}/src/lib.rs > /dev/null 2>&1


I think @jcvenegas is saying (and I agree) that you shouldn't be assuming $dir/src/lib.rs. Instead, why not do something like:

find . -type f -name "*.rs" | egrep -v "grpc-rs/|target/" | xargs rustfmt ...

Admittedly, that still hard-codes a small amount of rust agent knowledge (the grpc-rs), but that problem could be overcome easily by moving grpc-rs/ into vendor/grpc-rs/ and updating the new agents Cargo.toml to specify:

grpcio = { path = "vendor/grpc-rs" }

This patch adds preliminary support to rust agent. Fixes: kata-containers#1965 Signed-off-by: Yang Bo <bo@hyper.sh>

jodh-intel

Thanks @yyyeerbo.

lgtm

chavafg · 2019-10-10T17:05:56Z

/test

jodh-intel · 2019-10-11T07:25:36Z

Restarted two of the CI's which failed due to what looks like transitory issues.

yyyeerbo · 2019-10-11T09:37:52Z

-_-. Time out when downloading kernel

yyyeerbo force-pushed the master branch 5 times, most recently from 904fc8f to b223b8b Compare September 24, 2019 12:26

chavafg requested review from jodh-intel and jcvenegas September 24, 2019 12:58

chavafg reviewed Sep 24, 2019

View reviewed changes

.ci/install_rust.sh Outdated Show resolved Hide resolved

.ci/static-checks.sh Outdated Show resolved Hide resolved

yyyeerbo force-pushed the master branch from b223b8b to 3303173 Compare September 25, 2019 04:05

jcvenegas reviewed Sep 25, 2019

View reviewed changes

yyyeerbo force-pushed the master branch from 3303173 to 0ea3ee6 Compare September 26, 2019 03:13

yyyeerbo force-pushed the master branch 2 times, most recently from a8acbba to 0d64502 Compare September 26, 2019 03:54

chavafg reviewed Oct 1, 2019

View reviewed changes

.ci/install_rust.sh Outdated Show resolved Hide resolved

.ci/static-checks.sh Show resolved Hide resolved

.ci/static-checks.sh Show resolved Hide resolved

jcvenegas approved these changes Oct 2, 2019

View reviewed changes

jodh-intel reviewed Oct 7, 2019

View reviewed changes

yyyeerbo force-pushed the master branch from 0d64502 to 7908341 Compare October 8, 2019 04:46

ci: Add rust support to CI

7b803ca

This patch adds preliminary support to rust agent. Fixes: kata-containers#1965 Signed-off-by: Yang Bo <bo@hyper.sh>

yyyeerbo force-pushed the master branch from 7908341 to 7b803ca Compare October 8, 2019 07:17

jodh-intel approved these changes Oct 10, 2019

View reviewed changes

chavafg approved these changes Oct 10, 2019

View reviewed changes

gnawux mentioned this pull request Oct 11, 2019

Landing rust-agent in kata-containers org kata-containers/kata-containers#56

Closed

chavafg merged commit 72b3159 into kata-containers:master Oct 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rust support to CI #1966

Add rust support to CI #1966

yyyeerbo commented Sep 24, 2019

jcvenegas left a comment

jcvenegas Sep 25, 2019

yyyeerbo Sep 26, 2019

yyyeerbo Sep 26, 2019

yyyeerbo Sep 26, 2019

jcvenegas Sep 25, 2019

yyyeerbo Sep 26, 2019

jcvenegas Sep 25, 2019

yyyeerbo Sep 26, 2019

jodh-intel Oct 7, 2019

yyyeerbo Oct 8, 2019

yyyeerbo Oct 8, 2019

yyyeerbo Oct 8, 2019

yyyeerbo Oct 8, 2019

jcvenegas Sep 25, 2019

yyyeerbo Sep 26, 2019

jcvenegas Sep 25, 2019

yyyeerbo Sep 26, 2019

yyyeerbo commented Sep 26, 2019

jodh-intel Oct 7, 2019

yyyeerbo Oct 8, 2019

jodh-intel Oct 7, 2019

yyyeerbo Oct 8, 2019

jodh-intel Oct 7, 2019

yyyeerbo Oct 8, 2019

jodh-intel Oct 7, 2019

yyyeerbo Oct 8, 2019

yyyeerbo Oct 8, 2019

yyyeerbo Oct 8, 2019

jodh-intel Oct 7, 2019

jodh-intel left a comment

chavafg commented Oct 10, 2019

jodh-intel commented Oct 11, 2019

yyyeerbo commented Oct 11, 2019

Add rust support to CI #1966

Add rust support to CI #1966

Conversation

yyyeerbo commented Sep 24, 2019

jcvenegas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyyeerbo commented Sep 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel left a comment

Choose a reason for hiding this comment

chavafg commented Oct 10, 2019

jodh-intel commented Oct 11, 2019

yyyeerbo commented Oct 11, 2019