Move build logic to lib #97

lexfrl · 2018-07-31T18:15:08Z

Just a subtraction of build logic to a lib

lexfrl · 2018-07-31T18:22:51Z

don't quite understand a reason why

test optimizer::tests::call_indirect ... FAILED

But locally it passes. And also there is no changes in optimizer code..

lexfrl · 2018-07-31T21:40:35Z

so, it failing in master too https://travis-ci.org/paritytech/wasm-utils/builds/410384083

pepyakin · 2018-07-31T21:43:59Z

The error seems to come from the newest parity-wasm library, in particular this change.

NikVolf · 2018-08-01T10:49:38Z

cli/build/main.rs

+		matches.is_present("enforce_stack_adjustment"),
+		matches.value_of("shrink_stack").unwrap_or_else(|| "49152").parse()
+			.expect("New stack size is not valid u32"),
+		matches.is_present("skip_optimization")).map_err(Error::Build)?;


).map_err should be on the next line

NikVolf · 2018-08-01T10:50:14Z

cli/build/main.rs

-
+	parity_wasm::serialize_to_file(
+		&path,
+		ctor_module.expect("ctor_module shouldn't be None, b/c 'constructor' argument is set to true in build")


can we avoid non-obvious abbreviations?

NikVolf · 2018-08-01T10:51:26Z

src/build.rs

+	target: Target,
+	runtime_type: Option<&[u8]>,
+	runtime_version: Option<u32>,
+	mut public_api_entries: Vec<&str>,


this is a bad choice for this argument in signature for public api

&[&str] would be ok

can you explain? I'd better to use [u8; 4] or u32, not &str

because it allows wider use of types when invoking the function

&vec!["a", "b"][..], &["a", "b"], &["a", "b"][1..], etc.

I'd like to use the type system soundness here. So I'd better use [u8; 4] here

what is [u8; 4]? how it is related to Vec<&str>?

so for user it will be more descriptive API which could help to avoid the runtime error

how can you put a string in [u8; 4] ?

Cause for runtime_type we use exactly 4 bytes. Would be nice to declare it as it is 🤗

the conversation is about strings for public_api_entries argument

ah, ok 🙈

NikVolf · 2018-08-01T10:53:02Z

src/build.rs

+	if constructor {
+		if !has_ctor(&ctor_module) {
+			Err(Error::NoCreateSymbolFound)?
+		}


what are the identation here? some spaces + tab?

cant find any spaces in indentation in this snippet

hmm.. probably github changed the way how it displays the diff

looks ok, sorry

NikVolf · 2018-08-01T10:53:29Z

src/build.rs

+}
+
+pub fn build(
+	mut module: elements::Module,


bad identations everywhere

I don't understand, why you think so?

looks like tabs everywhere, sorry

NikVolf · 2018-08-01T11:23:41Z

src/build.rs

+	mut public_api_entries: Vec<&str>,
+	enforce_stack_adjustment: bool,
+	stack_size: u32,
+	skip_optimization: bool) -> Result<(elements::Module, Option<elements::Module>), Error> {


but this is bad should be

skip_optimization: bool, ) -> Result<(elements::Module, Option<elements::Module>), Error> {

NikVolf · 2018-08-01T11:24:31Z

src/build.rs

+	inject_runtime_type,
+	PackingError,
+	OptimizerError
+	};


should be at the beginning of the line

NikVolf · 2018-08-01T11:24:41Z

src/build.rs

+use parity_wasm;
+use parity_wasm::elements;
+
+


redundant empty line

NikVolf · 2018-08-01T11:26:38Z

src/build.rs

+		}
+		let ctor_module =
+			pack_instance(
+				parity_wasm::serialize(module.clone()).map_err(Error::Encoding)?, ctor_module.clone())?;


bad indentations

should use smth like

pack_instance( ... arguments )?;

NikVolf · 2018-08-01T11:33:45Z

src/build.rs

+		}
+		let ctor_module = pack_instance(
+			parity_wasm::serialize(module.clone()).map_err(Error::Encoding)?,
+			ctor_module.clone()


there should be a comma after the last argument

NikVolf · 2018-08-01T11:36:07Z

src/build.rs

+}
+
+#[derive(Debug, Clone, Copy)]
+pub enum Target {


I don't think Target is better name that SourceTarget, since SourceTarget outlines that it is the target which we take as a source to process further

sure.
But because now it lives in build.rs, which don't care know about "files" and now it doesn't require such a context clarification, IMO

But the logic retained
Why not call it "source" then?

NikVolf · 2018-08-01T11:40:12Z

cli/build/main.rs

-
+	parity_wasm::serialize_to_file(
+		&path,
+		ctor_module.expect("ctor_module can't be None, because 'constructor' argument is set to true in build")


there should be a comma after the last argument when each is on a dedicated line

NikVolf · 2018-08-01T12:27:46Z

src/build.rs

+pub fn build(
+	mut module: elements::Module,
+	constructor: bool,
+	target: SourceTarget,


source_target as well?

NikVolf · 2018-08-01T12:30:21Z

src/build.rs

+	mut module: elements::Module,
+	constructor: bool,
+	source_target: SourceTarget,
+	runtime_type: Option<[u8; 4]>,


should it be runtime_spec: Option<([u8; 4], u32)> (or dedicated struct inside Option) instead of two parameters?

it's even parsed from cli this way - either both are present, or both are None

make sense. I'd like also add a struct for additional params (flags)

NikVolf · 2018-08-01T12:57:00Z

src/build.rs

+	shrink_unknown_stack,
+	inject_runtime_type,
+	PackingError,
+	OptimizerError


NikVolf · 2018-08-01T12:57:20Z

src/build.rs

+	public_api_entries: &[&str],
+	enforce_stack_adjustment: bool,
+	stack_size: u32,
+	skip_optimization: bool


NikVolf · 2018-08-01T12:59:13Z

cli/build/main.rs

-	fn from(err: utils::PackingError) -> Self {
-		Error::Packing(err)
-	}
+	Build(BuildError)


NikVolf · 2018-08-01T14:26:18Z

Great job

lexfrl requested review from pepyakin and NikVolf and removed request for pepyakin and NikVolf July 31, 2018 18:15

lexfrl force-pushed the move-build-to-lib branch from bea81dc to e84600d Compare August 1, 2018 10:04