Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[ADD] dotnet solution migrate #44419

Merged
merged 20 commits into from
Oct 30, 2024
Merged

Conversation

edvilme
Copy link
Member

@edvilme edvilme commented Oct 23, 2024

Created dotnet solution migrate solutionfile.sln to generate a new solution file with the .slnx extension using https://github.com/microsoft/vs-solutionpersistence

Contributes to #40913 (dotnet sln migrate)

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-CLI untriaged Request triage from a team member labels Oct 23, 2024
@edvilme edvilme changed the title [ADD] dotnet sln: Migrate [ADD] dotnet solution: Migrate Oct 23, 2024
@edvilme edvilme marked this pull request as ready for review October 23, 2024 23:09
@edvilme edvilme requested a review from a team October 23, 2024 23:09
@kasperk81
Copy link
Contributor

kasperk81 commented Oct 24, 2024

unit tests upcoming?

Addresses #40913

I thought addressing that epic also includes:

  • dotnet new solution --format slnx (with alias -f slnx)
  • dotnet sln {add,remove,list} my.slnx my.csproj commands

@edvilme
Copy link
Member Author

edvilme commented Oct 24, 2024

unit tests upcoming?

Hello. Yes, this is still WIP and will have a unit test that verifies if the command work. Additional testing on how the conversion is done, is more related to the https://github.com/microsoft/vs-solutionpersistence project as we just consume their API

@kasperk81
Copy link
Contributor

btw, current main and 9.0.200 branch can "build" .slnx #40913 (comment), so the test can assert on that to cover migration.

you may also consider changing Addresses #40913 to Contributes to #40913 so it doesn't close the issue on merge for additional work (dotnet new sln, dotnet sln {add,remove,list} my.slnx my.csproj), and keep this PR for dotnet sln migrate only.

Directory.Packages.props Outdated Show resolved Hide resolved
Directory.Packages.props Outdated Show resolved Hide resolved
Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

The change looks good so far :) I've left some thoughts below. Definitely add some tests like mentioned above. Also, please keep the PR in a draft state if it's still a WIP so it's clear to reviewers.

@just-ero
Copy link

Hi, I just want to make sure: this command won't require the solution path to be provided, right? It will take the solution file present in the current directory if no path is specified?

After migrating (I feel like convert would have been a nicer name), does the original solution file get deleted? Renamed? Does it stay?

@edvilme edvilme marked this pull request as draft October 25, 2024 15:18
@edvilme edvilme changed the title [ADD] dotnet solution: Migrate [ADD] dotnet solution migrate (draft) Oct 25, 2024
Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

@just-ero raises a great point, many of our commands work if there is only one sln or project file in the directory where you don't need to specify a project or sln to take action on. Some of them even work if there are multiple and just picks one based on some order, like so:

string? potentialSln = Directory.GetFiles(arg, "*.sln", SearchOption.TopDirectoryOnly).FirstOrDefault();

I don't think this was in the original spec but we should support that. I am very familiar with the code to do that, so if you have questions let me know :)

@nagilson
Copy link
Member

nagilson commented Oct 25, 2024

After migrating (I feel like convert would have been a nicer name), does the original solution file get deleted? Renamed? Does it stay?

@baronfel? I prefer 'migrate' personally and would keep the original solution file, deleting the file may be unexpected and cause pain there but it could be an option to provide.

@edvilme
Copy link
Member Author

edvilme commented Oct 25, 2024

After migrating (I feel like convert would have been a nicer name), does the original solution file get deleted? Renamed? Does it stay?

@baronfel? I prefer 'migrate' personally and would keep the original solution file, deleting the file may be unexpected and cause pain there but it could be an option to provide.

I also prefer migrate and definitely think we should keep the original solution file at least until the rest of the dotnet sln commands are ready. Otherwise people would run the command with no way of undoing it and things could get messy (?)

I would advocate towards having a --replace flag and additional confirmation from the user if that was the path we wanted to follow

@just-ero
Copy link

string? potentialSln = Directory.GetFiles(arg, "*.sln", SearchOption.TopDirectoryOnly).FirstOrDefault();

Maybe this command should panic when more than one solution file is found?

Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com>
@baronfel
Copy link
Member

I've added details to the migration section on the linked issue. We should not delete the pre-existing .sln file, users are free to do that once they have tested and verified the slnx. solves their needs. We should use our existing solution-discovery mechanisms like we do for the other dotnet sln commands. We should error out if there are multiple .sln files in a directory that is used and no explicit sln argument is provided.

@nagilson
Copy link
Member

string? potentialSln = Directory.GetFiles(arg, "*.sln", SearchOption.TopDirectoryOnly).FirstOrDefault();

Maybe this command should panic when more than one solution file is found?

That is not how other commands behave, so I think its good to be consistent. This is the logic used by other commands.

