SimpleInjector icon indicating copy to clipboard operation
SimpleInjector copied to clipboard

Nuget SemVer, dotnet build, dotnet pack and SourceLink Implemented

Open K4PS3 opened this issue 2 years ago • 7 comments

What this PR did:

  • Added 'Directory.Build.props' to the 'src' folder.
  • Added 'SimpleInjector.WithoutDoc.sln' to the 'src' folder (all the projects without SimpleInjector.Documentation).
  • Added '.gitignore' to the solution root.
  • Deleted 'packages.config' from the project 'SimpleInjector.CI'.
  • Removed the unnecessarily code from all 'AssemblyInfo.cs' (you can even delete them all).
  • All project files (.csproj) stripped down to the bare minimum required to function correctly.
  • The ActiveXTests.cs disabled by falsely pragma (#if false), because it use SHF COM Interop that require installing 3rd party library and a lot of configuration, disabled for now, you can advice me about the need for it, and the SHD Docs is it still in use or abandoned.
  • Changed the GetRepositoryRoot method in ConventionValues.cs to find root based on the new location of the build output (artifacts folder in the root).

What are the consequences of that:

  • You are able to run dotnet build SimpleInjector.WithoutDoc.sln to build 14 projects at once.
  • You are able to run dotnet test SimpleInjector.WithoutDoc.sln to run all the '1934' tests.
  • You are able to run dotnet test SimpleInjector.WithoutDoc.sln to generate 7 nugets with SemVer enforced to limit on the maximum allowed version that the dependencies can updated to.
  • SourceLink now part of the build process, and build valid nugets linked to the commit that was used to build the binaries and the nugets.
  • The nugets were including 'LICENSE' and 'README.md' files, according to the specifications of nuget.
  • Some of properties in the previous manually created nugets, now deprecated by Nuget, they are not exist in newly created by 'dotnet pack'.
  • Every project now include '.Files', that linked to the physical files on disk, the benefit you will access these files easily from anywhere in the solution without digging in folders or moving up and down.
  • The build output now centralized to the 'artifacts' folder in the root, this will make your 'src' clean, not cluttered with 'bin' and 'obj' folders, you have on location for everything come from the build process (assemblies, nugets, docs, ...).

Now to the hero of story 'Directory.Build.props':

This file contains (more than 600 lines) of MSBuild logic, most of them comments to make it easy for you to review the changes. I hope you will reread these words again after you check the changes, and i am sure after that all of this PR will make a sense and became clear.

you can split the content of the file to 3 categories :

  1. multiple 'PropertyGroup'.
  2. multiple 'ItemGroup'.
  3. and two 'Target'.

The logic is very simple, calculate properties values based on conditions, run certain parts of logic based on some conditions. another feature of the file is 'fallback to defaults' in case projects does not specify some necessarily properties or in case optional properties. The good news, with these changes, you are able to build correctly nugets wiht SemVer that limit lower and upper versions of nuget without involving any custom tools or long batch/PowerShell/bash scripts, or the use of 'Windows Only' tools. You are free now to build on Windows/Linux/macOS, even on containers or as i did, built it on github with Github Action. You are now able to publish your releases directly from github to nuget.org with automated and scheduled actions.

Now to the less happy part of the story, and i am writing this as a loyal user of SimpleInjector, and hope the best for it and all the team behind it. During the change over of the projects, i enabled CodeAnalysis with its full capabilities, unfortunately i got a lot really a lot of warnings, some of them expected and easy to fix, and there other that need to be fixed urgently, they for sure will introduce breaking changes. I did make use of 'NoWarn' in the 'Directory.Build.props' file to suppress these warnings, you will find note about every single warning that was disabled, and i hope that will make it easy for you to pick theme one by one and fix whatever you find it worth fixing.

Also during the test locally will debugging, i found that there 2 test failing randomly, but the same test run perfectly with release build.

First one, the test method: Verify_WhenCollectionIsCreatedForTypeThatFailsDuringCreation_VerifySucceedsWhenCollectionWasAlreadyCollected

With this error:

Test method SimpleInjector.Tests.Unit.ContainerCollectionsCreateTests.Verify_WhenCollectionIsCreatedForTypeThatFailsDuringCreation_VerifySucceedsWhenCollectionWasAlreadyCollected threw exception: System.InvalidOperationException: The configuration is invalid. Creating the instance for type FailingConstructorLogger failed. Value cannot be null. Parameter name: some programming error. - - - > SimpleInjector.ActivationException: Value cannot be null. Parameter name: some programming error. - - - > System.ArgumentNullException: Value cannot be null. Parameter name: some programming error.

The second one, the test method: Analyze_UnusedProducerWithGarbageCollected_DoesNotProduceAnyWarnings

With this error:

Test method SimpleInjector.Diagnostics.Tests.Unit.ExternalProducerCreationAnalysisTests.Analyze_UnusedProducerWithGarbageCollected_DoesNotProduceAnyWarnings threw exception: SimpleInjector.DiagnosticVerificationException: The configuration is invalid. The following diagnostic warnings were reported: -[Lifestyle Mismatch] RealUserService (Singleton) depends on IUserRepository implemented by InMemoryUserRepository (Transient). See the Error property for detailed information about the warnings. Please see https://simpleinjector.org/diagnostics how to fix problems and how to suppress individual warnings.

I Hope that will find something useful from this PR, and i hope that i am not crossing the line by my words, please bear with me, as I am not native English speaker, I could have used the wrong words.

looking forward for your comments.

Images for varies stages of this pr:

Successfully built nuget package with dotnet pack, including SourceLink and enforcing SemVer ValidPackage

On the left new package created with dotnet pack, on the right The official SimpleInjector package. ComparePackages

The generated nugets now include Symbol Packages. Nugets

The build process now emit logs with info about currently building project and other info about paths and environment. vs

K4PS3 avatar Oct 17 '23 02:10 K4PS3

You can use the attached workflows.zip file to add actions to the repo to activate the build on Pull requests or manual run, also there workflow for CodeQL.

K4PS3 avatar Oct 17 '23 02:10 K4PS3

I'm sorry, but I have little time to read let alone review your PR. Can you provide me with a summary of what problem your PR request solves?

dotnetjunkie avatar Oct 17 '23 07:10 dotnetjunkie

This PR solve the problem of manually manipulating .nuspec file to generate .nuget files out of the source code of SimpleInjector's projects.

With this PR :

  • you will be able to set the desired properties of the nuget in the project file '.csproj', and with simple command dotnet pack you will get .nuget files with upper limit of dependency enforced.
  • The generated .nuget files linked to github source code via SourceLink.
  • The project file .csproj become clean & clear and only contains specific unique properties of the project.

K4PS3 avatar Oct 17 '23 08:10 K4PS3

Thank you for clarifying this. Creating this PR must have taken you quite some time; I skimmed through it and it's quite impressive. You implemented quite a few things that were on my wish list, and what others requested as well (such as package symbols). One of the things that blocked me from adding this is the disability to specify a package version upper limit. You can read more about this feature request here.

I'm unfortunately quite busy at work, so I hope to go through your PR within a few weeks. I'll let you know.

dotnetjunkie avatar Oct 17 '23 20:10 dotnetjunkie

@dotnetjunkie Thanks for your kind words, I'm very excited. You can take your time to review this PR, I don't have any problem with that.

I am aware of the issue you referring to, version range implemented by nuget team for PackageReference items, but not for ProjectReference. I hope they soon manage to make it officially supported by them in professional way. In the mean time, hooking into Nuget internal implementation details (Tasks and Items that start with '_') is one way to solve the problem.

K4PS3 avatar Oct 18 '23 10:10 K4PS3

I am aware of the issue you referring to, version range implemented by nuget team for PackageReference items, but not for ProjectReference. I hope they soon manage to make it officially supported by them in professional way. In the mean time, hooking into Nuget internal implementation details (Tasks and Items that start with '_') is one way to solve the problem.

I tried using a similar workaround for version ranges in <ProjectReference> in some other projects but ran into issues. Have you tested the generated package by actually using it in a project? My experience is that the generated .nuspec in the package is correct (ie. the reference uses a version range), but the project.assets.json still references a single specific version. This caused dependency versioning issues that only surfaced at runtime.

This is the related issue on NuGet tracker and my comment describing the problem.

Apologies if this is know and tested already, I just want to make sure these changes don't accidentally create unforeseen problems. Thanks for putting the effort in!

w5l avatar Feb 24 '24 08:02 w5l

I small update on this PR: I decided not to merge this PR at the moment, but rather use the knowledge of this PR later when I start working on v6.0 of Simple Injector. This is why I keep this PR open; the work done in this PR is impressive and will be extremely useful for me once I start integrating this into v6.0. I added this PR to the v6.0 milestone.

dotnetjunkie avatar Feb 24 '24 08:02 dotnetjunkie