Central package management (CPM) #188

Bartleby2718 · 2024-02-24T21:58:38Z

This implements #179.

Turns out I couldn't reopen #186 because I did some force-pushes on git. Copypasted all comments so that reviewers won't have to go back and forth between two PRs.

Bartleby2718 · 2024-02-24T21:59:05Z

src/Directory.Build.props

Created this file so that it's easy to tell the difference what CPM did to lock files.

Bartleby2718 · 2024-02-24T21:59:39Z

src/Directory.Packages.props

+<Project>
+  <PropertyGroup>
+    <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
+    <CentralPackageTransitivePinningEnabled>true</CentralPackageTransitivePinningEnabled>


We're doing both CPM and transitive pinning here. See https://learn.microsoft.com/en-us/nuget/consume-packages/central-package-management for details.

Bartleby2718 · 2024-02-24T22:00:31Z

src/DistributedLock.Azure/DistributedLock.Azure.csproj

Removed the Version attribute from all PackageReference items In all project files. There are only two project files of note: DistributedLock.ZooKeeper.csproj and DistributedLockCodeGen.csproj.

Bartleby2718 · 2024-02-24T22:01:32Z

src/DistributedLock.Azure/packages.lock.json

@@ -47,14 +47,6 @@
          "System.IO.Hashing": "6.0.0"
        }
      },
