Make multitarget possible

[sensslen]

Add multi-targeting support

[codecov]

Codecov Report

❌ Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.72%. Comparing base (713b42e) to head (3620b7c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/CommandLineUtils/Internal/ReflectionHelper.cs	28.57%	2 Missing and 3 partials ⚠️
...LineUtils/Conventions/OptionAttributeConvention.cs	80.00%	1 Missing and 2 partials ⚠️
src/CommandLineUtils/CommandLineApplication.cs	50.00%	0 Missing and 1 partial ⚠️
src/CommandLineUtils/Utilities/DotNetExe.cs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
- Coverage   79.93%   79.72%   -0.22%     
==========================================
  Files         120      121       +1     
  Lines        4017     4024       +7     
  Branches      872      873       +1     
==========================================
- Hits         3211     3208       -3     
- Misses        618      627       +9     
- Partials      188      189       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sensslen]

@natemcmaster this PR now almost builds successfully. The only missing bit is that the strong naming does not properly work for .net framework - hence the tests fail. Since I have no knowledge on strong naming whatsoever, can you please help me fix this?

[natemcmaster]

@claude can you provide PR feedback?

[claude]

Claude encountered an error —— View job

---

PR Review: Multi-targeting Support

Reviewing the multi-targeting implementation changes...

Todo List

• Examine PR diff and understand scope of changes
• Review project file modifications (.csproj)
• Analyze core library code changes
• Review test changes
• Investigate strong naming issue
• Check code coverage gaps
• Provide comprehensive feedback

---

[natemcmaster]

Thanks for taking time to put this together. Seeing all these little quirks coming back into the code reminds me of why I dropped .NET Framework in the first place. But, I'm still willing to move forward on this. We will need to get CI building and passing tests first.

One the potential issues I'm particularly keen to discuss is ongoing maintenance. I'm the only maintainer here, and I don't developer with windows anymore. If we could make compilation work for .NET Framework on all platforms, that would make it much easier to validate changes locally instead of needing to roundtrip through GitHub actions. I left a few comments on places in csproj that we can adjust to make that work.

As for the strong-naming piece...not sure. I don't know why that would have stopped working or how to fix it.

[natemcmaster]

I would generally prefer to keep linux runners. The windows agents are 5 to 10x slower (from my non-scientific testing). Is there any reason this needs to run on Windows?

[sensslen]

I was not aware of a way to make linux compile .net framework. That's the reason for the change. I may be wrong here though.

[natemcmaster]

Can we make this unconditional? There should be a way to get compiling working on non-windows platforms. IIRC there was a nuget package for this - https://www.nuget.org/packages/Microsoft.NETFramework.ReferenceAssemblies/

[sensslen]

Changed the main projects to do unconditional compilation. This is harder with the test projects though

[claude]

Claude finished @natemcmaster's task in 2m 17s —— View job

---

PR Review Feedback

Thanks for working on multi-targeting support! I've reviewed the PR and compiled feedback based on the maintainer's comments and code analysis. Here's a comprehensive review:

---

Critical Issues

1. Strong Naming Issue (Blocking)

The PR description and comments mention that strong naming doesn't work properly for .NET Framework, causing test failures. This needs to be resolved before the PR can be merged. The maintainer also noted they're unsure how to fix this - this is a critical blocker that needs investigation.

2. CI/Build Configuration Issues

.github/workflows/ci.yml:61 - The CI was changed to use Windows runners:

• The maintainer prefers Linux runners (5-10x faster)
• Question: Is there a specific reason this must run on Windows?
• If .NET Framework compilation can work on Linux with Microsoft.NETFramework.ReferenceAssemblies, the CI should stay on Linux

Language Version Divergence (McMaster.Extensions.Hosting.CommandLine.csproj:15):

• Using different C# language versions per target framework (latest vs framework-specific) creates maintenance issues
• The maintainer doesn't develop on Windows and won't be able to catch issues locally
• Recommendation: Find a way to use consistent language version across all targets

3. Project Configuration Concerns

Windows-Only Compilation (McMaster.Extensions.Hosting.CommandLine.csproj:7):

• Making compilation conditional on Windows makes it impossible for the maintainer to build locally on non-Windows platforms
• Solution: Use Microsoft.NETFramework.ReferenceAssemblies NuGet package to enable cross-platform .NET Framework compilation
• This should be unconditional to support all platforms

Wildcard Version (McMaster.Extensions.CommandLineUtils.csproj:42):

• Wildcards in version specifications lead to unpredictable behavior
• Should be pinned to a specific version for reproducibility

---

Code Quality Issues

4. Bare Catch Block (src/CommandLineUtils/Internal/ReflectionHelper.cs:212)

• Contains a bare catch without specifying exception type
• This is generally problematic as it catches all exceptions including system exceptions
• Need explanation of what specific exceptions this is meant to handle
• Should catch specific exception types instead

5. New Extension Methods Need Justification

StringExtensions.cs:11:

• New extension methods were added but their rationale isn't clear
• The maintainer asked: "These don't appear to be doing much"
• Need to explain why these are necessary and what problem they solve

DictionaryExtensions.cs (new file):

• Similarly needs justification for why this was added
• What .NET Framework limitation does this address?

---

Test Changes Requiring Explanation

6. Null-Coalescing Additions (CommandLineApplicationTests.cs:702 and many others)

• Many test assertions changed to use ?? [] pattern
• The maintainer asked what compiler error this fixes and how you determined this was the solution
• This appears throughout the test suite and needs explanation

Example locations:

• test/CommandLineUtils.Tests/CommandLineApplicationTests.cs:702
• Multiple similar changes across test files

7. Assert.IsAssignableFrom Additions (OptionAttributeTests.cs:344 and others)

• Many tests now use Assert.IsAssignableFrom<T> where they didn't before
• Need explanation for why this is required
• Is this a .NET Framework-specific requirement?

Example locations:

• test/CommandLineUtils.Tests/OptionAttributeTests.cs:344
• Multiple other test files

---

Code Coverage Concerns

8. Coverage Dropped

According to Codecov:

• Patch coverage is only 71.43% (10 lines missing coverage)
• Overall project coverage dropped from 79.93% to 79.72%

Files with poor coverage:

• src/CommandLineUtils/Internal/ReflectionHelper.cs - 16.66% patch coverage (5 lines missing)
• src/CommandLineUtils/Conventions/OptionAttributeConvention.cs - 80% patch coverage (3 lines missing)
• src/CommandLineUtils/CommandLineApplication.cs - 50% patch coverage (1 line missing)
• src/CommandLineUtils/Utilities/DotNetExe.cs - 0% patch coverage (1 line missing)

Recommendation: Add tests to cover the new conditional compilation paths, especially the .NET Framework-specific code.

---

Additional Observations

9. Scope of Changes

• 42 files changed (244 additions, 110 deletions)
• This is a substantial change affecting the core library, hosting library, and entire test suite
• Given the project is in "stable/maintenance mode", this warrants careful review

10. Maintenance Burden

The maintainer is the sole maintainer and doesn't develop on Windows anymore. This PR:

• Adds Windows-specific compilation requirements
• Introduces framework-specific code paths
• May make local validation difficult

Recommendation: Ensure all changes can be validated on Linux to reduce maintenance burden.

---

Summary

The PR shows good effort toward multi-targeting, but has several blocking issues:

Must Address:

1. Strong naming for .NET Framework
2. Make compilation work cross-platform (use Microsoft.NETFramework.ReferenceAssemblies)
3. Keep CI on Linux runners if possible
4. Remove wildcard versions
5. Fix bare catch block
6. Improve test coverage

Should Explain:

1. Rationale for new extension methods
2. Why ?? [] changes were needed
3. Why Assert.IsAssignableFrom<T> was added
4. Divergent language version requirements

Consider:

• Whether the maintenance burden of multi-targeting aligns with the project being in maintenance mode
• How to ensure changes can be validated without Windows

The maintainer has already flagged most of these concerns in their review. Addressing the review comments, especially the cross-platform compilation and strong naming issues, should be the priority.

---

[sensslen]

@natemcmaster I was able to figure out the strong naming issue. Basically Microsoft changed how strong named assemblies are loaded between .nset framework and .net core. Basically the source generator failed to load when it was strong named --> See https://learn.microsoft.com/en-us/dotnet/standard/library-guidance/strong-naming

Or so I thought... :(