Switch to using a simpler-but-still-hacky way of invoking stata

[bloodearnest]

We create a wrapper script around the supplied script that writes a file
if it successfully runs the script. We use the presence of this file as
indication of success or failure, and exit appropriately.

We can then use the regular stata (without -b), which gives us a much simpler streaming stdout.

We also refactor the Dockerfile to our new approach, and add a very basic test suite

[evansd]

Ah, this is both so great and so terrible that it needs to exist. Only real question for me is over the wrapper= thing.

[evansd]

Would shellcheck tell us to use script="$1" here as, theoretically, the argument could contain spaces?

[bloodearnest]

So shellcheck doesn't complain, as it knows $1 must be space-less, for it to be parsed as $1 on the cli! But it does no harm, so will add

Worth adding shellcheck to the lint command though.

[evansd]

Args are definitely not guaranteed to be space-less, unless I'm misunderstanding you

dave@carnap:~/tmp$ cat test.sh 
#!/bin/bash
echo "\$1 is $1"

dave@carnap:~/tmp$ ./test.sh 'hello there'
$1 is hello there

[bloodearnest]

right, fair point. I've already fixed it anyway, though.

[bloodearnest]

I guess my point was about what actions would have as their first argument is designed to be interpreted as a path to pass to stata, but there's no harm in being explicit about it.

[bloodearnest]

Also foo=$1 as assignment is fine in bash, it's if you use it in other commands its an issue (which in hindsight is I think the real reason shellcheck didn't complain).

[evansd]

Also foo=$1 as assignment is fine in bash

Ah, so it is! I've always been so paranoid about quoting everything I never realised that

[evansd]

I don't think this transformation will work in general. $script will usually contain slashes so analysis/my_script.do becomes wrapper.analysis/my_script.do which will give you an error when you try to write to it. Maybe just use mktemp again?

[bloodearnest]

arg, good point.

So, I didn't want to use temp, as that made for ugly logs for users to see (I want to link the wrapper name to their name as much as possible). It's easy enough to fix though, with ${script%.do}.wrapper.do

[evansd]

Bash string transformations will never cease to amaze me!

I agree it makes sense to try to keep the name sensible-looking, I just wonder if there's any way to do that while keeping it out of /workspace. The trap .. rm solution is neat but there's still something a bit yucky about writing temporary stuff as root in the working directory. What about something like replacing the workspace path segment with wrapped so you'd get e.g. /wrapped/analysis/script.do? Not saying that's a great solution either but feels worth thinking through the solution space a bit further.

[bloodearnest]

I think we need to be explicit about the fact there is a wrapper, but obvious what its wrapping.

Which for me means the basename not being the same as the scriptname (e.g. I expect most people would not notice /wrapped/ above and they'd just read /workspace/ there, which potential confusion),,

e.g. if their action is stata analysis/model.do, the log files will talk about analysis/model.wrapper.do, i.e same path prefix.

Also, I'm wary of putting it in a different directory, in case there are some odd CWD semantics in stata.

We could do analysis/wrapper-model.do or something similar, but stata seems to require a .do extension.

[evansd]

Also, I'm wary of putting it in a different directory, in case there are some odd CWD semantics in stata.

That is a fair worry. Fine to leave as is I think. Just felt worth checking there wasn't a good alternative.

[evansd]

I reckon future us would be grateful for more detail here. Specifically the fact it always exits 0, that it doesn't stop on error if you pipe commands in, and that it doesn't log to stdout if you use batch mode.

[bloodearnest]

Yep, already had a change pending for this, and a README update

[evansd]

Tests! 🎉