Fix issue18076: allow -run to work with - (stdin) #7435
Conversation
Turns out, a trivial change is all that's needed.
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
Wow. That's a simple fix! A few comments regarding the test case.
| output_file=${dir}/test18076.sh.out | ||
|
|
||
| echo 'import std.stdio; void main() { writeln("Success"); }' | \ | ||
| $DMD -m${MODEL} -run - > ${output_file} || exit 1 |
There was a problem hiding this comment.
Don't you have to check that it actually prints "Success" here?
[[ $(echo "Success") == "Success" ]] || exit 1
test/runnable/test18076.sh
Outdated
| @@ -0,0 +1,9 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| src=runnable${SEP}extra-files | |||
test/runnable/test18076.sh
Outdated
|
|
||
| echo 'import std.stdio; void main() { writeln("Success"); }' | \ | ||
| $DMD -m${MODEL} -run - > ${output_file} || exit 1 | ||
|
|
There was a problem hiding this comment.
Remove the empty, final line?
quickfur
force-pushed
the
issue18076
branch
2 times, most recently
from
8e98977 to
a920164
Compare
|
|
||
| echo 'import std.stdio; void main() { writeln("Success"); }' | \ | ||
| $DMD -m${MODEL} -run - > ${output_file} || exit 1 | ||
| grep -q '^Success$' ${output_file} || exit 1 |
There was a problem hiding this comment.
FWIW the || exit is superfluous here.
A bash script will yield the last return value as exit code if not explicitly exited
Convince yourself:
sh -c "grep -q 'foo' afile.d" ; echo $? There was a problem hiding this comment.
True. I'm wary of relying on these implicit behaviours, though. Stating || exit 1 makes it more obvious what's happening. Not that it makes a difference, I just prefer to have things up-front than implicit. Implicit things tend to be fragile and unintentionally break later.
There was a problem hiding this comment.
That's why one usually does a set -e at the beginning 😉
https://unix.stackexchange.com/questions/15998/what-does-the-e-do-in-a-bash-shebang
PetarKirov
left a comment
PetarKirov
left a comment
There was a problem hiding this comment.
I really like the new functionality and the changes LGTM too.
| break; | ||
| } | ||
| files.push(arguments[i + 1]); | ||
| if (strcmp(arguments[i + 1], "-") == 0) |
There was a problem hiding this comment.
This is too simplistic, it won't work if there's a command after -run, e.g. dmd -run -m64 -
There was a problem hiding this comment.
Actually, -run takes an argument, so your example is invalid. (I'm not sure why whoever originally implemented -run did it this way, but that's how it currently works, and this PR is just following in its steps.)(And yes, the first time I realized -run took an argument I was surprised too. :-P)
There was a problem hiding this comment.
Oh sorry. I just looked over this again from my phone and imagined that doing dmd -run -version=Foo myfile.d is legimate, so yeah that seems like bad design too me too, but that's an opportunity for another PR ;-)
wilzbach
dismissed
their stale review
Oh I didn't realize that -run always has to take an argument
5 participants
Turns out, only a trivial change is needed to make this work, by hooking into the code implemented in #6880 for reading source code from stdin.
With this change, dmd acquires the functionality:
which compiles and runs standard input without the user needing to specify any intermediate filenames.
The above is a trivial example, of course. A more pertinent use case would be executing the source code in an editor's buffer via stdin, and having the output captured by the editor as stdout, without needing to specify intermediate temporary files. Another use case would be code generation tools that can instantly produce output without needing to manage temporary files.