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

sln-add: Support for slnx #44570

Merged
merged 64 commits into from
Dec 6, 2024
Merged

sln-add: Support for slnx #44570

merged 64 commits into from
Dec 6, 2024

Conversation

edvilme
Copy link
Member

@edvilme edvilme commented Oct 31, 2024

Contributes to #40913

The dotnet CLI should support the new slnx format for building and in the existing solution management commands. It should also help interested users migrate to the new format.

This adds dotnet sln add support for .slnx files

@edvilme edvilme force-pushed the edvilme-slnx-add branch 2 times, most recently from d397257 to b6aa066 Compare November 5, 2024 20:55
@kasperk81
Copy link
Contributor

@edvilme checkout the complete feedback. using solution.FindProject makes the regex usage irrelevant

Copy link
Contributor

dotnet-policy-service bot commented Dec 3, 2024

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

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.

Excellent work here. I think you did a great job looking at the solution persistence code and implementing this. This will likely cause several breaking changes, which should be documented as mentioned in our comments above and also prior discussions with the SLN team. Also, thanks to @/richardstanton for doing some calls with us on this to determine the desired behavior, and @kasperk81 / others for helping!

Please do follow up with the remaining comments before merging -- and it would be ideal to get a review from the SLNX side, but not necessary. I'm approving because everything has been tentatively accounted for and the changes look good (don't want to block this for eternity) but it is important that the remaining items have some sort of resolution before merging. (Also, I will be gone soon and I want to give you a thumbs up while I'm still here.)

@edvilme
Copy link
Member Author

edvilme commented Dec 3, 2024

This will likely cause several breaking changes, which should be documented as mentioned in our comments above and also prior discussions with the SLN team.

Yes, I will review those. For now, I think the breaking changes would be:

  • projects, solution folders, (and soon loose files) cannot longer have invalid names;
  • the command will no longer fail when attempting to add a nested project with the same name but different path;
  • default configuration for solution files will be adding Any CPU, x86 and x64 platforms, and Debug and Release build types (I will try researching a bit more on that)

@edvilme
Copy link
Member Author

edvilme commented Dec 3, 2024

Excellent work here. I think you did a great job looking at the solution persistence code and implementing this. This will likely cause several breaking changes, which should be documented as mentioned in our comments above and also prior discussions with the SLN team. Also, thanks to @/richardstanton for doing some calls with us on this to determine the desired behavior, and @kasperk81 / others for helping!

Thank you, and thanks for all your support ant the thorough review. Likewise thanks to @kasperk81 and @/richardstanton for all your support here :)

@edvilme edvilme merged commit 80c7725 into dotnet:main Dec 6, 2024
38 checks passed
@kasperk81
Copy link
Contributor

The dotnet sln add command should avoid generating project GUIDs for .slnx files because .slnx is a path-based format.

  • When the project file contains <ProjectGuid>:

    • Include the GUID in .slnx, but ensure it is in uppercase.
    • Currently, .slnx uses lowercase GUIDs, while .sln uses uppercase. This inconsistency should be resolved.
  • When <ProjectGuid> is absent:

    • Use the default path-based format without including any GUID (Id) in the .slnx entry.

The current behavior auto-generates GUIDs and inserts them in entries like <Project Path="{path}" Id="{guid}" />. This is unnecessary because GUIDs are optional in .slnx, intended only for legacy support. The path-based format was specifically requested by developers for over 10 years (see this discussion: dotnet/msbuild#1730). Visual Studio developers eventually implemented the .slnx format to support path-based solutions without forcing GUIDs.

In short, the SDK should not force GUIDs when the community prefers the cleaner, GUID-free path-based approach.

@edvilme
Copy link
Member Author

edvilme commented Dec 10, 2024

/backport to release/9.0.2xx

Copy link
Contributor

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

Copy link
Contributor

@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: [IMP] sln-list: Support for slnx
Using index info to reconstruct a base tree...
M	src/Cli/dotnet/commands/dotnet-sln/LocalizableStrings.resx
M	src/Cli/dotnet/commands/dotnet-sln/list/Program.cs
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.cs.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.de.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.es.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.fr.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.it.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.ja.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.ko.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.pl.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.pt-BR.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.ru.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.tr.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.zh-Hans.xlf
M	src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.zh-Hant.xlf
Falling back to patching base and 3-way merge...
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.zh-Hant.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.zh-Hant.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.zh-Hans.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.zh-Hans.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.tr.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.tr.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.ru.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.ru.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.pt-BR.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.pt-BR.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.pl.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.pl.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.ko.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.ko.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.ja.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.ja.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.it.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.it.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.fr.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.fr.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.es.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.es.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.de.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.de.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.cs.xlf
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/xlf/LocalizableStrings.cs.xlf
Auto-merging src/Cli/dotnet/commands/dotnet-sln/list/Program.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/list/Program.cs
Auto-merging src/Cli/dotnet/commands/dotnet-sln/LocalizableStrings.resx
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-sln/LocalizableStrings.resx
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 [IMP] sln-list: Support for slnx
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@Bartleby2718
Copy link

@edvilme @kasperk81 @nagilson @baronfel @Forgind I'm thinking of picking up #45072 again, but I want to make sure we're aligned on the approach.

First, my personal preferences:

  1. I prefer using giving descriptive variable names and using the latest syntax (e.g. collection expressions).
  2. I prefer small, incremental PRs as they are easier to understand and review.

Problems for each:

  1. These style changes can be distracting for reviewers.
  2. A lot of PRs get merged to this repository, so merge conflicts (e.g. in resx files) can happen, which isn't the end of the world but can be annoying for me to resolve.

Do you want a big PR (like #45072 currently is) that includes style changes, or do you want a few small PRs (perhaps with style/resx changes coming first)?

@edvilme
Copy link
Member Author

edvilme commented Dec 17, 2024

Hello @Bartleby2718
Firstly thanks for your interest in this, I believe this will be very useful functionality.

Regarding the PRs I agree that atomic PRs might be easier to review and develop than massive PRs. However my personal suggestion would be to find something in the middle and making sure those are independent, as to avoid having a lot of merge conflicts, backports and reviews on different PRs.
I would recommend maybe creating different PRs dotnet sln add folder, and dotnet sln add file as they would be independent functionalities.

As for the style, whatever works best for you, but due to the size of the repo, I would suggest attaching to the code conventions used as it makes it easier to maintain and contribute to in the future.

@marcpopMSFT marcpopMSFT added this to the 9.0.2xx milestone Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CLI Area-VS breaking-change Using this label will notify dotnet/compat and trigger a request to file a compat bug needs-breaking-change-doc-created Review for breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants