Linearize execution for single job

[PATCH v3] Linearize execution for single job Export this patch

Signed-off-by: Autumn! <autumnull@posteo.net>
---
Fixes deadlock due to missing defer() on io::write
 README.md           |   2 +-
 src/main.ha         | 139 ++++++++++++++++++++++++--------------------
 test.do             |   2 +
 test/should-fail.do |   1 +
 4 files changed, 79 insertions(+), 65 deletions(-)
 create mode 100644 test/should-fail.do

diff --git a/README.md b/README.md
index b9b1a6a..30d9dc7 100644
--- a/README.md
+++ b/README.md
@@ -20,7 +20,7 @@ Problems with `redo`:
 `haredo` solves all of these problems:
 - Script syntax is plain shell script
 - Only one command with few extraneous rules
-- Source code is absurdly simple (~250 lines)
+- Source code is extremely simple (~400 lines)
 - .do files are short and modular like in `redo`
 - Builds its dependency tree on the fly, uses no database
 - Doesn't break the build state when interrupted
diff --git a/src/main.ha b/src/main.ha
index a44862f..f5a8425 100644
--- a/src/main.ha
+++ b/src/main.ha
@@ -7,6 +7,7 @@ use getopt;
 use io;
 use os;
 use os::exec;
+use path;
 use strconv;
 use strings;
 use temp;
@@ -17,11 +18,14 @@ use uuid;
 type context = struct {
 	verbose: bool,
 	quiet: bool,
+	indent: str,
+	toplevel: bool,
 	parent_timestamp: i64,
 	jobs: uint,
 	rd: io::file,
 	wr: io::file,
 };
