-
Notifications
You must be signed in to change notification settings - Fork 153
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
base: master
Are you sure you want to change the base?
Nuget SemVer, dotnet build, dotnet pack and SourceLink Implemented #980
Conversation
* 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.
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. |
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? |
This PR solve the problem of manually manipulating With this PR :
|
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 Thanks for your kind words, I'm very excited. I am aware of the issue you referring to, version range implemented by nuget team for PackageReference items, but not for ProjectReference. |
<ContinuousIntegrationBuild>false</ContinuousIntegrationBuild> | ||
<ContinuousIntegrationBuild Condition=" ( '$(GITHUB_ACTIONS)' == 'true' ) or | ||
( '$(TF_BUILD)' == 'true' ) ">true</ContinuousIntegrationBuild> | ||
<ContinuousIntegrationBuild>true</ContinuousIntegrationBuild> |
There was a problem hiding this comment.
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?
I tried using a similar workaround for version ranges in 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! |
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. |
What this PR did:
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.GetRepositoryRoot
method inConventionValues.cs
to find root based on the new location of the build output (artifacts folder in the root).What are the consequences of that:
dotnet build SimpleInjector.WithoutDoc.sln
to build 14 projects at once.dotnet test SimpleInjector.WithoutDoc.sln
to run all the '1934' tests.dotnet test SimpleInjector.WithoutDoc.sln
to generate 7 nugets withSemVer
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.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 :
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:
The second one, the test method:
Analyze_UnusedProducerWithGarbageCollected_DoesNotProduceAnyWarnings
With this error:
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 SemVerOn the left new package created with
dotnet pack
, on the right The official SimpleInjector package.The generated nugets now include Symbol Packages.
The build process now emit logs with info about currently building project and other info about paths and environment.