[hooks_runner] Enforce absolute paths for executables in runProcess - #3480
Conversation
PR HealthAPI leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with |
dcharkes
left a comment
dcharkes
left a comment
There was a problem hiding this comment.
Thanks for the quick response @jakobkordez! 🙏
| required Uri? workingDirectory, | ||
| }) { | ||
| final executablePath = executable.toFilePath(); | ||
| if (workingDirectory == null) return executablePath; |
There was a problem hiding this comment.
Also early return if platform is not Windows?
There was a problem hiding this comment.
So basically we cannot do the right thing if (1) the path has spaces, (2) the path is relying on PATHEXT, and (3) we're trying to run with a relative path.
Do I understand this correctly?
I think that's probably fine, I think users relying on PATHEXT is a bad idea anyways.
There was a problem hiding this comment.
Within hooks_runner, I think we should only be running absolute paths and not relying on PATH EXT. And if we do, we should fix that.
Within dartdev tests we were running tests that explicitly test PATH EXT and relative paths. And not paths with spaces.
I think it's probably the wrong thing to try to share the implementation if we can't make the implementation universally doing the right thing. So, I'm looking at stopping to rely on this internal lib/src/ function in those tests. https://dart-review.googlesource.com/c/sdk/+/525220
There was a problem hiding this comment.
And we may also remove that TODO to share it between packages. Maybe the needs inside native_toolchain_c differ again from the needs in hooks_runner
There was a problem hiding this comment.
Do I understand this correctly?
Yes
I think it's probably the wrong thing to try to share the implementation
I agree
I think the function should be simplified - revert #3472 and not merge this PR. Let me know how you want me to proceed.
There was a problem hiding this comment.
Okay maybe the best way is to indeed revert it. And add a bunch of checks that throw exceptions if (1) the path is not absolute, and (2) doesn't end with an executable with a file extension. And then update the documentation to say that the function supports spaces in executable and does not support relative paths and PATHEXT.
We just need to check that we don't have any tests in package hooks_runner that try to use it.
The package itself should work with absolute paths and no path-ext because we only (1) invoke the Dart compiler and (2) invoke the kernel-files that contain the hook code. Those should work with the absolute Dart path that's passed in. final Uri dartExecutable; And we should also check that that's absolute in the constructor.
I think that's the cleanest approach. WDYT?
I've already landed https://dart-review.googlesource.com/c/sdk/+/525220, so if the CI is green here on GitHub, it should not go red again on the Dart CI.
(Helicopter view: Too much code sharing is bad, because you get too many conflicting requirements and non-local code breakages. You live you learn!)
There was a problem hiding this comment.
Sounds good. Do you want a new PR or do I just continue in this one: make two git reverts and proceed with the other changes
There was a problem hiding this comment.
Either approach is fine 👍
…r Windows" This reverts commit ac44bb1.
…art-lang#3472)" This reverts commit 7b299f3.
…nd update documentation
|
Maybe the PR title needs to be updated as well (squash on merge will take the PR title as commit message). |
jakobkordez
changed the title
2 participants
@dcharkes
Description
Fix running a relative executable path with a
workingDirectoryon WindowsRelated Issues
PR Checklist
dart tool/ci.dart --alllocally and resolved all issues identified. This ensures the PR is formatted, has no lint errors, and ran all code generators. This applies to the packages part of the toplevelpubspec.yamlworkspace.CHANGELOG.mdfor the relevant packages. (Not needed for small changes such as doc typos).