runner script #53

akash-fortanix · 2019-01-03T08:12:29Z

This script encapsulates the three steps listed here under Running.

To trigger this script on cargo run, add .cargo/config file within crate with content:

[target.x86_64-fortanix-unknown-sgx]
runner = "ftxsgx-runner-cargo"

And to set custom values for arguments to ftxsgx-elf2sgxs script, add package.metadata section in Cargo.toml

[package.metadata]
threads=15

VardhanThigle · 2019-01-03T09:29:22Z

fortanix-sgx-tools/src/bin/ftxsgx-runner-cargo.rs

+
+            let args: Vec<String> = env::args().collect();
+
+            let mut ftxsgx_elf2sgxs_command = Command::new("ftxsgx-elf2sgxs");


We can have a help option, which could mention something on the line of "ftxsgx-elf2sgxs, ftxsgx-runner etc." should be available in $PATH.

I don't think that's necessary. This tool will always be installed alongside those.

Not sure how command::new works, would that require Command::new("./ftxsgx-elf2sgxs")

No it will look up the commands in your path

akash-fortanix · 2019-01-08T09:33:31Z

Offline Review Comments:
1. Configuration should be under package.metatdata.fortanix-sgx - Done
2. If a subcommand fails, you should forward the exit code - Done
3. Don't run the commands with .output - Done
4. I think .status works here - Done
5. When a subcommand fails, print out its command line - Check?
6. expect("Unable to execute process") which process? - Done
7. Default heap size should be bigger - experiment and decide (one option: 32MB)

akash-fortanix · 2019-01-08T10:28:54Z

When a subcommand fails, print out its command line - Check?
Done

akash-fortanix

~~I believe I need to remove Cargo.lock~~

akash-fortanix · 2019-01-09T10:23:47Z

fortanix-sgx-tools/src/bin/ftxsgx-runner-cargo.rs

+                });
+            }
+
+            process::exit(0);


This is unnecessary.

akash-fortanix · 2019-01-09T10:24:06Z

fortanix-sgx-tools/src/bin/ftxsgx-runner-cargo.rs

