[MC] Rewrite stdin.s to use python

[boomanaiden154]

This test needs to change the file descriptor offset for the llvm-mc
output file to test that we do not get an assertion in that situation.
This doesn't seem easy to do without bash subshells. Rewrite these test
in Python so we can remove the shell requirement from this test and
enable using it with lit's internal shell. This also has the bonus of
making the behavior that we are trying to create for the test much more
explicit (a .seek call on the FD).

[ilovepi]

I don't know if I understand what you're going for in this patch. How is this an improvement over just keeping REQUIRES: shell? The REQUIRES: bit is what's going to stop the test from running in the wrong environment. That seems like the right tradeoff.

[boomanaiden154]

I don't know if I understand what you're going for in this patch. How is this an improvement over just keeping REQUIRES: shell? The REQUIRES: bit is what's going to stop the test from running in the wrong environment. That seems like the right tradeoff.

If we don't get rid of all the REQUIRES: shell checks and we want to turn on the internal shell by default, we end up losing test coverage here. Unless we explicitly have a buildbot running tests with the exteral shell, which seems like a bit of a waste.

Let me try rewriting this one in Python though, because I think that will capture the semantics of the test better and also avoid the use of bash.

[MaskRay]

The previous, uglified lld tests, and this, make me believe that we should revisit the lit feature. I am not sure I agree with this test rewriting.

[boomanaiden154]

The previous, uglified lld tests, and this, make me believe that we should revisit the lit feature. I am not sure I agree with this test rewriting.

I understand the argument that the tests in #156526 look a bit worse. It's not an opinion that everyone shares though. @petrhosek thinks it makes the tests more readable because you no longer need to know about subshells to understand the semantics of the test.

For this test though, modifying it to use python adds a bit of boilerplate, but I think significantly clarifies what the test is actually trying to test. Instead of using arguably a hack with subshells to create a fd with a non-zero offset for llvm-mc to output into, we can explicitly create exactly that in python with easily readable code.

Overall, there are three options I can see for this test.

1. Rewrite in python (what this patch currently does).
2. Rewrite to invoke the subshell through bash first.
3. Implement subshells in lit.

I think 1 makes the best set of tradeoffs. I'm not opposed to 3, but I would like a reasonably strong consensus that the handful of tests impacted by this are objectively worse in terms of readability/cleanliness before doing that. Everyone with knowledge of lit's internal shell that I have talked about subshells thinks it would be a poor decision to implement them. I have not personally done enough digging/attempted enough of an implementation to know the exact tradeoffs though.

[nikic]

I don't know if I understand what you're going for in this patch. How is this an improvement over just keeping REQUIRES: shell? The REQUIRES: bit is what's going to stop the test from running in the wrong environment. That seems like the right tradeoff.

If we don't get rid of all the REQUIRES: shell checks and we want to turn on the internal shell by default, we end up losing test coverage here. Unless we explicitly have a buildbot running tests with the exteral shell, which seems like a bit of a waste.

Somewhat off-topic for this PR, but why can't REQUIRES: shell force use of shell even if internal shell is the default? Then we don't lose test coverage and don't block this on migration of the long-tail of tests.

[boomanaiden154]

Somewhat off-topic for this PR, but why can't REQUIRES: shell force use of shell even if internal shell is the default? Then we don't lose test coverage and don't block this on migration of the long-tail of tests.

That's another option. The long tail isn't actually that long though. There are only a handful of tests left in the monorepo that don't pass with the internal shell (~10 for clang, similar numbers for compiler-rt as far as I'm aware, PRs are already up for all the LLVM tests). Most of the tests don't require any complicated adjustments to work with the internal shell and end up cleaner (or at least I would argue) after being ported.

[ilovepi]

Somewhat off-topic for this PR, but why can't REQUIRES: shell force use of shell even if internal shell is the default? Then we don't lose test coverage and don't block this on migration of the long-tail of tests.

Previously, we've discussed giving lit a hybrid mode that would allow you to run all the tests w/o REQUIRES: shell in the internal shell, but still allow it to run all the shell tests if the shell is available. That would allow us to burn down the REQUIRES: shell bits over time, and figure out what features we want the internal shell to support.

[ilovepi]

LGTM. IMO this is a much nicer way to test a property on stdin's positioning. Lets get a bit more consensus from other maintainers before landing though.

[boomanaiden154]

@MaskRay I'm going to land this now so I can get the internal shell enabled by default. If you're still unhappy with this test/the lld one, I'm happy to discuss further postcommit and work towards reaching a consensus on what to do here.