-      "Microsoft.Bcl.AsyncInterfaces": {


Manually confirmed that there is a corresponding NuGet package with type CentralTransitive here and elsewhere.

Bartleby2718 · 2024-02-24T22:02:52Z

src/Directory.Packages.props

+  </PropertyGroup>
+  <ItemGroup>
+    <PackageVersion Include="Microsoft.SourceLink.GitHub" Version="1.0.0" />
+    <PackageVersion Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="3.3.4" Condition="$(Configuration) == 'Release'"/>


Open question: do we need Conditions like this? I got this from the corresponding project files.

Response from @madelson (copypasting from #186 (comment)):

Yes I don't want public api analysis in debug mode, because of the weird setup I have where internal methods in Core are public in debug mode. The reason for that is that it forces me to think explicitly about which Core methods are "internal APIs" exposed to all DL implementations and which ones are for use within Core only, but it plays havoc with the analyzer in debug mode.

Bartleby2718 · 2024-02-24T22:03:27Z

src/DistributedLock.Azure/packages.lock.json

@@ -1,5 +1,5 @@
 {
-  "version": 1,
+  "version": 2,


Interesting that the version changed from 1 to 2. Given that there is a new type CentralTransitive, I guess that makes sense.

Bartleby2718 · 2024-02-24T22:04:13Z

src/DistributedLock.Tests/DistributedLock.Tests.csproj

-    <PackageReference Include="Moq" Version="4.13.1" />
+    <PackageReference Include="nunit" />
+    <PackageReference Include="nunit3testadapter" />
+    <PackageReference Include="Microsoft.NET.Test.SDK" />


Fixed tabs and spaces

Bartleby2718 · 2024-02-24T22:07:17Z

src/DistributedLockCodeGen/DistributedLockCodeGen.csproj

-    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.2.0" />
+    <PackageReference Include="nunit" />
+    <PackageReference Include="NUnit3TestAdapter" />
+    <PackageReference Include="Microsoft.NET.Test.Sdk" />


Note that this used to have a different version than DistributedLock.Tests.csproj. There is no VersionOverride per this thread, but that means that there are extra diffs in the lock file.

Bartleby2718 · 2024-02-24T22:08:17Z

src/DistributedLock.ZooKeeper/DistributedLock.ZooKeeper.csproj

-    <PackageReference Include="Nullable" Version="1.3.1">
+    <PackageReference Include="ZooKeeperNetEx" />
+    <PackageReference Include="Microsoft.SourceLink.GitHub" PrivateAssets="All" />
+    <PackageReference Include="Nullable" Condition="'$(TargetFramework)' != 'netstandard2.1'">


Given <TargetFrameworks>netstandard2.0;netstandard2.1;net461</TargetFrameworks> on line 4 of this file, I think it was an oversight that this Condition didn't exist. Note that this change is reflected in the lock file diff:
c87fb79#diff-b74c05733e0ca95b4f1c079e5455270db5118c2ba952b1cbc46d7a1452a8fb18L175. (Reminder that netstandard2.1 uses C# 8 by default.)

Comment from @madelson:

Does it break without this condition? I would have thought the package just noops in that case.

@madelson If you remove Condition only from ZooKeeper.csproj, you get

DistributedLock\src\DistributedLock.ZooKeeper\DistributedLock.ZooKeeper.csproj : error NU1010: The PackageReference items Nullable do not have corresponding PackageVersion.

This makes sense because

ZooKeeper.csproj targets netstandard2.1

Condition="'$(TargetFramework)' != 'netstandard2.1'" exists for Directory.Packages.props

the version must always be specified in Directory.Packages.props, but in this case, the version is not specified for Nullable in netstandard2.1

It's okay to install Nullable for netstandard2.1 here (or in other projects), but it's unnecessary.

Bartleby2718 · 2024-02-24T22:18:40Z

src/DistributedLock.Redis/DistributedLock.Redis.csproj

      <PrivateAssets>all</PrivateAssets>
    </PackageReference>
-    <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All"/>
-    <PackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="3.3.4" PrivateAssets="All" Condition="$(Configuration) == 'Release'"/>
-    <Using Remove="System.Net.Http"/>


Fly-by fix: removed this duplicate Using item because it appears a few lines below.

Bartleby2718 · 2024-02-24T22:22:21Z

@madelson Seems like there are some build errors:

unrelated compile-time errors

C:\projects\distributedlock\src\DistributedLock.Postgres\PostgresDistributedLock.cs(52,9): error CS8132: Cannot deconstruct a tuple of '2' elements into '3' variables. [C:\projects\distributedlock\src\DistributedLock.Postgres\DistributedLock.Postgres.csproj::TargetFramework=net461]
C:\projects\distributedlock\src\DistributedLock.Postgres\PostgresDistributedLock.cs(52,49): error CS8130: Cannot infer the type of implicitly-typed deconstruction variable 'useTransaction'. [C:\projects\distributedlock\src\DistributedLock.Postgres\DistributedLock.Postgres.csproj::TargetFramework=net461]
C:\projects\distributedlock\src\DistributedLock.Postgres\PostgresDistributedLock.cs(52,9): error CS8132: Cannot deconstruct a tuple of '2' elements into '3' variables. [C:\projects\distributedlock\src\DistributedLock.Postgres\DistributedLock.Postgres.csproj::TargetFramework=netstandard2.1]
C:\projects\distributedlock\src\DistributedLock.Postgres\PostgresDistributedLock.cs(52,49): error CS8130: Cannot infer the type of implicitly-typed deconstruction variable 'useTransaction'. [C:\projects\distributedlock\src\DistributedLock.Postgres\DistributedLock.Postgres.csproj::TargetFramework=netstandard2.1]
C:\projects\distributedlock\src\DistributedLock.Postgres\PostgresDistributedLock.cs(52,9): error CS8132: Cannot deconstruct a tuple of '2' elements into '3' variables. [C:\projects\distributedlock\src\DistributedLock.Postgres\DistributedLock.Postgres.csproj::TargetFramework=netstandard2.0]
C:\projects\distributedlock\src\DistributedLock.Postgres\PostgresDistributedLock.cs(52,49): error CS8130: Cannot infer the type of implicitly-typed deconstruction variable 'useTransaction'. [C:\projects\distributedlock\src\DistributedLock.Postgres\DistributedLock.Postgres.csproj::TargetFramework=netstandard2.0]
0 Warning(s)
6 Error(s)
Time Elapsed 00:00:57.69

.NET SDK 7.0.400 is installed in Ubuntu

/usr/share/dotnet/sdk/7.0.400/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(160,5): error NETSDK1045: The current .NET SDK does not support targeting .NET 8.0. Either target .NET 7.0 or lower, or use a version of the .NET SDK that supports .NET 8.0. Download the .NET SDK from https://aka.ms/dotnet/download [/home/appveyor/projects/distributedlock/src/DistributedLock.Tests/DistributedLock.Tests.csproj::TargetFramework=net8.0]
/usr/share/dotnet/sdk/7.0.400/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(160,5): error NETSDK1045: The current .NET SDK does not support targeting .NET 8.0. Either target .NET 7.0 or lower, or use a version of the .NET SDK that supports .NET 8.0. Download the .NET SDK from https://aka.ms/dotnet/download [/home/appveyor/projects/distributedlock/src/DistributedLockTaker/DistributedLockTaker.csproj::TargetFramework=net8.0]

Edit: just saw #189

Bartleby2718 · 2024-02-24T22:36:00Z

src/Directory.Build.props

Open question: do you want to keep the lock files? My intention was that they will make this PR a lot more review-able, but I'm fine with not using them.

I'm happy to have them

…nfiguration

Bartleby2718 added 2 commits February 24, 2024 16:41

Add RestorePackagesWithLockFile true for better diffs

e7bf9ab

Close madelson#179 adopt central package management

784eabd

Bartleby2718 changed the title ~~Central package management~~ Central package management (CPM) Feb 24, 2024

Remove VersionOverride for DistributedLockCodeGen.csproj

a7d8a9a

Bartleby2718 commented Feb 24, 2024

View reviewed changes

Bartleby2718 marked this pull request as ready for review February 24, 2024 22:19

Bartleby2718 commented Feb 24, 2024

View reviewed changes

Remove RestoreLockedMode because some packages have a condition on Co…

8df027a

…nfiguration

madelson merged commit 0d54c1c into madelson:release-2.4 Mar 17, 2024
1 check failed

madelson assigned Bartleby2718 Mar 17, 2024

Bartleby2718 deleted the central-package-management branch March 26, 2024 01:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Central package management (CPM) #188

Central package management (CPM) #188

Bartleby2718 commented Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 Feb 24, 2024

Bartleby2718 commented Feb 24, 2024 •

edited

Loading

Bartleby2718 Feb 24, 2024

madelson Mar 17, 2024

Central package management (CPM) #188

Central package management (CPM) #188

Conversation

Bartleby2718 commented Feb 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bartleby2718 commented Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bartleby2718 commented Feb 24, 2024 •

edited

Loading