+        }
+        None => {
+            println!("{} is not defined in the environment.", key);
+            process::exit(1);


This is incorrect.

I was not sure because in this case the sub-commands don't run, so they can't error out, and so parent process shouldn't have non-zero exit status. But I believe the inability to run the process also classifies as a error.

Not running anything is definitely an error, yes.

akash-fortanix · 2019-01-11T06:47:56Z

This PR can be reviewed.

jethrogb

Improve error handling.

Add at the top:

#[macro_use]
extern crate failure;
#[macro_use]
extern crate failure_derive;

use failure::{Error, ResultExt};

#[derive(Debug, Fail)]
enum CommandFail {
    #[fail(display = "failed to run {}, {}", _0, _1)]
    Io(String, io::Error),
    #[fail(display = "while running {} got {}", _0, _1)]
    Status(String, ExitStatus),
}

fn run_command(cmd: Command) -> Result<(), CommandFail> {
    match cmd.status() {
        Err(e) => Err(CommandFail::Io(format!("{:?}", cmd), e)),
        Ok(status) if status.success() => Ok(()),
        Ok(status) => Err(CommandFail::Status(format!("{:?}", cmd), status)),
    }
}

Move the code in fn main() into a new function:

fn run() -> Result<(), Error> {
    ...

    Ok(())
}

Replace the match env::var_os(key) block with:

let mut filepath = env::var_os(key)
    .ok_or_else(|| format_err!("{} is not defined in the environment.", key))?;

Replace the first three occurences of .expect(...) by .context(...)?.
Replace the command execution blocks by run_command(...)?.
New fn main():

fn main() {
    if let Err(e) = run() {
        eprintln!("ERROR: {}", e);
        process::exit(match e.downcast_ref::<CommandFail>() {
            Some(CommandFail::Status(_, status)) => status.code().unwrap(),
            _ => 1,
        })
    }
}

jethrogb · 2019-01-11T07:10:12Z

fortanix-sgx-tools/Cargo.toml

@@ -26,3 +26,6 @@ sgx-isa = { version = "0.2.0-rc1", path = "../sgx-isa" }
 xmas-elf = "0.6.0" # Apache-2.0/MIT
 clap = "2.2.5"     # MIT
 failure = "0.1.1"  # MIT/Apache-2.0
+serde_derive = "1.0.84"
+serde = "1.0.84"
+toml = "0.4.10"


Please add license info like for the other crates above.

jethrogb · 2019-01-11T07:10:44Z

fortanix-sgx-tools/src/bin/ftxsgx-runner-cargo.rs

+use std::io::Read;
+use std::process::{self, Command};
+
+const HEAP_SIZE: u32 = 0x20000;


Please increase default heap size

Changed to 32MB as per last discussion.

jethrogb · 2019-01-11T07:11:31Z

fortanix-sgx-tools/src/bin/ftxsgx-runner-cargo.rs

+const HEAP_SIZE: u32 = 0x20000;
+const SSAFRAMESIZE: u32 = 1;
+const STACK_SIZE: u32 = 0x20000;
+const THREADS: u32 = 1;


Can we set default number of threads based on num_cpus crate?

jethrogb · 2019-01-11T07:40:23Z

fortanix-sgx-tools/src/bin/ftxsgx-runner-cargo.rs

+#[derive(Deserialize, Debug)]
+#[serde(rename_all = "kebab-case")]
+struct Target {
+    heap_size: Option<u32>,


Should be u64

jethrogb · 2019-01-11T07:50:42Z

fortanix-sgx-tools/src/bin/ftxsgx-runner-cargo.rs

+                .expect("Unable to execute ftxsgx-elf2sgxs");
+
+            if !ftxsgx_elf2sgxs_status.success() {
+                println!("{:?}", ftxsgx_elf2sgxs_command);


jethrogb · 2019-01-11T07:51:17Z

fortanix-sgx-tools/src/bin/ftxsgx-runner-cargo.rs

+            if debug {
+                ftxsgx_elf2sgxs_command.arg("--debug");
+            }
+            let mut ftxsgx_elf2sgxs_status = ftxsgx_elf2sgxs_command.status()


I don't think mut is necessary here? And you should get a lint for that?

Yes, it wasn't necessary, but wasn't getting any lint either.

jethrogb · 2019-01-11T07:53:54Z

fortanix-sgx-tools/src/bin/ftxsgx-runner-cargo.rs

+
+            let mut bin_with_ext = String::new();
+            bin_with_ext.push_str(&args[1]);
+            bin_with_ext.push_str(".sgxs");


let bin_with_ext = args[1].clone() + ".sgxs";

jethrogb

Looks great now. r=me after minor nits

jethrogb · 2019-01-14T14:32:04Z

fortanix-sgx-tools/Cargo.toml

+serde_derive = "1.0.84"    # MIT/Apache-2.0
+serde = "1.0.84"           # MIT/Apache-2.0
+toml = "0.4.10"            # MIT/Apache-2.0
+num_cpus = "1.9.0"         # MIT/Apache-2.0


Missing newline. Please configure your editor to automatically add a newline at the end of a file

jethrogb · 2019-01-14T14:33:20Z

fortanix-sgx-tools/src/bin/ftxsgx-runner-cargo.rs

+    let key = "CARGO_MANIFEST_DIR";
+    let mut filepath = env::var_os(key)
+        .ok_or_else(|| format_err!("{} is not defined in the environment.", key))?;
+    filepath.push(OsString::from("/Cargo.toml"));


Can just write filepath.push("/Cargo.toml");

akash-fortanix · 2019-01-14T14:48:17Z

bors r=jethrogb

bors · 2019-01-14T14:48:19Z

👎 Rejected by code reviews

jethrogb · 2019-01-14T14:48:46Z

bors r+

53: runner script r=jethrogb a=akash-fortanix This script encapsulates the three steps listed [here](#49) under `Running`. To trigger this script on `cargo run`, add .cargo/config file within crate with content: > [target.x86_64-fortanix-unknown-sgx] > runner = "ftxsgx-runner-cargo" And to set custom values for arguments to `ftxsgx-elf2sgxs` script, add `package.metadata` section in Cargo.toml > [package.metadata] > threads=15 Co-authored-by: akashfortanix <[email protected]>

bors · 2019-01-14T14:53:32Z

Build succeeded

continuous-integration/travis-ci/push

akash-fortanix requested review from jethrogb and VardhanThigle January 3, 2019 08:12

VardhanThigle reviewed Jan 3, 2019

View reviewed changes

akash-fortanix force-pushed the runner branch from 8f82e24 to a835598 Compare January 8, 2019 09:31

akash-fortanix force-pushed the runner branch 2 times, most recently from cd998fb to 03cd031 Compare January 8, 2019 10:27

akash-fortanix commented Jan 9, 2019

View reviewed changes

akash-fortanix force-pushed the runner branch 2 times, most recently from 10b5cf2 to dc68783 Compare January 10, 2019 05:15

jethrogb requested changes Jan 11, 2019

View reviewed changes

akash-fortanix force-pushed the runner branch from dc68783 to d430452 Compare January 14, 2019 14:23

jethrogb requested changes Jan 14, 2019

View reviewed changes

runner script

96f2038

akash-fortanix force-pushed the runner branch from d430452 to 96f2038 Compare January 14, 2019 14:40

jethrogb approved these changes Jan 14, 2019

View reviewed changes

bors bot merged commit 96f2038 into fortanix:master Jan 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runner script #53

runner script #53

akash-fortanix commented Jan 3, 2019

VardhanThigle Jan 3, 2019

jethrogb Jan 8, 2019

VardhanThigle Jan 8, 2019 •

edited

Loading

jethrogb Jan 8, 2019

akash-fortanix commented Jan 8, 2019

akash-fortanix commented Jan 8, 2019

akash-fortanix left a comment •

edited

Loading

akash-fortanix Jan 9, 2019

akash-fortanix Jan 9, 2019

jethrogb Jan 11, 2019

akash-fortanix Jan 14, 2019

jethrogb Jan 14, 2019

akash-fortanix commented Jan 11, 2019

jethrogb left a comment

jethrogb Jan 11, 2019

jethrogb Jan 11, 2019

akash-fortanix Jan 14, 2019

jethrogb Jan 11, 2019

akash-fortanix Jan 14, 2019

jethrogb Jan 11, 2019

jethrogb Jan 11, 2019

jethrogb Jan 11, 2019

akash-fortanix Jan 14, 2019

jethrogb Jan 11, 2019

jethrogb left a comment

jethrogb Jan 14, 2019

jethrogb Jan 14, 2019

akash-fortanix commented Jan 14, 2019

bors bot commented Jan 14, 2019

jethrogb commented Jan 14, 2019

bors bot commented Jan 14, 2019


		let args: Vec<String> = env::args().collect();

		let mut ftxsgx_elf2sgxs_command = Command::new("ftxsgx-elf2sgxs");

runner script #53

runner script #53

Conversation

akash-fortanix commented Jan 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VardhanThigle Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akash-fortanix commented Jan 8, 2019

akash-fortanix commented Jan 8, 2019

akash-fortanix left a comment • edited Loading

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

akash-fortanix commented Jan 11, 2019

jethrogb 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

jethrogb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akash-fortanix commented Jan 14, 2019

bors bot commented Jan 14, 2019

jethrogb commented Jan 14, 2019

bors bot commented Jan 14, 2019

Build succeeded

VardhanThigle Jan 8, 2019 •

edited

Loading

akash-fortanix left a comment •

edited

Loading