Skip to content

Clean up project files (reduce conditionals and reference assemblies)#1468

Merged
lucaspimentel merged 6 commits into
masterfrom
lpimentel/cleanup-reference-assemblies-redux
May 26, 2021
Merged

Clean up project files (reduce conditionals and reference assemblies)#1468
lucaspimentel merged 6 commits into
masterfrom
lpimentel/cleanup-reference-assemblies-redux

Conversation

@lucaspimentel

@lucaspimentel lucaspimentel commented May 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

This week on Cleaning up the old project files...

Remove most <TargetFrameworks Condition="'$(OS)' == 'Windows_NT'">

Most checks for '$(OS)' == 'Windows_NT' have been redundant since we added references to Microsoft.NETFramework.ReferenceAssemblies. These references assemblies allow the .NET SDK to target .NET Framework (net45, net461, etc) even if the .NET Framework SDK is not installed (for example, on Linux and macOS).

Remove <PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies">

.NET SDK 5.0 automatically includes the .NET Framework reference assemblies if needed, so we no longer need the explicit reference to Microsoft.NETFramework.ReferenceAssemblies.

Remove <PublicSign Condition="...">true</...>

.NET SDK 3.0 added support for signing assemblies on non-Windows OS. This public signing workaround to sign on Linux is a holdover from the 2.x era.

Remove <TargetFrameworks Condition="...">$(TargetFrameworks);net5.0</...>

We now support .NET 5 everywhere and require it to build the solution. No need to check the MSBuild version anymore.

@lucaspimentel lucaspimentel force-pushed the lpimentel/cleanup-reference-assemblies-redux branch from 2f6b7ec to 7ecca18 Compare May 17, 2021 14:20
@lucaspimentel lucaspimentel changed the title [WIP] lpimentel/cleanup reference assemblies, redux clean up OS checks and reference assemblies, redux May 17, 2021
@lucaspimentel lucaspimentel added area:builds project files, build scripts, pipelines, versioning, releases, packages type:cleanup Minor code clean up labels May 17, 2021
@lucaspimentel lucaspimentel force-pushed the lpimentel/cleanup-reference-assemblies-redux branch 7 times, most recently from cc477a0 to 049c08a Compare May 17, 2021 21:47
@lucaspimentel lucaspimentel changed the title clean up OS checks and reference assemblies, redux Clean up project files (reduce conditionals and reference assemblies) May 17, 2021
lucaspimentel
lucaspimentel commented May 17, 2021
View reviewed changes
Comment thread test/Directory.Build.props
<TargetFrameworks Condition="'$(OS)' == 'Windows_NT'">net452;net461;netcoreapp2.1;netcoreapp3.0;netcoreapp3.1</TargetFrameworks>
<TargetFrameworks Condition="'$(OS)' != 'Windows_NT'">netcoreapp2.1;netcoreapp3.0;netcoreapp3.1</TargetFrameworks>
<TargetFrameworks Condition="$(MSBuildVersion) >= 16.8.0">$(TargetFrameworks);net5.0</TargetFrameworks>
<!-- only run .NET Framework tests on Windows -->

@lucaspimentel lucaspimentel May 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Note: keeping the '$(OS)' == 'Windows_NT' here to avoid running .NET Framework test on Linux/macOS. Otherwise dotnet test tries to find mono to run the tests.

lucaspimentel
lucaspimentel commented May 17, 2021
View reviewed changes
Comment thread test/test-applications/integrations/Directory.Build.props
@@ -1,3 +0,0 @@
<Project>
<Import Project="..\Directory.Build.props" />

@lucaspimentel lucaspimentel May 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Note: removing redundant file, looking for Directory.Build.props up the folder hierarchy is already the default behavior.

lucaspimentel
lucaspimentel commented May 17, 2021
View reviewed changes
Comment thread test/test-applications/integrations/Samples.RabbitMQ/Samples.RabbitMQ.csproj
<TargetFrameworks Condition="'$(OS)' == 'Windows_NT'">net461;netcoreapp2.1;netcoreapp3.0;netcoreapp3.1</TargetFrameworks>
<!-- <TargetFrameworks Condition="'$(OS)' == 'Windows_NT' AND $(ApiVersion) &lt; 6.0.0">$(TargetFrameworks);net452</TargetFrameworks> -->
<TargetFrameworks Condition="$(MSBuildVersion) >= 16.8.0 AND !$(TargetFrameworks.Contains('net5.0'))">$(TargetFrameworks);net5.0</TargetFrameworks>
<DefineConstants Condition="$(ApiVersion) >= 6.0.0">$(DefineConstants);RABBITMQ_6_0</DefineConstants>

