Fix redirecting WslLaunch's output to a file #29
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EduardGomezEscandell
force-pushed
the
fix-exec-output-to-file
branch
from
43ed6b2
to
b4ca1dc
Compare
EduardGomezEscandell
changed the title
EduardGomezEscandell
changed the title
EduardGomezEscandell
force-pushed
the
fix-exec-output-to-file
branch
4 times, most recently
from
aa3d4a6
to
a047622
Compare
EduardGomezEscandell
changed the title
EduardGomezEscandell
force-pushed
the
fix-exec-output-to-file
branch
from
d4ff898
to
a047622
Compare
EduardGomezEscandell
force-pushed
the
fix-exec-output-to-file
branch
from
76843be
to
d7b4048
Compare
EduardGomezEscandell
force-pushed
the
fix-exec-output-to-file
branch
2 times, most recently
from
88c4358
to
56bf249
Compare
EduardGomezEscandell
requested review from
didrocks
and removed request for
didrocks
didrocks
requested changes
Jan 25, 2023
There was a problem hiding this comment.
I can’t say I grocked every single details, but here is my general feedbacks. My main concern is the last comment (about returning nil if the command was running on WSL side, but failed). I know we discussed it but rereading it and seeing it back in tests, I’m less sure nowdays this is the right call.
EduardGomezEscandell
force-pushed
the
fix-exec-output-to-file
branch
2 times, most recently
from
ffb01de
to
b602a90
Compare
|
Forced pushed to rebase to main. Also used the oportunity to strategically squash commits so that they make sense once this branch is merged to main. |
EduardGomezEscandell
force-pushed
the
fix-exec-output-to-file
branch
from
f064c32
to
9a90d52
Compare
didrocks
reviewed
Jan 26, 2023
didrocks
reviewed
Jan 26, 2023
didrocks
reviewed
Jan 26, 2023
didrocks
requested changes
Jan 26, 2023
EduardGomezEscandell
force-pushed
the
fix-exec-output-to-file
branch
3 times, most recently
from
3b1459c
to
65f8ee0
Compare
The WSL API doesn't allow writing to a file directly without an intermediate pipe. Since pipes are also represented as an *os.File, we have no way of distinguishing the two to treat them differently. These tests demonstrate this failure, although they are skipped for now until a fix is found.
The main implication is that we offload responsabilities to the stdlib: - We can use os.Process utils instead of our own implementation - We can use exec.ExitError An adjacent change: since wsl.ExitError no longer exists, (*distro).Shell had to be rethought when it comes to its return values. I decided to create a ShellError that contains the exit code.
Stdout and Stderr pipes were being read from before the process finished, so they were sometimes empty.
We know that the WSL API doesn't allow writing to a file directly without an intermediate pipe. Since pipes are also represented as an *os.File, we have to distinguish the two with the Win32 API. This fixes this problem and removes the Skip in the tests
Sometimes WSL takes a while to boot up, making some timeouts trigger for even the simplest commands. Keeping the test distro alive during the test should prevent this.
Seems like the Azure machine is quite slow.
EduardGomezEscandell
force-pushed
the
fix-exec-output-to-file
branch
from
65f8ee0
to
8b54b41
Compare
didrocks
reviewed
Jan 26, 2023
didrocks
reviewed
Jan 26, 2023
didrocks
requested changes
Jan 26, 2023
It makes for easier debugging
didrocks
approved these changes
Jan 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Passing a real file descriptor to WslLaunch does not write to a file for some reason, so I have to stick an intermediate pipe in there. This pull request does two things that should have probably been two separate PR, but became too intertwined:
Brought (*distro).Cmd closer to the standard library for more details.
WSL-401