simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.21k stars 155 forks source link

Nuget SemVer, dotnet build, dotnet pack and SourceLink Implemented #980

Open K4PS3 opened 10 months ago

K4PS3 commented 10 months ago

What this PR did:

What are the consequences of that:

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 commented 10 months ago

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.

dotnetjunkie commented 10 months ago

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?

K4PS3 commented 10 months ago

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 :

dotnetjunkie commented 10 months ago

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.

K4PS3 commented 10 months ago

@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.

w5l commented 6 months ago

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!

dotnetjunkie commented 6 months ago

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.