Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix bug when adding solution folder containing .. #46456

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

edvilme
Copy link
Member

@edvilme edvilme commented Feb 1, 2025

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2321742

When attempting to add a project in a parent directory (e.g., dotnet sln add ../foo/bar.csproj), the tool automatically generates solution folder ../foo. vs-solutionpersistence generates folders and subfolders dynamically, causing a folder .. to be created.

When adding projects from outside the solution directory, the tool should not create solution folders.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-CLI untriaged Request triage from a team member labels Feb 1, 2025
@edvilme edvilme changed the title Fix bug when adding solution folder containing .. Fix bug when adding solution folder containing .. Feb 1, 2025
@edvilme edvilme requested a review from a team February 1, 2025 01:10
@Forgind
Copy link
Member

Forgind commented Feb 3, 2025

Did we finalize the design on this, or is it still being discussed?

@edvilme
Copy link
Member Author

edvilme commented Feb 3, 2025

Rebasing to previous commit as it matches behavior on 9.0.1xx (before transition to vs-solutionpersistence) as to avoid regressions.
image

@edvilme edvilme force-pushed the edvilme-sln-add-parent branch from a0507b7 to c043f68 Compare February 3, 2025 21:30
@edvilme edvilme requested a review from Forgind February 3, 2025 21:31
Copy link
Member

@MiYanni MiYanni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very confused here. .. is a valid path navigator, just like ..

  • . is current working directory
  • .. is the parent of the current working directory

I looked at the work item, and them specifying ..\RazorClassLibrary1\RazorClassLibrary1.csproj is completely normal use case. In English, it means "From the directory above the current directory, go into the RazorClassLibrary1 folder and use the RazorClassLibrary1.csproj file."

@edvilme
Copy link
Member Author

edvilme commented Feb 4, 2025

@MiYanni
Yes. "../foo/bar.csproj" is a valid project path, but it should not create a solution folder as "../foo" is not a valid solution folder name.

Relative path gets resolved correctly, but the issue occurs when dynamically generating solution folders (ie, not actual disk folders)

Hence, the expected behavior is that when adding projects from parent directories, no solution folder should be generated. This mimics previous behavior from 9.0.1xx

Copy link
Member

@MiYanni MiYanni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, now I understand the situation. I completely misunderstood that "solution folder" meant "the feature for solutions to contain virtual folders" and not "the folder a solution is in".

I guess in this situation, the user could intend to have a project defined in a solution folder, correct? I'm still slightly confused that there is a feature to automatically create a solution folder based on the path provided. But I don't know this CLI interface at all.

Either way, I can see this is a very targeted fix, but why is .. special in this situation? Couldn't you just check to see if this is a valid relative path? I think we have SDK methods for that.

@edvilme
Copy link
Member Author

edvilme commented Feb 4, 2025

Either way, I can see this is a very targeted fix, but why is .. special in this situation? Couldn't you just check to see if this is a valid relative path? I think we have SDK methods for that.

Yes, by default the CLI generates solution folders automatically when adding a project based on its path. Hence, dotnet sln add ../path/to/project.csproj would generate solution folder(s) ../path/to. .. is an invalid solution folder name under vs-solutionpersistence (see here), which is why in that specific case, the CLI should not generate a solution folder. This behavior existed under the old serializer for what I assume to be similar reasons

There are other situations in which users might create solution folders with invalid names that we currently don't handle, but this is a special case because we still want to support adding projects from parent directories

Copy link
Member

@MiYanni MiYanni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! I looked the docs on add to get a better understanding. I see now. Given all that, this change looks good.

@edvilme
Copy link
Member Author

edvilme commented Feb 4, 2025

Thanks for the explanation! I looked the docs on add to get a better understanding. I see now. Given all that, this change looks good.

Glad it helped, thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area-CLI untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants