Generate assembly attributes using MinVer and csproj #779

lukebakken · 2020-03-25T22:34:44Z

Fixes #768

lukebakken · 2020-03-25T22:36:05Z

projects/client/RabbitMQ.Client/src/util/BlockingCell.cs

@@ -57,7 +57,6 @@ class BlockingCell<T>
    {
        private readonly ManualResetEventSlim _manualResetEventSlim = new ManualResetEventSlim(false);
        private T _value = default;
-        public EventHandler<T> ContinueUsingValue;


Removed since this is unused. Build failed with the following (no idea why it started now):

src\util\BlockingCell.cs(60,32): error CS0649: Field 'BlockingCell<T>.ContinueUsingValue' is never assigned to, and will always have its default value null [C:\Users\lbakken\development\rabbitmq\rabbitmq-dotnet-client\projects\client\RabbitMQ.Client\Ra bbitMQ.Client.csproj]

I'm not seeing a failure related to this, but if it's not being used, no harm in removing it.

Fixes #768

bording · 2020-03-26T15:53:05Z

@lukebakken Let me know if/when you'd like a review of this.

Alternatively, I have a branch ready to go with MinVer added if you'd like me to push that up.

lukebakken · 2020-03-26T16:31:36Z

@bording this PR appears to be working fine, but I need to edit all the internal CI stuff to work with it now and re-think how the myget / nuget packages are built (auto vs manual)

bording · 2020-03-26T16:39:16Z

@bording this PR appears to be working fine, but I need to edit all the internal CI stuff to work with it now and re-think how the myget / nuget packages are built (auto vs manual)

Ok, then I'll do a review pass on it if you don't mind, because there are some things I think need to be tweaked.

lukebakken · 2020-03-26T16:52:45Z

Sure, go right ahead. I just invited you as a collaborator, so feel free to push right to this branch.

bording

Also, it's a nitpick, but you removed all the blank lines I just intentionally added in my most recent PR. Having that spacing really improves the readability!

bording · 2020-03-26T16:39:38Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

@@ -1,43 +1,49 @@
 <Project Sdk="Microsoft.NET.Sdk">
-
  <PropertyGroup>
    <VersionPrefix>6.0.0</VersionPrefix>


This should be removed.

Yeah, we're using this as part of our CI scripts but I think that's going to change quite a bit. I'll remove it.

bording · 2020-03-26T16:39:53Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

+    <Copyright>Copyright © 2007-2020 VMware, Inc. or its affiliates.</Copyright>
+    <Description>The RabbitMQ .NET client is the official client library for C# (and, implicitly, other .NET languages)</Description>
+    <EmbedUntrackedSources>true</EmbedUntrackedSources>
+    <GenerateAssemblyInfo>true</GenerateAssemblyInfo>


This should be removed.

bording · 2020-03-26T16:40:07Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

    <PublishRepositoryUrl>true</PublishRepositoryUrl>
-    <EmbedUntrackedSources>true</EmbedUntrackedSources>
+    <RepositoryUrl>https://github.com/rabbitmq/rabbitmq-dotnet-client</RepositoryUrl>


This should be removed.

This was added by Visual Studio when I filled-out the project properties via the built-in properties editor. I'm assuming there's no harm in having it there.

It's being filled in already by Source Link, so you don't want to have it in the project file.

bording · 2020-03-26T16:40:15Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

    <PublishRepositoryUrl>true</PublishRepositoryUrl>
-    <EmbedUntrackedSources>true</EmbedUntrackedSources>
+    <RepositoryUrl>https://github.com/rabbitmq/rabbitmq-dotnet-client</RepositoryUrl>
+    <RepositoryType>Git</RepositoryType>


This should be removed.

This was added by Visual Studio when I filled-out the project properties via the built-in properties editor. I'm assuming there's no harm in having it there.

Same reply.

bording · 2020-03-26T16:43:09Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

    <IncludeSymbols>true</IncludeSymbols>
    <SymbolPackageFormat>snupkg</SymbolPackageFormat>
    <AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>
    <AssemblyOriginatorKeyFile>../rabbit.snk</AssemblyOriginatorKeyFile>
    <SignAssembly>true</SignAssembly>
+    <MinVerTagPrefix>v</MinVerTagPrefix>
+    <MinVerVerbosity>detailed</MinVerVerbosity>
+    <GeneratePackageOnBuild>false</GeneratePackageOnBuild>


I actually think it's a good idea to generate a package all the time, so I'd set this to true. It also makes sense to set PackageOutputPath to move the location of where they get created.

Something like <PackageOutputPath>..\..\..\packages</PackageOutputPath> for example.

bording · 2020-03-26T16:45:06Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

-  </ItemGroup>
-
-  <ItemGroup>
+    <AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo">


This is how you have to do this now, but there is better syntax coming for this soon! I think it might arrive in the 3.1.200 SDK

Yep I tried the new syntax which of course failed on 2.1 builds

bording · 2020-03-26T16:46:02Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

    <PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0" PrivateAssets="All" />
    <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All" />
+    <PackageReference Include="MinVer" Version="2.2.0">


This can be simplified to

<PackageReference Include="MinVer" Version="2.2.0" PrivateAssets="All" />

bording · 2020-03-26T16:53:35Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

-    <AssemblyTitle>RabbitMQ Client Library for .NET</AssemblyTitle>
-    <Description>The RabbitMQ .NET client is the official client library for C# (and, implicitly, other .NET languages)</Description>
-    <Company>VMware, Inc. or its affiliates.</Company>
+    <PackageLicenseFile>LICENSE</PackageLicenseFile>


Moving to embedding the license file is good, but this is incomplete. The PackageLicense line above should be removed, and you need to actually include the file in the package.

bording · 2020-03-26T16:54:03Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

  <ItemGroup>
    <Compile Include="..\..\..\gensrc\autogenerated-api-0-9-1.cs" />
-  </ItemGroup>
-
-  <ItemGroup>
    <None Remove="icon.png" />
    <Content Include="icon.png" PackagePath="" />


Right around here is where you need

<None Include="..\..\..\LICENSE" Pack="true" PackagePath="" />

bording · 2020-03-26T16:54:47Z

projects/client/RabbitMQ.Client/src/util/BlockingCell.cs

@@ -57,7 +57,6 @@ class BlockingCell<T>
    {
        private readonly ManualResetEventSlim _manualResetEventSlim = new ManualResetEventSlim(false);
        private T _value = default;
-        public EventHandler<T> ContinueUsingValue;


I'm not seeing a failure related to this, but if it's not being used, no harm in removing it.

bording · 2020-03-26T16:56:43Z

Sure, go right ahead. I just invited you as a collaborator, so feel free to push right to this branch.

Just saw this after I submitted my review.

This error showed up in the internal CI when packing the PR build: ``` /usr/share/dotnet/sdk/3.1.102/Sdks/NuGet.Build.Tasks.Pack/buildCrossTargeting/NuGet.Build.Tasks.Pack.targets(198,5): error NU5030: The license file 'LICENSE' does not exist in the package. [/tmp/build/db7f1294/rabbitmq_dotnet_client/projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj] ```

lukebakken · 2020-03-26T17:28:19Z

Thanks @bording

I'm just now addressing this issue - telia-oss/github-pr-resource#136

We use that Concourse resource and it doesn't fetch tags by default 🤦‍♂

...but I think this PR is looking alright, lmk.

bording · 2020-03-26T17:42:24Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

+    <AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo">
+      <_Parameter1>Unit, PublicKey=00240000048000009400000006020000002400005253413100040000010001008d20ec856aeeb8c3153a77faa2d80e6e43b5db93224a20cc7ae384f65f142e89730e2ff0fcc5d578bbe96fa98a7196c77329efdee4579b3814c0789e5a39b51df6edd75b602a33ceabdfcf19a3feb832f31d8254168cd7ba5700dfbca301fbf8db614ba41ba18474de0a5f4c2d51c995bc3636c641c8cbe76f45717bfcb943b5</_Parameter1>
+    </AssemblyAttribute>
+    <None Include="..\..\..\LICENSE">


This can be simplified to

<None Include="..\..\..\LICENSE" Pack="true" PackagePath="" />

I'd also move it up above the AssemblyAttribute entry.

bording · 2020-03-26T17:44:00Z

projects/client/RabbitMQ.Client/RabbitMQ.Client.csproj

    <None Remove="icon.png" />
    <Content Include="icon.png" PackagePath="" />
-  </ItemGroup>
-
-  <ItemGroup>


I personally like seeing the package references group separately into their own ItemGroup instead of all being in one, but that's more style / readability than a technical reason.

Feel free to re-org however you'd like! I'll refrain from smashing things all together going forward 😄

Feel free to re-org however you'd like! I'll refrain from smashing things all together going forward 😄

Just wanted to make sure I was pointing out style / preference things as such, and not trying to dictate how to do things!

bording · 2020-03-26T17:45:40Z

We use that Concourse resource and it doesn't fetch tags by default 🤦‍♂

Ah, yeah you're going to need the tags!

lukebakken · 2020-03-26T18:15:36Z

As always thank you @bording

Generate assembly attributes using MinVer and csproj (cherry picked from commit ca93809)

lukebakken self-assigned this Mar 25, 2020

lukebakken added this to the 6.0.0 milestone Mar 25, 2020

lukebakken commented Mar 25, 2020

View reviewed changes

lukebakken force-pushed the rabbitmq-dotnet-client-768 branch 2 times, most recently from 461dea1 to 03c4627 Compare March 26, 2020 14:56

lukebakken mentioned this pull request Mar 26, 2020

Graceful stop of worker services #781

Merged

Generate assembly attributes using MinVer and csproj

735b010

Fixes #768

lukebakken force-pushed the rabbitmq-dotnet-client-768 branch from 03c4627 to 735b010 Compare March 26, 2020 15:51

lukebakken marked this pull request as ready for review March 26, 2020 16:50

bording reviewed Mar 26, 2020

View reviewed changes

lukebakken added 3 commits March 26, 2020 09:57

PR review changes

05b3225

Remove redundant info

45d54c1

lukebakken requested a review from bording March 26, 2020 17:27

bording reviewed Mar 26, 2020

View reviewed changes

Tweaks

86b7049

bording approved these changes Mar 26, 2020

View reviewed changes

lukebakken merged commit ca93809 into master Mar 26, 2020

lukebakken deleted the rabbitmq-dotnet-client-768 branch March 26, 2020 18:15

lukebakken mentioned this pull request Mar 27, 2020

Back-port #779 to 5.x branch #784

Closed

lukebakken added a commit that referenced this pull request Mar 27, 2020

Merge pull request #779 from rabbitmq/rabbitmq-dotnet-client-768

c6ad6b0

Generate assembly attributes using MinVer and csproj (cherry picked from commit ca93809)

Generate assembly attributes using MinVer and csproj #779

Generate assembly attributes using MinVer and csproj #779

Uh oh!

Conversation

lukebakken commented Mar 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bording commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukebakken commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bording commented Mar 26, 2020

Uh oh!

lukebakken commented Mar 26, 2020

Uh oh!

bording left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bording Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bording commented Mar 26, 2020

Uh oh!

lukebakken commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bording commented Mar 26, 2020

Uh oh!

lukebakken commented Mar 26, 2020

Uh oh!

Uh oh!

bording commented Mar 26, 2020 •

edited

Loading

lukebakken commented Mar 26, 2020 •

edited

Loading

bording Mar 26, 2020 •

edited

Loading

lukebakken commented Mar 26, 2020 •

edited

Loading