Do not go beyond the root when searching for a previously installed tool Fixes #40655 #40800
Conversation
dotnet-issue-labeler
bot
added
Area-Tools
untriaged
| var parseResult = Parser.Instance.Parse("tool install dotnetsay"); | ||
| new ToolInstallLocalCommand(parseResult, runtimeJsonPathForTests: ridGraphPath).Execute().Should().Be(0); | ||
|
|
||
| Directory.SetCurrentDirectory("/tmp/folder/sub"); |
There was a problem hiding this comment.
I'm not sure I fully understand this test/bug, may you please expand upon how this tests that the tool gets installed if a tools-manifest sets isRoot to true? Based on what I read, I think the bug is that the tool does not get installed locally because the tool is already installed in a parent directory. But we don't want to search the parent directory if there's a tools-manifest with isRoot is true, so it should get installed a 2nd time.
How does tools-manifest gets created within this test/is it mimicking this behavior somehow?
There was a problem hiding this comment.
I mimicked the bug in this case:
- Create a tool-manifest (line 49)
- Install dotnetsay (line 50-51)
- Create another tool-manifest, in the subfolder this time (line 54)
- Install dotnetsay again (it should be in the subdirectory this time if it succeeds) (line 55-56)
- Run dotnetsay (line 58)
You're correct about what the bug is—more specifically that it doesn't install the tool if it's installed in a parent directory even if there's a tools-manifest specifying isRoot between them. So this test creates a tools manifest via dotnet new tools-manifest in the parent directory and installs dotnetsay there. When we make another tool-manifest in the subdirectory, it should no longer be able to find dotnetsay from the subdirectory, but we install it again, and it works.
Does that make sense?
There was a problem hiding this comment.
Thank you, that was well written. I am not sure if that's the desired behavior or not. My guess is that this wouldn't work in 8.0.100 if you did not set isRoot, because it probably shouldn't replicate the install if isRoot is not set to true? In this case, I think the test is testing that this works regardless of whether or not isRoot is set, but I would expect it to only work if it was set even before the 8.0.200 change. I could be wrong!
There was a problem hiding this comment.
Does 8.0.100 default isRoot to false? If so, then you're correct: it wouldn't (and shouldn't) replicate the install, and this test wouldn't be too useful. I wasn't aware that the default changed so recently, but that's good to keep in mind. I don't think this is likely to be backported to 8.0.100, but if it is, we should definitely make sure to update the test to explicitly set isRoot to true partway through. Thanks for noticing that!
There was a problem hiding this comment.
I'm not sure about the 8.0.100 behavior. Based on what #40655 @havranek1024 said it sounds like it defaults to false. May you try it out please and see?
There was a problem hiding this comment.
I just tried out 8.0.107-dev (i.e., the current tip of the release/8.0.1xx branch), and it defaulted isRoot to true.
There was a problem hiding this comment.
Thanks for trying that out, apologies for my delay.
|
Ping @nagilson |
Forgind
requested review from
a team,
ericstj,
joperezr,
tmat,
arunchndr,
AntonLapounov and
sharwell
as code owners
Forgind
requested review from
lewing,
radical,
pavelsavara,
maraf and
vijayrkn
as code owners
Forgind
removed request for
a team,
radical,
lewing,
tmat,
pavelsavara,
sharwell,
vijayrkn,
ericstj,
maraf,
joperezr,
AntonLapounov and
arunchndr
Fixes #40655
dotnet tool install currently looks for a previously installed tools manifest. It reads manifests it finds in looking for one that has the manifest it's trying to install but does not check whether that manifest 'isRoot', which means it keeps going outside the root.
We don't use a tool beyond the root, which means we can't install then use a tool as would be expected.
@nagilson, can you take a look at this? I don't know all that much about dotnet tool.