Permit Invalid Symlink Folders to Work as Custom Path Setting #2098
Conversation
For dotnet/vscode-csharp#7925 (comment) We have a custom setting and could cause C# to fail if someone sets the setting to a symlink install which doesn't actually have a folder in it, even if it has a valid executable because DOTNET_ROOT wants a folder. Read more below on how this fix works and why. # Snap (after installing via package manager and uninstalling, then using snap) Interesting, the symlink which could be set as /usr/lib/dotnet in this case doesn't work as an executable with snap when we try to find the executable due to how its placed (/usr/lib/dotnet/dotnet does not exist.) But our code uses custom snap logic to make it work in a different method. # Package Manager However with a package manager, /usr/lib/dotnet is the actual location and /usr/lib/dotnet/dotnet is a valid executable, so this works. /usr/bin/dotnet is also a valid executable, and that is the symlink to the lib folder, so our logic accepts it, but then it's not valid for DOTNET_ROOT. I didnt know DOTNET_ROOT required the folder, so thank you for that context @dibarbet. Our path logic tries to not do realpath first, and this would normally have been caught so we'd give the correct path; its OK because we call --list-runtimes and go 2 directories up. But we didn't do that for the user setting path in an attempt to respect what they set. # Solution This should be fixed in our extension. I dont know if I'd call it a 'bug' per se, but we can make the behavior better. They shouldnt have to know all of this nuance. This means we could a - call realpath on /usr/bin/dotnet (or the user setting) b - call --list-runtimes on /usr/bin/dotnet (the user setting) and go 2 directories up to find the actual path both would cause us to return path.join(/usr/lib/dotnet, dotnet), however, realpath already returns the executable, while list--runtimes parsing would point us only to /usr/lib/dotnet/shared/blah. a and b have similar ish performance characteristics because they both need to spawn a shell (most expensive perf), but --list-runtimes is going to be cached in the future so that theoretically means list-runtimes is faster as it wont need to spawn a shell. I guess we could cache realpath too, so performance isnt much of a concern. b is better in my opinion, as a would actually break with a snap install. realpath of snap/bin/dotnet is /usr/bin/snap, which is not a valid executable, so it wouldn't be found. since it follows the flow of the other code in our extension to go 2 dirs up at the end of the call. Users do some (wild) stuff and if they removed realpath from their system we'd also be cooked in that situation, not that its a high priority.
|
This is ready for review, Linux test failure is pending a fix in #2096 and not failing due to this change |
There was a problem hiding this comment.
I'd personally have gone with the realPath solution because that's what MSBuildLocator uses, and I think it's a better assumption that users aren't going to delete realPath than that users won't have unexpectedly changed their folder structure, but I'm not going to block on that since you did some testing, and this seems to work. Weird linux distros may break, but if a distro is weird enough with a small enough user base, at some point it's asking to break.
| @@ -3,6 +3,7 @@ | |||
| * The .NET Foundation licenses this file to you under the MIT license. | |||
| *--------------------------------------------------------------------------------------------*/ | |||
| import * as chai from 'chai'; | |||
| import * as os from 'os'; | |||
There was a problem hiding this comment.
Can test code be moved next to other test code?
There was a problem hiding this comment.
I'm not sure what you mean - maybe you mean if this import can be added to when it needs to be used? The typical style is to put them at the top. But probably the answer is yes
Approach-A would cause a lot of issues with snap installations and while it is a more obscure linux situation, a lot of this work was done to try to support a large number of edge cases, so I think it's worth doing in this case. Snap is not a distro but an installation mechanism/paradigm on ubuntu which we are supposed to support. I respect that opinion, though and see what you mean 👀 |
|
Merging on red since the other PR to fix the linux leg needs to be merged. |
For dotnet/vscode-csharp#7925 (comment)
We have a custom setting and could cause C# to fail if someone sets the setting to a symlink install which doesn't actually have a folder in it, even if it has a valid executable because DOTNET_ROOT wants a folder. Read more below on how this fix works and why.
Works with snap setting set:
Works with Feed Set to Symlink:
/usr/bin/dotnet (which is really /usr/lib/dotnet/)
Snap (after installing via package manager and uninstalling, then using snap)
Interesting, the symlink which could be set as /usr/lib/dotnet in this case doesn't work as an executable with snap when we try to find the executable due to how its placed (/usr/lib/dotnet/dotnet does not exist.)
But our code uses custom snap logic to make it work in a different method.
Package Manager
However with a package manager, /usr/lib/dotnet is the actual location and /usr/lib/dotnet/dotnet is a valid executable, so this works. /usr/bin/dotnet is also a valid executable, and that is the symlink to the lib folder, so our logic accepts it, but then it's not valid for DOTNET_ROOT. I didnt know DOTNET_ROOT required the folder, so thank you for that context @dibarbet.
Our path logic tries to not do realpath first, and this would normally have been caught so we'd give the correct path; its OK because we call --list-runtimes and go 2 directories up. But we didn't do that for the user setting path in an attempt to respect what they set.
Solution
This should be fixed in our extension.
I dont know if I'd call it a 'bug' per se, but we can make the behavior better. They shouldnt have to know all of this nuance.
This means we could
a - call realpath on /usr/bin/dotnet (or the user setting) b - call --list-runtimes on /usr/bin/dotnet (the user setting) and go 2 directories up to find the actual path
both would cause us to return path.join(/usr/lib/dotnet, dotnet), however, realpath already returns the executable, while list--runtimes parsing would point us only to /usr/lib/dotnet/shared/blah.
a and b have similar ish performance characteristics because they both need to spawn a shell (most expensive perf), but --list-runtimes is going to be cached in the future so that theoretically means list-runtimes is faster as it wont need to spawn a shell. I guess we could cache realpath too, so performance isnt much of a concern.
b is better in my opinion, as a would actually break with a snap install. realpath of snap/bin/dotnet is /usr/bin/snap, which is not a valid executable, so it wouldn't be found. since it follows the flow of the other code in our extension to go 2 dirs up at the end of the call. Users do some (wild) stuff and if they removed realpath from their system we'd also be cooked in that situation, not that its a high priority.