@edvilme edvilme marked this pull request as ready for review October 29, 2024 00:01
@edvilme edvilme requested review from baronfel and nagilson October 29, 2024 00:01
@edvilme edvilme changed the title [ADD] dotnet solution migrate (draft) [ADD] dotnet solution migrate Oct 29, 2024
@edvilme
Copy link
Member Author

edvilme commented Oct 29, 2024

Hello @dotnet/product-construction I am trying to add package vs-solutionpersistence but got a strong name error. Is this already signed? Or should I be doing it differently?

CC: @surayya-MS

@kasperk81
Copy link
Contributor

to unblock, we can add this line to dotnet.csproj

@edvilme
Copy link
Member Author

edvilme commented Oct 29, 2024

to unblock, we can add this line to dotnet.csproj

I wouldn't choose to do that as that would not address the issue but mostly mask it, and could potentially cause issues in the future

@kasperk81
Copy link
Contributor

you mean there are potential issues with Microsoft.NET.Build.Containers 1.3M downloads https://www.nuget.org/packages/Microsoft.NET.Build.Containers/8.0.403? ok, if you say so..

@kasperk81
Copy link
Contributor

Co-authored-by: Noah Gilson <OTAKUPENGUINOP@GMAIL.COM>
Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

This is almost ready 🔥 I am so excited for this to go in, and I think a lot of people are :) Great work so far, the code looks clean. I have one more concern / comment, after that I think we can merge this.

Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

Well done, excited to see this go into the SDK 👏

I think we can merge this, but I would still suggest to have the API owner look over the call as well as mentioned above.

@edvilme
Copy link
Member Author

edvilme commented Oct 29, 2024

Hello @dotnet/source-build @richardstanton can I please get some help on the Strong Name requirement for Microsoft.VisualStudio.SolutionPersistence? Thx!

@richardstanton
Copy link

Hello @dotnet/source-build @richardstanton can I please get some help on the Strong Name requirement for Microsoft.VisualStudio.SolutionPersistence? Thx!

My understanding was that strong naming was generally deprecated in .NET Core as it is ignored in assembly loading. (https://github.com/dotnet/runtime/blob/main/docs/project/strong-name-signing.md) So it wasn't a priority when adding to source-build. (Note, the dll should still be authenticode signed which is important)

The dll built for the NuGet package is strong named signed, partially because it is built with the .NET Framework version that requires this, but also because it is built with a very different pipeline than source-build.

It seems like suppressing the warning might be ok if everything is .NET Core.

@ellahathaway
Copy link
Member

Hello @dotnet/source-build @richardstanton can I please get some help on the Strong Name requirement for Microsoft.VisualStudio.SolutionPersistence? Thx!

My understanding was that strong naming was generally deprecated in .NET Core as it is ignored in assembly loading. (https://github.com/dotnet/runtime/blob/main/docs/project/strong-name-signing.md) So it wasn't a priority when adding to source-build. (Note, the dll should still be authenticode signed which is important)

The dll built for the NuGet package is strong named signed, partially because it is built with the .NET Framework version that requires this, but also because it is built with a very different pipeline than source-build.

It seems like suppressing the warning might be ok if everything is .NET Core.

Agreed that the suppressing the warning is the way to go here.

@edvilme edvilme requested a review from nagilson October 30, 2024 16:50
Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

Looks good! I would check with @marcpopMSFT to see if we still have time to get this into 9.0.2xx and if there's any process to do so. (that was the goal, correct @baronfel?) Right now, this is targeting main, so this one you can merge regardless.

@edvilme edvilme merged commit c898476 into dotnet:main Oct 30, 2024
37 checks passed
@edvilme
Copy link
Member Author

edvilme commented Dec 6, 2024

/backport to release/9.0.2xx

Copy link
Contributor

github-actions bot commented Dec 6, 2024

Started backporting to release/9.0.2xx: https://github.com/dotnet/sdk/actions/runs/12204682295

Copy link
Contributor

github-actions bot commented Dec 6, 2024

@edvilme backporting to release/9.0.2xx failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: [ADD] dotnet sln: Migrate
Using index info to reconstruct a base tree...
M	Directory.Packages.props
M	src/Cli/dotnet/dotnet.csproj
Falling back to patching base and 3-way merge...
Auto-merging src/Cli/dotnet/dotnet.csproj
CONFLICT (content): Merge conflict in src/Cli/dotnet/dotnet.csproj
Auto-merging Directory.Packages.props
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 [ADD] dotnet sln: Migrate
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

edvilme added a commit to edvilme/sdk that referenced this pull request Dec 10, 2024
Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com>
Co-authored-by: Noah Gilson <OTAKUPENGUINOP@GMAIL.COM>
edvilme added a commit to edvilme/sdk that referenced this pull request Dec 11, 2024
Co-authored-by: kasperk81 <83082615+kasperk81@users.noreply.github.com>
Co-authored-by: Noah Gilson <OTAKUPENGUINOP@GMAIL.COM>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.

7 participants