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.

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

K4PS3
Copy link

@K4PS3 K4PS3 commented Oct 17, 2023

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

* let 'dotnet build SimpleInjector.WithoutDoc.sln' to build 14 projects at once.
* let 'dotnet test SimpleInjector.WithoutDoc.sln' to run all the '1934' tests.
* let 'dotnet test SimpleInjector.WithoutDoc.sln' to generate 7 nugets with SemVer enforced.
* Include SourceLink in the generated nugets.
* Centralize The build output location to 'artifacts' folder in the root.
* let the projects to be built in github,linux or mac in addition to windows.
@K4PS3
Copy link
Author

K4PS3 commented Oct 17, 2023

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
Copy link
Collaborator

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
Copy link
Author

K4PS3 commented Oct 17, 2023

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.

@dotnetjunkie
Copy link
Collaborator

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
Copy link
Author

K4PS3 commented Oct 18, 2023

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

<ContinuousIntegrationBuild>false</ContinuousIntegrationBuild>
<ContinuousIntegrationBuild Condition=" ( '$(GITHUB_ACTIONS)' == 'true' ) or
( '$(TF_BUILD)' == 'true' ) ">true</ContinuousIntegrationBuild>
<ContinuousIntegrationBuild>true</ContinuousIntegrationBuild>
Copy link

Choose a reason for hiding this comment

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

This second true without any condition looks suspicious, could it be a copy paste error or testing leftover?

@w5l
Copy link

w5l commented Feb 24, 2024

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 dotnetjunkie added this to the v6.0 milestone Feb 24, 2024
@dotnetjunkie
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants