-
Notifications
You must be signed in to change notification settings - Fork 12
Add --solution option to export compilations as buildable VS projects #323
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: jaredpar <[email protected]>
…o --solution Co-authored-by: jaredpar <[email protected]>
…d compliance Co-authored-by: jaredpar <[email protected]>
| var solutionContent = File.ReadAllText(solutionFile); | ||
| Assert.Contains("<Solution>", solutionContent); | ||
| Assert.Contains("<Project Path=", solutionContent); | ||
| Assert.Contains("</Solution>", solutionContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify the entire content of the generated file using raw string literals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Verify the entire content of the generated file using raw string literals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to verify the complete solution file content using raw string literals in commit e703181.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
- Coverage 96.22% 95.66% -0.56%
==========================================
Files 53 53
Lines 6404 6628 +224
Branches 707 739 +32
==========================================
+ Hits 6162 6341 +179
- Misses 154 189 +35
- Partials 88 98 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| var solutionContent = File.ReadAllText(solutionFile); | ||
| Assert.Contains("<Solution>", solutionContent); | ||
| Assert.Contains("<Project Path=", solutionContent); | ||
| Assert.Contains("</Solution>", solutionContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Verify the entire content of the generated file using raw string literals.
| // Verify project file format | ||
| Assert.Contains("<Project Sdk=\"Microsoft.NET.Sdk\">", projectContent); | ||
| Assert.Contains("<TargetFramework>", projectContent); | ||
| Assert.Contains("<AssemblyName>", projectContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Verify the entire content of the generated file using raw string literals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to verify complete project file content with all key properties in commit e703181.
| RunDotNet("new console --name example --output ."); | ||
| RunDotNet("build -bl -nr:false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Do not generate a new project. Use the Console project from CompilerTestFixture. Also update the instruction file to prefer using existing projects from CompilerTestFixture vs. creating new ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use Fixture.Console.Value.CompilerLogPath instead of generating a new project in commit e703181.
| RunDotNet("new console --name example --output ."); | ||
| RunDotNet("build -bl -nr:false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot prefer an existing project in CompilerTestFixture vs. creating a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use Fixture.Console existing project in commit e703181.
|
|
||
| predicate ??= static _ => true; | ||
|
|
||
| Directory.CreateDirectory(destinationDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Directory.CreateDirectory(destinationDir); | |
| _ = Directory.CreateDirectory(destinationDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied suggestion in commit e703181.
| Directory.CreateDirectory(destinationDir); | ||
|
|
||
| var referencesDir = Path.Combine(destinationDir, "references"); | ||
| Directory.CreateDirectory(referencesDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Directory.CreateDirectory(referencesDir); | |
| _ = Directory.CreateDirectory(referencesDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied suggestion in commit e703181.
…aw strings, use discard for Directory.CreateDirectory Co-authored-by: jaredpar <[email protected]>
| var solutionContent = File.ReadAllText(solutionFile); | ||
| var expectedSolution = """ | ||
| <Solution> | ||
| <Project Path="console-net9.0/console-net9.0.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot don't hardcode net9.0 but instead use TestUtil.TestTargetFramework
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use TestUtil.TestTargetFramework with string interpolation in commit 5166b07.
| Assert.Contains("<Project Sdk=\"Microsoft.NET.Sdk\">", projectContent); | ||
| Assert.Contains("<TargetFramework>net9.0</TargetFramework>", projectContent); | ||
| Assert.Contains("<AssemblyName>console</AssemblyName>", projectContent); | ||
| Assert.Contains("<OutputType>Exe</OutputType>", projectContent); | ||
| Assert.Contains("<EnableDefaultCompileItems>false</EnableDefaultCompileItems>", projectContent); | ||
| Assert.Contains("<GenerateAssemblyInfo>false</GenerateAssemblyInfo>", projectContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot use a raw string literal to verify the content of the entire file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to verify the complete project file content using raw string literal in commit 5166b07.
| // Verify that framework references like System.Runtime are NOT included | ||
| Assert.DoesNotContain("System.Runtime.dll", projectContent); | ||
| Assert.DoesNotContain("System.Private.CoreLib.dll", projectContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot use a raw string ilteral to verify the entire project file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to verify the entire project file using raw string literal in commit 5166b07.
…h raw string literals Co-authored-by: jaredpar <[email protected]>
| // Disable default compile items since we're explicitly including source files | ||
| sb.AppendLine(" <EnableDefaultCompileItems>false</EnableDefaultCompileItems>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot the project file should look natural which means it should use globbing and default items. Given that all the source files will appear under the project directory I would expect that there essentially no <Compile> entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use SDK default item globbing for source files. Removed EnableDefaultCompileItems=false and explicit <Compile> entries - the SDK now automatically includes all .cs files in the project directory. This makes the generated project files look more natural and idiomatic. Changes in commit 1d16038.
… entries Co-authored-by: jaredpar <[email protected]>
jaredpar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// <summary> | ||
| /// Export compilations as a full solution with project files that can be opened in VS or VS Code | ||
| /// </summary> | ||
| public void ExportProject(string destinationDir, Func<CompilerCall, bool>? predicate = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot I want to use different types to support exporting to RSP and Projects. To do that
- Take the existing ExportUtil type and rename it to ExportRspUtil
- Take the new functionality for solution / projects and put it in ExportSolutionUtil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll refactor ExportUtil into ExportRspUtil and ExportSolutionUtil. This is a significant change that I'll implement incrementally to ensure each step builds and tests correctly.
| var projectFilePath = Path.Combine(projectDir, projectFileName); | ||
| CreateProjectFile(index, compilerCall, projectFilePath, projectInfos, mvidToReferenceFile, destinationDir); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot the code also needs to write out all of the content files to the project directory: source files, analyzer config, resources, etc ... Those files need to maintain the same relative path in the new file structure as the orignal one.
For content that appeared outside the cone of the original project write it to a new directory called "external". Keep the same directory hierarchy inside "external" as the original file path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll enhance the solution export to write all content files (source, analyzer config, resources) maintaining their relative paths, with an "external" directory for files outside the project cone.
| "mscorlib", | ||
| }; | ||
|
|
||
| private bool IsFrameworkReference(ReferenceData refData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot I want to take a different approach here for determining if these are reference assemblies
- If there is a target framework and the references come from the runtime or reference pack then do not emit them to the project file. Can assume they are implicit from the
<TargetFramework>. - If there is no target framework then disable standard lib and add every reference as a
<Reference>item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update the reference detection logic to check if references come from runtime/reference packs and only emit non-framework references to the project file.
| } | ||
| #endif | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot lets add more tests. Update the UsingAllCompilerLogTests to have a new test method called ExportSolutionAndBuild. This should be essentially the same as ExportAndBuild but using solution export.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the ExportSolutionAndBuild test to UsingAllCompilerLogTests following the same pattern as ExportAndBuild.
|
|
||
| <ItemGroup> | ||
| <Reference Include="WindowsBase"> | ||
| <HintPath>../references/WindowsBase.dll</HintPath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot this path is linux specific but the tests run on windows and linux. Make this work for both operating systems. Suggest just using Path.Combine(.., "references", "WindowsBase.dll")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use Path.Combine("..", "references", "WindowsBase.dll") for cross-platform compatibility in commit 416bea1.
|
|
||
| <ItemGroup> | ||
| <Reference Include="WindowsBase"> | ||
| <HintPath>../references/WindowsBase.dll</HintPath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot this path is linux specific but the tests run on windows and linux. Make this work for both operating systems. Suggest just using Path.Combine(.., "references", "WindowsBase.dll")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use Path.Combine("..", "references", "WindowsBase.dll") for cross-platform compatibility in commit 416bea1.
Co-authored-by: jaredpar <[email protected]>
Implementation Complete: --solution Export Option
Summary
Added a
--solutionoption to theexportcommand that generates a complete Visual Studio/VS Code solution with project files that faithfully recreate the original compilation.Changes Implemented
Key Features
<Compile>entries<ProjectReference>elementsTesting
Usage
complog export --solution -o /path/to/output file.complog💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.