+let tmpdir: str = "";
 
 // default environment variables
 const envprogs: [_](str, str) = [
@@ -36,9 +40,9 @@ const envprogs: [_](str, str) = [
 	("SCDOC", "scdoc"),
 ];
 
-let tmpdir: str = "";
-
 export fn main() void = {
+	// job slots for the -j option are handled by reading a byte from a pipe when
+	// a slot is acquired, and writing a byte to the pipe when a slot is freed.
 	let (rd, wr) = match (os::getenv("HAREDO_PIPE")) {
 	case void =>
 		yield unix::pipe(unix::pipe_flag::NOCLOEXEC)!;
@@ -48,16 +52,17 @@ export fn main() void = {
 		let wr = io::fdopen(strconv::stoi(wr)!);
 		yield (rd, wr);
 	};
+	// free a job slot if called from a parent .do script
+	io::write(wr, [0])!;
+	defer io::read(rd, [0])!;
 
+	const parent = os::getenv("HAREDO_PARENT");
 	let ctx = context {
 		verbose = os::getenv("HAREDO_VERBOSE") is str,
 		quiet = os::getenv("HAREDO_QUIET") is str,
-		parent_timestamp = match (os::getenv("HAREDO_PARENT")) {
-		case void =>
-			yield types::I64_MIN;
-		case let s: str =>
-			yield strconv::stoi64(s)!;
-		},
+		indent = os::tryenv("HAREDO_INDENT", ""),
+		toplevel = parent is void,
+		parent_timestamp = if (parent is void) types::I64_MIN else strconv::stoi64(parent as str)!,
 		jobs = strconv::stou(os::tryenv("HAREDO_JOBS", "1"))!,
 		rd = rd,
 		wr = wr,
@@ -65,7 +70,7 @@ export fn main() void = {
 	tmpdir = temp::dir();
 
 	const cmd = getopt::parse(os::args,
-		"simple and idiomatic build automator.\nsee `man haredo` for detailed usage.",
+		"simple and unix-idiomatic build automator.\nsee `man haredo` for detailed usage.",
 		('v', "print verbose logs"),
 		('q', "(quiet) don't print 'redo' logs"),
 		('j', "jobs", "run <jobs> jobs in parallel"),
@@ -86,15 +91,6 @@ export fn main() void = {
 			abort();
 		};
 	};
-
-	for (let i = 0z; i < ctx.jobs; i += 1) {
-		io::write(ctx.wr, [0])!;
-	};
-
-	defer for (let i = 0z; i < ctx.jobs; i += 1) {
-		io::read(ctx.rd, [0])!;
-	};
-
 	if (len(cmd.args) == 0) {
 		cmd.args = ["all"];
 	};
@@ -108,57 +104,49 @@ export fn main() void = {
 		free(children);
 	};
 
-	// first run sub-builds
+	if (ctx.toplevel) {
+		// initialize job slots
+		for (let i = 0z; i < ctx.jobs - 1; i += 1) {
+			io::write(ctx.wr, [0])!;
+		};
+	};
+
+	// first start sub-builds...
 	for (let i = 0z; i < len(cmd.args); i += 1) {
 		if (cmd.args[i] == "++") break;
-		let child = match (try_do(ctx, cmd.args[i])) {
+		let child = match (try_do(&ctx, cmd.args[i])) {
 		case void =>
 			continue;
 		case let child: (exec::process, str) =>
 			yield child;
 		case let e: exec::error =>
-			const indent = os::tryenv("HAREDO_INDENT", "");
 			fmt::fatalf("\x1b[31mharedo {}{} (error: {})\x1b[0m",
-				indent, cmd.args[i], exec::strerror(e));
+				ctx.indent, cmd.args[i], exec::strerror(e));
+		};
+		if (ctx.jobs > 1) {
+			append(children, (child.0, child.1, cmd.args[i]));
+		} else {
+			// if there's only one job (or zero), wait immediately for the child
+			const status = exec::wait(&child.0)!;
+			defer io::write(ctx.wr, [0])!;
+			if (!(cleanup_child(&ctx, &status, child.1, cmd.args[i]) is void)) os::exit(1);
 		};
-		append(children, (child.0, child.1, cmd.args[i]));
 	};
 
-	// ...then wait for them to finish
+	// ...then wait for them to finish...
 	for (len(children) != 0) {
 		const (child, status) = exec::waitany()!;
-		defer io::write(ctx.wr, [0])!;
-		let tmpfile: (str | void) = void;
-		let target: (str | void) = void;
-		for (let i = 0z; i < len(children); i += 1) {
-			if (child == children[i].0) {
-				tmpfile = children[i].1;
-				target = children[i].2;
-				delete(children[i]);
-			};
+		let i = 0z;
+		for (i < len(children); i += 1) {
+			if (child == children[i].0) break;
 		};
-		const tmpfile = tmpfile as str;
-		const target = target as str;
-		const indent = os::tryenv("HAREDO_INDENT", "");
+		assert(i < len(children), "Unknown child process returned from exec::waitany");
+		const tmpfile = children[i].1;
+		const target = children[i].2;
+		delete(children[i]);
 		defer free(tmpfile);
-
-		match (exec::check(&status)) {
-		case void => void;
-		case let e: !exec::exit_status =>
-			fmt::fatalf("\x1b[31mharedo {}{} (error: {})\x1b[0m",
-				indent, target, exec::exitstr(e));
-		};
-		match (os::move(tmpfile, target)) {
-		case void => void;
-		case errors::noentry => void;
-		case let e: fs::error =>
-			fmt::fatalf("\x1b[31mharedo {}{} (error: {})\x1b[0m",
-				indent, target, fs::strerror(e));
-		};
-		if (!ctx.quiet) {
-			errorfln("\x1b[32mharedo {}{} (done)\x1b[0m",
-				indent, target)!;
-		};
+		defer io::write(ctx.wr, [0])!;
+		if (!(cleanup_child(&ctx, &status, tmpfile, target) is void)) os::exit(1);

Ember Sawady <ecs@d2evs.net>

1 year, 8 months ago

Better to handle printing the error message here rather than in
cleanup_child, since the io::error here won't get printed out. Should
also allow you to avoid duplicating the errorfln. Oh, you should also
make sure to use errorfln() rather than fmt::errorfln(), in order to
avoid the interleaving issue.

 	};
 
 	// ...then check for updates
@@ -189,14 +177,12 @@ type do_paths = struct {
 };
 
 fn try_do(
-	ctx: context,
+	ctx: *context,
 	target: str,
 ) ((exec::process, str) | void | exec::error) = {
-	const indent = os::tryenv("HAREDO_INDENT", "");
-
-	const dopaths = match (find_do_file(target)) {
+	const dopaths = match (find_do_file(target)!) {
 	case void =>
-		if (!ctx.quiet) errorfln("\x1b[33mharedo {}{} (no dofile)\x1b[0m", indent, target)?;
+		if (!ctx.quiet) errorfln("\x1b[33mharedo {}{} (no dofile)\x1b[0m", ctx.indent, target)?;
 		return;
 	case let s: do_paths =>
 		yield s;
@@ -218,7 +204,7 @@ fn try_do(
 
 	if (!ctx.quiet) {
 		// indent subprocesses by 2 spaces
-		const new_indent = strings::concat("  ", indent);
+		const new_indent = strings::concat("  ", ctx.indent);
 		defer free(new_indent);
 		exec::setenv(&cmd, "HAREDO_INDENT", new_indent)?;
 	};
@@ -237,16 +223,19 @@ fn try_do(
 	if (ctx.verbose) exec::setenv(&cmd, "HAREDO_VERBOSE", "1")?;
 	if (ctx.quiet) exec::setenv(&cmd, "HAREDO_QUIET", "1")?;
 
-	// set default program variables in toplevel process
-	if (os::getenv("HAREDO_PARENT") is void) {
+	if (ctx.toplevel) {
+		// set default program variables in toplevel process
 		for (let i = 0z; i < len(envprogs); i += 1) {
 			const (var, value) = envprogs[i];
 			exec::setenv(&cmd, var, os::tryenv(var, value))?;
 		};
-		exec::unsetenv(&cmd, "HAREDO_JOBS")?;
+
+		// set pipe for job control
 		let pipe = fmt::asprintf("{},{}", ctx.rd: int, ctx.wr: int);
 		defer free(pipe);
 		exec::setenv(&cmd, "HAREDO_PIPE", pipe)?;
+
+		exec::setenv(&cmd, "HAREDO_JOBS", strconv::utos(ctx.jobs))?;
 	};
 
 	const proc = match (exec::fork()?) {
@@ -255,7 +244,7 @@ fn try_do(
 	case void =>
 		io::read(ctx.rd, [0])!;
 		if (!ctx.quiet) errorfln("\x1b[32mharedo {}{}\x1b[0m",
-			indent, target)?;
+			ctx.indent, target)?;
 		os::chdir(dopaths.execdir)!;
 		exec::exec(&cmd);
 	};
@@ -265,6 +254,28 @@ fn try_do(
 	return (proc, tmpfilepath);
 };
 
+fn cleanup_child(ctx: *context, status: *exec::status, tmpfile: str, target: str) (void | io::error | !exec::exit_status | fs::error) = {

Ember Sawady <ecs@d2evs.net>

1 year, 8 months ago
This should be split across multiple lines, like what I did for try_do.

+	match (exec::check(status)) {
+	case void => void;
+	case let e: !exec::exit_status =>
+		fmt::errorfln("\x1b[31mharedo {}{} (error: {})\x1b[0m",
+			ctx.indent, target, exec::exitstr(e))?;
+		return e;
+	};
+	match (os::move(tmpfile, target)) {
+	case void => void;
+	case errors::noentry => void;
+	case let e: fs::error =>
+		fmt::errorfln("\x1b[31mharedo {}{} (error: {})\x1b[0m",
+			ctx.indent, target, fs::strerror(e))?;
+		return e;
+	};
+	if (!ctx.quiet) {
+		errorfln("\x1b[32mharedo {}{} (done)\x1b[0m",
+			ctx.indent, target)?;
+	};
+};
+
 // finds the do file for a given target.
 // all strings in do_paths must be freed by the caller.
 fn find_do_file(target: str) (do_paths | void) = {
diff --git a/test.do b/test.do
index 605f8bd..d95280f 100644
--- a/test.do
+++ b/test.do
@@ -5,3 +5,5 @@ haredo test/all || true
 touch test/rv64/all.h
 haredo test/all || true
 haredo test/clean-gen || true
+
+haredo test/should-fail || true
diff --git a/test/should-fail.do b/test/should-fail.do
new file mode 100644
index 0000000..379a4c9
--- /dev/null
+++ b/test/should-fail.do
@@ -0,0 +1 @@
+exit 1
-- 
2.39.0

Ember Sawady <ecs@d2evs.net>

1 year, 8 months ago
-%<-

~autumnull/haredo-devel

Linearize execution for single job v3 REJECTED

[PATCH v3] Linearize execution for single job Export this patch