Add tye.yml VS Solution Items on init #568
Conversation
|
When doing From a developer point of view "First sln match" is
I understand that it should not "fail", maybe a warning while still creating
|
|
Agree with @tebeco here, I think we should not add it to any slns when multiple. |
Agreed.Accepting change here. Co-authored-by: Justin Kotalik <[email protected]>
There was a problem hiding this comment.
Manually editing a sln file seems quite error prone. What kind of testing has been done to verify this PR?
Ideally we would use a library like Microsoft.DotNet.Cli.Sln.Internal which is available on this feed. The problem here is that the TFMs are incompatible but I'm curious if we can update the TFM of the tool to net5.0 @jkotalik ?
src/tye/InitHost.cs
Outdated
| // assume the first one is the one wanted. | ||
| var sln = slnFiles[0]; | ||
| //format | ||
| var insertItem = @$"Project(""{fileGuid}\"") = ""Solution Items"", ""Solution Items"", ""{fileGuid}"" |
There was a problem hiding this comment.
A few things here:
- Why is there a backslash
\after fileGuid? Looks like that's not needed since you already have""which should resolve correctly to a quotation mark. - How did you come up with this format for insert Item? Specifically, from my understanding of this doc the two Guids should not be the same, one is a project Guid, one is a project type Guid. The current logic creates a random Guid and uses it for both, which doesn't seem right. Have you tested this logic? What does the sln file look like after you open it?
- This logic assumes it's safe to insert a folder called
Solution Items, what if that folder already exists in the sln file?
There was a problem hiding this comment.
I tested it out and it seems like adding this section worked:
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution Items", "{D85F58E9-CD8F-45D7-B2F2-ADAE5812E9C0}"
+ ProjectSection(SolutionItems) = preProject
+ tye.yaml = tye.yaml
+ EndProjectSection
+EndProject
Still needs logic to resolve whether Solution Items already exists in the sln file.
There was a problem hiding this comment.
Thanks @JunTaoLuo for the comments here. The \ was a miss on the last edit, will fix this and I'll make a quick change to check for the existing "Solution Items".
|
|
||
| lines.ForEach(l => | ||
| { | ||
| if (l.IndexOf("(SolutionItems)") > 0) |
There was a problem hiding this comment.
This logic doesn't work, (SolutionItems) may be specified in other itemgroups. For example in gRPC projects, often protobuf files are included as solution items under Proto directory e.g. here, this logic will add tye.yaml to that directory which wouldn't make any sense. You'll need to look for a project with the name Solution Items.
| { | ||
| var fileGuid = Guid.NewGuid().ToString("B").ToUpper(); | ||
|
|
||
| insertItem = @$"Project(""{fileGuid}"") = ""Solution Items"", ""Solution Items"", ""{fileGuid}"" |
There was a problem hiding this comment.
No this is still wrong, the two Guids have different purposes and should not be the same. Again, how are you verifying this change? I think if you opened the sln file, there will be a load error.
There was a problem hiding this comment.
The SolutionItems is a good call out here. Testing process has been using the simple microservices.sln example from the docs. Opening the sln in VS works fine. There is no loading error.
Ideally, there should be dotnet sln add functionality in the dotnet CLI to support this beyond **proj files.
There was a problem hiding this comment.
When I test this locally, I see
It's because the project type Guid is invalid if you just use a randomly generated Guid.
There was a problem hiding this comment.
And I agree, maybe file an issue here https://github.com/dotnet/sdk/issues for dotnet sln to have the ability to add solution file? Also this is why I mentioned that it would be better to use the Microsoft.DotNet.Cli.Sln.Internal library if we plan to update our TFM.
There was a problem hiding this comment.
Resolved the Guid issue. There is a section Guid, project references should have the same in the first and then an identifier for the second. Solution items are their own section. Taking a dependency on on dotnet cli commands would be preferred over Microsoft.DotNet.Cli.Sln.Internal in the future.
Added issue :dotnet/sdk#12454
|
Closing for now. This is getting messy and not providing value, might revisit. Also found old backlog item for adding sln non-project items in CLI - dotnet/sdk#9611 @JunTaoLuo |
Closes #372