Add a windows "known-paths" locator. - #14675
Conversation
Codecov Report
@@ Coverage Diff @@
## main #14675 +/- ##
=======================================
- Coverage 65% 65% -1%
=======================================
Files 552 552
Lines 25926 26026 +100
Branches 3678 3693 +15
=======================================
+ Hits 16918 16926 +8
- Misses 8315 8413 +98
+ Partials 693 687 -6
|
292d176 to
c79f3cd
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
You need to rebase. The API in index has changed a bit. |
Kartik Raj (karrtikr)
left a comment
Kartik Raj (karrtikr)
left a comment
There was a problem hiding this comment.
It seems we have a disagreement in the way testing is supposed to be done, I am concerned this will make testing the component as a whole harder.
c79f3cd to
ce255b1
Compare
Eric Snow (ericsnowcurrently)
changed the title
| async function* generator() { | ||
| try { | ||
| for await (const executable of findInterpretersInDir(entry)) { | ||
| const okay = await isValidAndExecutable(executable); |
There was a problem hiding this comment.
We should only be returning python.exe or python to avoid returning duplicates (as done in other locators using isStandardPythonBinary).
There was a problem hiding this comment.
That's what the reducer is supposed to do. Each low-level locator is responsible only for tracking any valid environment within its knowledge domain. They should be focused on the one job and leave any other responsibilities to composite locators.
There was a problem hiding this comment.
The reducer doesn't resolve symlinks. cc Karthik Nadig (@karthiknadig)
We decided to delegate the responsibility to the locators as they know best regarding that. (for eg. windows store locators symlinks for the same env maybe located in different folders, so there's no way for the reducer to resolve that, hence the locator itself makes sure to not return symlinks).
There was a problem hiding this comment.
Kartik Raj (@karrtikr) is right. Depending on the context, which the individual locator have, we may or may not want to resolve symlinks.
There was a problem hiding this comment.
This has been addressed.
There was a problem hiding this comment.
So I should be checking for symlinks? How does that work on Windows?
There was a problem hiding this comment.
Hmm, I guess I'm still not clear on why ReducingLocator wouldn't take care of this. Is it just that we want to favor python.exe when it is in the same directory as python3.8.exe? Why wouldn't we do that in the reducer (via a call to some helper)?
There was a problem hiding this comment.
We can address the question of a better approach separately. I'll use isStandardPythonBinary() for now.
There was a problem hiding this comment.
Is it just that we want to favor python.exe when it is in the same directory as python3.8.exe? Why wouldn't we do that in the reducer (via a call to some helper)?
We don't want to do it for every locator, for eg. in Windows store we prioritize python3.8.exe over python.exe, hence logic is specific to the locator and not the reducer.
c82acb6 to
6faa56b
Compare
|
|
||
| constructor() { | ||
| const dirLocators: DirFilesLocator[] = getSearchPathEntries() | ||
| .map((dirname) => new DirFilesLocator(dirname, PythonEnvKind.Unknown)); |
There was a problem hiding this comment.
This should probably be marked as PythonEnvKind.Global instead.
There was a problem hiding this comment.
It could be System, OtherGlobal, Custom, Venv, or really any of them. We simply do not know. Hence Unknown.
There was a problem hiding this comment.
If it's Venv it should be resolved in the reducer as it'll also be discovered by Venv locators. If we're not able to distinguish between System, OtherGlobal, Custom for now, we should just go with System IMO. Having an Unknown type could mean anything.
There was a problem hiding this comment.
Note we also had an OKR to move away from Unknown type.
There was a problem hiding this comment.
We currently also detect in as Unknown, so nevermind.
There was a problem hiding this comment.
This locator should not be responsible for guessing/determining the environment's kind. That is the responsibility of kind-specific locators. So ultimately every env that this locator identifies should also be identified by at least one other locator that can determine the kind. If we end up keeping this locator in the long term then it will only be as a fallback when we are missing the appropriate locator. Regardless, rather than a dedicated $PATH-based locator we should have a $PATH helper that the kind-specific locators use. If we kept a generic $PATH-based locator then it would just be a light wrapper around that helper.
(FWIW, all the same things apply to any generic Windows registry locator, and probably a couple of others.)
All that said, we have diverged a bit from the kind-specific focus of locators and this PR is not the place to fix that. 😄
Karthik Nadig (karthiknadig)
left a comment
Karthik Nadig (karthiknadig)
left a comment
There was a problem hiding this comment.
Look good to me. Approving assuming Kartik's comments will be addressed.
Kartik Raj (karrtikr)
left a comment
Kartik Raj (karrtikr)
left a comment
There was a problem hiding this comment.
LGTM once the filter comment is addressed.
b87cc62 to
68151a3
Compare
Co-authored-by: Eric Snow <[email protected]>
…#14675) (microsoft#14951)" This reverts commit 651a6b9.
…#14675) (microsoft#14951)" (microsoft#14968) This reverts commit 651a6b9.
4 participants
Note that all but ~100 lines of this PR is tests.
[ ] Has a news entry file (remember to thank yourself!).[ ] Has telemetry for enhancements.[ ] Test plan is updated as appropriate.[ ]package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).[ ] The wiki is updated with any design decisions/details.