@lucaspimentel lucaspimentel May 17, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Note: this <DefineConstants> was a duplicate of line 5 above.

This was referenced May 19, 2021
Merged
Merged
@lucaspimentel lucaspimentel added the status:work-in-progress Actively worked on. If this is a PR, no review needed yet. WIP. label May 19, 2021
@lucaspimentel lucaspimentel force-pushed the lpimentel/cleanup-reference-assemblies-redux branch from 9d914dd to dffcf77 Compare May 19, 2021 22:02
@lucaspimentel lucaspimentel mentioned this pull request May 19, 2021
Merged
@lucaspimentel lucaspimentel changed the base branch from master to lpimentel/remove-oracle-sample-apps May 19, 2021 22:23
@lucaspimentel lucaspimentel marked this pull request as ready for review May 20, 2021 19:53
@lucaspimentel lucaspimentel requested a review from a team as a code owner May 20, 2021 19:53
andrewlock
andrewlock approved these changes May 21, 2021
View reviewed changes

@andrewlock andrewlock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just some minor comments

Comment thread test/Directory.Build.props
Comment thread test/test-applications/integrations/Samples.Dapper/Samples.Dapper.csproj
<!-- override to remove net452 -->
<TargetFrameworks Condition="'$(OS)' == 'Windows_NT'">net461;netcoreapp2.1;netcoreapp3.0;netcoreapp3.1</TargetFrameworks>
<TargetFrameworks Condition="$(MSBuildVersion) >= 16.8.0 AND !$(TargetFrameworks.Contains('net5.0'))">$(TargetFrameworks);net5.0</TargetFrameworks>
<TargetFrameworks>net461;netcoreapp2.1;netcoreapp3.0;netcoreapp3.1;net5.0</TargetFrameworks>

@andrewlock andrewlock May 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#1477 is about not building samples unless we're going to use them in CI (to avoid waste). This change (and other samples changes) make these build in CI on Linux where previously they weren't. Is that a change we def want to make?

FWIW, I'd rather have the simpler project files, as we can do optimisations in the CI build itself for speed separately.

@lucaspimentel lucaspimentel May 25, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TBH, #1477 was more about getting around build errors in one of those projects (i.e. kicking the can down the road 😅). Adding an extra target here on Linux/macOS only adds a few seconds to the build, and I'd rather have the simpler project files as well. It may even help Linux/macOS users find compiler errors that only happen when targeting .NET Framework.

Comment thread ...ns/integrations/dependency-libs/Samples.NoMultiLoader.Deps/Samples.NoMultiLoader.Deps.csproj
Comment thread ...t-applications/integrations/dependency-libs/Samples.SqlServer.Vb/Samples.SqlServer.Vb.vbproj Outdated
Comment thread ...NLog10LogsInjection.NullReferenceException/NLog10LogsInjection.NullReferenceException.csproj
zacharycmontoya
zacharycmontoya approved these changes May 24, 2021
View reviewed changes

@zacharycmontoya zacharycmontoya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with some small comments

@lucaspimentel lucaspimentel force-pushed the lpimentel/remove-oracle-sample-apps branch from 01565c4 to badae75 Compare May 25, 2021 15:16
Base automatically changed from lpimentel/remove-oracle-sample-apps to master May 25, 2021 22:30
@lucaspimentel lucaspimentel force-pushed the lpimentel/cleanup-reference-assemblies-redux branch 2 times, most recently from 522764f to 94de9af Compare May 25, 2021 23:16
@lucaspimentel lucaspimentel force-pushed the lpimentel/cleanup-reference-assemblies-redux branch from 94de9af to 2132ab3 Compare May 25, 2021 23:25
@lucaspimentel lucaspimentel merged commit d1e68d9 into master May 26, 2021
@lucaspimentel lucaspimentel deleted the lpimentel/cleanup-reference-assemblies-redux branch May 26, 2021 01:40
@lucaspimentel lucaspimentel mentioned this pull request May 28, 2021
Merged
@lucaspimentel lucaspimentel removed the status:work-in-progress Actively worked on. If this is a PR, no review needed yet. WIP. label May 28, 2021
@pellared pellared mentioned this pull request May 31, 2021
Merged
@andrewlock andrewlock added this to the 1.27.0 milestone Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes

@andrewlock andrewlock andrewlock approved these changes

Assignees

No one assigned

Labels

area:builds project files, build scripts, pipelines, versioning, releases, packages type:cleanup Minor code clean up

Projects

None yet

Milestone

1.27.0

Development

Successfully merging this pull request may close these issues.

3 participants

@lucaspimentel @zacharycmontoya @andrewlock