fix: resolve rustup from HOME when not in PATH - #99
Conversation
| environment: environment, | ||
| ); | ||
| } on ProcessException catch (e) { | ||
| lastException = e; |
There was a problem hiding this comment.
Can we differentiate on when it was an executable not found error vs some other error? That’d be better than just blindly using the last exception seen
There was a problem hiding this comment.
Sure. I hesitated with this, but ultimately decided I wasn't sure what would be the more valuable error. Generally I think you'd want to see that PATH rustup was not found, but that's not necessarily always true.
What do you think this should look like?
There was a problem hiding this comment.
I was envisioning an edge case where normal rustup failed to run via network issue or something of the like, and then later on we run into an “executable not found” and then the last encountered error is a red herring.
There was a problem hiding this comment.
So ideally, the actual error (if there was one), followed by the binary not found error as a last resort
There was a problem hiding this comment.
I don't think that scenario can occur. The only time we will see a ProcessException is when we can't find the process or something of that caliber. Any other types of errors would have a non-zero exit code, thus creating a RustProcessException and ignoring our catch statement (as it was before my change)
|
Thanks for the PR. For a change like this probably fine to skip tests as long as it was working for you |
67f48ce to
ec24880
Compare
That particular problem may be solved in the future with dart-lang/native#3545 |
3 participants
Fixes #95
I couldn't come up with a nice way to test this due to how Dart restricts environment mutation for tests. If you want me to add tests, let me know.