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.

Sign up for GitHub

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

Jump to bottom

#9 add in net 6 #10

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

#9 add in net 6 #10

wants to merge 6 commits into from

Conversation

thompson-tomo
Copy link

This will add net 6 as a TFM and ensure that the dependencies are not added.

Closes #1
Closes #9

304NotModified
304NotModified requested changes Apr 6, 2024
View reviewed changes
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

We need also this for .NET8 isn't?

Also we need to update the targets in the unit tests.

The build isn't working, I guess it needs and update the #if statements

@pull-request-size pull-request-size bot added size/L and removed size/XS labels Apr 7, 2024
@thompson-tomo
Copy link
Author

@304NotModified dotnet 8 is not needed as for dotnet 8 it will use the net 6 compilation. Yes i have gone and adjusted the conditional compiles

@snakefoot
Copy link
Contributor

Well NET6 will expire soon (end of year), and then the build-pipeline will explode. So I guess you need to add both NET6 + NET8,

@thompson-tomo
Copy link
Author

Strongly doubt that it would explode given that pipelines using net core 3 are still operational.

@snakefoot
Copy link
Contributor

snakefoot commented Apr 7, 2024 •
edited
Loading

Can these build-warnings CA1416 be fixed:

NLog.WindowsIdentity -> C:\projects\NLog-WindowsIdentity\src\NLog.WindowsIdentity\bin\Any CPU\Release\net46\NLog.WindowsIdentity.dll
C:\projects\NLog-WindowsIdentity\tests\NLog.WindowsIdentity.Tests\ImpersonatingTargetWrapperTests.cs(133,70): warning CA1416: This call site is reachable on all platforms. 'WindowsIdentity.Name' is only supported on: 'windows'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [C:\projects\NLog-WindowsIdentity\tests\NLog.WindowsIdentity.Tests\NLog.WindowsIdentity.Tests.csproj::TargetFramework=net6.0]

@pull-request-size pull-request-size bot added size/M and removed size/L labels Apr 7, 2024
@thompson-tomo
Copy link
Author

Can these build-warnings CA1416 be fixed:

NLog.WindowsIdentity -> C:\projects\NLog-WindowsIdentity\src\NLog.WindowsIdentity\bin\Any CPU\Release\net46\NLog.WindowsIdentity.dll
C:\projects\NLog-WindowsIdentity\tests\NLog.WindowsIdentity.Tests\ImpersonatingTargetWrapperTests.cs(133,70): warning CA1416: This call site is reachable on all platforms. 'WindowsIdentity.Name' is only supported on: 'windows'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1416) [C:\projects\NLog-WindowsIdentity\tests\NLog.WindowsIdentity.Tests\NLog.WindowsIdentity.Tests.csproj::TargetFramework=net6.0]

This should also be resolved now

@snakefoot
Copy link
Contributor

Then I guess it is "just" a matter of making the build-pipeline green.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 7, 2024
@thompson-tomo thompson-tomo force-pushed the chore/#9_AddNet6 branch 2 times, most recently from 5318cdf to f88049a Compare April 7, 2024 09:23
@pull-request-size pull-request-size bot added size/M and removed size/L labels Apr 7, 2024
@thompson-tomo thompson-tomo force-pushed the chore/#9_AddNet6 branch 2 times, most recently from 17cae55 to 8350d51 Compare April 7, 2024 09:26
@thompson-tomo
Copy link
Author

Then I guess it is "just" a matter of making the build-pipeline green.

hopefully those last changes to the test projects resolve the build pipeline

@snakefoot
Copy link
Contributor

snakefoot commented Apr 11, 2024 •
edited
Loading

Since you are explicitly adding NET6 and NET8, then I guess we should change this:

<EnableTrimAnalyzer>true</EnableTrimAnalyzer>
<IsTrimmable>true</IsTrimmable>

To this:

<EnableTrimAnalyzer Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0'))">true</IsTrimmable>
<IsTrimmable Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0'))">true</IsTrimmable>

@snakefoot snakefoot mentioned this pull request Apr 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snakefoot snakefoot snakefoot left review comments

@304NotModified 304NotModified Awaiting requested review from 304NotModified

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add additional TFM to reduce dependencies .NET6 includes System.Security.Principal.Windows
3 participants
@thompson-tomo @snakefoot @304NotModified