WorkloadRestoreCommand.DiscoverAllProjects: Return full paths

[edvilme]

Fixes #47390

When running dotnet workload restore ../foo.sln, the project paths in the sln file are evaluated based on the current directory instead of the sln directory. To avoid this, WorkloadRestoreCommand.DiscoverAllProjects should return full paths instead of relative paths

[nagilson]

This change looks good to me, nice find! It would be nice to have a test, though I understand this is a small, well-scoped problem and honestly a quirky bug that would not necessarily happen again if coded correctly. Curious what others think on the importance of a test.

[Forgind]

This doesn't look unreasonable, but if this worked in 9.0.100, can you explain what regressed this? I suspect this may paper over a problem instead of resolving one.

[nagilson]

This doesn't look unreasonable, but if this worked in 9.0.100, can you explain what regressed this? I suspect this may paper over a problem instead of resolving one.

That's a good point. I assume it's possible that a change to the sln library caused this in 9.0.200, but I don't really know enough to say so I'm curious about this too.

[edvilme]

This doesn't look unreasonable, but if this worked in 9.0.100, can you explain what regressed this? I suspect this may paper over a problem instead of resolving one.

It is not related to vs-solutionpersistence itself, but rather to the refactorings done on #45442 to remove SlnFile 😟

[Forgind]

Can you be more specific on which part of that change led to this issue?

[edvilme]

@Forgind yes, instead of resolving the project paths to be absolute paths, the refactoring resolved the relative paths instead.

[Forgind]

Do you need to have filtered out SolutionFolders there?

Also, I think that instead of this fix, we should instead make that return absolute paths. Otherwise I'd be concerned that other places may be affected.

[edvilme]

Do you need to have filtered out SolutionFolders there?

In the old parser, a Solution Folder is considered a special type of project. In this case.

var projects = solutionFilesInOrder.Where(p => p.ProjectType != SolutionProjectType.SolutionFolder);

is equivalent to

var projects = solutionFile.SolutionProjects

in the new parser

Also, I think that instead of this fix, we should instead make that return absolute paths. Otherwise I'd be concerned that other places may be affected.

You mean p.FilePath? SlnFileFactory.CreateFromFileOrDirectory returns a SolutionProjectModel which is owned by the vs-solutionpersistence team. Also, it returns relative paths as they are stored that way in the solution files

[Forgind]

You mean p.FilePath? SlnFileFactory.CreateFromFileOrDirectory returns a SolutionProjectModel which is owned by the vs-solutionpersistence team. Also, it returns relative paths as they are stored that way in the solution files

Yeah, so instead of p.FilePath, it would be like Path.GetFullPath(p.FilePath) or whatever.

[Forgind]

I talked with edvilme a bit more offline, and we looked into other places where this gets used, and they should all be fine with the relative or absolute paths, so this change should suffice to resolve the change in behavior, so I'm satisfied. Thanks for fixing this!