net5.0-platform support for Pack #3479

zkat · 2020-06-26T20:56:54Z

So #3339 had to be reverted because it caused some nasty failures during integration with dotnet, because our Pack tests weren't covering net5.0 scenarios, much less with the updated net5 tfm format changes.

This PR fixes this specific issue and adds some tests so we're not caught by surprise in the future.

Review of this PR might be better off being focused on the changes in 22b5836e8e66448e332515d33f5e9204ea0118aa, since the other commit is what was in #3339 originally, since I'm reverting the revert.

Fixes: NuGet/Home#9682

zivkan · 2020-06-26T23:17:31Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Utility/NuGetFrameworkNameUtility.cs

+
+namespace NuGet.Packaging
+{
+    public static class NuGetFrameworkNameUtility


This class is used a few times within the same project/assembly, but also once from NuGet.CommandLine. Is there a way to refactor a little so it doesn't need to be used by NuGet.CommandLine, and change the class to internal?

NuGetFrameworkNameUtility doesn't seem like an API that should be public for customers using NuGet.Packaging in their own apps.

I'm confused. Does this file already exist? https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Packaging/PackageCreation/Utility/FrameworkNameUtility.cs

Why does it appear as a new file in this PR?

FrameworkNameUtility vs FrameworkNameUtility.

I think we can combine them into 1.

One returns FrameworkName, this one returns NuGetFramework.

I've merged them into a single utility.

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/IPackageFile.cs

erdembayar · 2020-06-29T18:59:27Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Utility/NuGetFrameworkNameUtility.cs

+
+            effectivePath = path;
+
+            if (String.IsNullOrEmpty(targetFrameworkString))


string.IsNullOrEmpty instead of String.IsNullOrEmpty? Guideline #11

src/NuGet.Clients/NuGet.CommandLine/Commands/ProjectFactory.cs

nkolev92 · 2020-07-01T00:08:18Z

src/NuGet.Core/NuGet.Frameworks/def/IFrameworkMappings.cs

@@ -60,8 +60,8 @@ interface IFrameworkMappings
        IEnumerable<OneWayCompatibilityMappingEntry> CompatibilityMappings { get; }

        /// <summary>
-        /// Ordered list of framework identifiers. The first framework in the list will be preferred over other 
-        /// framework identifiers. This is enable better tie breaking in scenarios where legacy frameworks are 
+        /// Ordered list of framework identifiers. The first framework in the list will be preferred over other


nit: You can consider reverting this givent hat there are no other changes.

src/NuGet.Core/NuGet.Packaging/PackageCreation/Extensions/FrameworksExtensions.cs

nkolev92 · 2020-07-01T00:11:43Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Utility/NuGetFrameworkNameUtility.cs

+
+namespace NuGet.Packaging
+{
+    public static class NuGetFrameworkNameUtility


FrameworkNameUtility vs FrameworkNameUtility.

I think we can combine them into 1.

nkolev92 · 2020-07-01T00:12:38Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Utility/NuGetFrameworkNameUtility.cs

+{
+    public static class NuGetFrameworkNameUtility
+    {
+        [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1021:AvoidOutParameters", MessageId = "1#")]


Is CA1021 still raised?
I don't think we have this enabled anymore.

nkolev92 · 2020-07-01T00:13:37Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Utility/NuGetFrameworkNameUtility.cs

+
+namespace NuGet.Packaging
+{
+    public static class NuGetFrameworkNameUtility


One returns FrameworkName, this one returns NuGetFramework.

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/IPackageFile.cs

nkolev92 · 2020-07-01T00:14:50Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Utility/NuGetFrameworkNameUtility.cs

+        /// <param name="strictParsing">if set to <c>true</c>, parse the first folder of path even if it is unrecognized framework.</param>
+        /// <param name="effectivePath">returns the path after the parsed target framework</param>
+        /// <returns></returns>
+        [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1021:AvoidOutParameters", MessageId = "2#")]


Same here, I'm not sure C1021 is enabled in our current analyzers.

In fact, we can use a value tuple instead of out params cause they are more explicit.

nkolev92 · 2020-07-01T00:17:32Z

test/NuGet.Core.Tests/NuGet.Packaging.Test/PackageBuilderTest.cs

@@ -160,8 +214,8 @@ public void CreatePackageWithNuspecIncludeExclude()
            var net45 = new PackageDependencyGroup(new NuGetFramework(".NETFramework", new Version(4, 5)), dependencies45);
            builder.DependencyGroups.Add(net45);

-            var net46 = new PackageDependencyGroup(new NuGetFramework(".NETFramework", new Version(4, 6)), dependencies46);
-            builder.DependencyGroups.Add(net46);
+            var net50win = new PackageDependencyGroup(new NuGetFramework(".NETCoreApp", new Version(5, 0), "windows", new Version(0, 0)), dependencies50);


The most full proof test to ensure net50 can be packed is to add a test in https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs.

Add the test only for net5.0 projects.

Fixes: NuGet/Home#9682

wli3 · 2020-07-06T21:00:14Z

Any update?

zkat · 2020-07-06T21:20:48Z

Just waiting for final approvals. Everything is passing, but I need another review (/cc @nkolev92 @zivkan )

wli3 · 2020-07-07T00:08:51Z

@nkolev92 @zivkan could you approve it soon? We have several works being blocked by this change. And the deadline is next week.

nkolev92

Looks good.

A few comments to address before merging and a nit or two for you consider.

src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs

nkolev92 · 2020-07-07T02:53:59Z

src/NuGet.Core/NuGet.Frameworks/GlobalSuppressions.cs

@@ -40,3 +40,4 @@
 [assembly: SuppressMessage("Build", "CA1062:In externally visible method 'bool NuGetFrameworkUtility.IsNetCore50AndUp(NuGetFramework framework)', validate parameter 'framework' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Frameworks.NuGetFrameworkUtility.IsNetCore50AndUp(NuGet.Frameworks.NuGetFramework)~System.Boolean")]
 [assembly: SuppressMessage("Build", "CA2237:Add [Serializable] to FrameworkException as this type implements ISerializable", Justification = "<Pending>", Scope = "type", Target = "~T:NuGet.Frameworks.FrameworkException")]
 [assembly: SuppressMessage("Build", "CA1067:Type NuGet.Frameworks.OneWayCompatibilityMappingEntry should override Equals because it implements IEquatable<T>", Justification = "<Pending>", Scope = "type", Target = "~T:NuGet.Frameworks.OneWayCompatibilityMappingEntry")]
+[assembly: SuppressMessage("Globalization", "CA1308:Normalize strings to uppercase", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Frameworks.FrameworkNameProvider.TryGetShortPlatform(System.String,System.Version,System.String,System.String@)~System.Boolean")]


nit: Add a justification.

nkolev92 · 2020-07-07T02:54:25Z

src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs

+        /// <summary>
+        /// Creates a new NuGetFramework instance, with an optional platform and platformVersion (only available for net5.0+)
+        /// </summary>
+        public NuGetFramework(string frameworkIdentifier, Version frameworkVersion, string platform, Version platformVersion)


Same feedback as above, just confirm that platform and version are still the correct concept names.

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PhysicalPackageFile.cs

nkolev92 · 2020-07-07T02:58:34Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Extensions/FrameworksExtensions.cs

        public static string GetFrameworkString(this NuGetFramework self)
        {
+            if (self.Version.Major >= 5)


This feels very fragile and likely incorrect. We should check the TargetFrameworkIdentifier too.

That's what the IsNet5Era check does:

IsNet5Era = (_frameworkVersion.Major >= Version5 && StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, _frameworkIdentifier));

I've decided to go ahead and export IsNet5Era as a public property. I think it's going to be useful for more than just our own needs. I don't like the idea of copying this stuff all over the place when special-casing net5 stuff.

I'd rather have a duplicated logic tbh.

NuGet.Frameworks and NuGet.Versioning are arguably the most important core libraries in all of NuGet.
ISNet5Era screams like it's something that we couldn't properly design, very much an internal detail.

@zivkan thoughts?

src/NuGet.Core/NuGet.Packaging/PackageCreation/Extensions/FrameworksExtensions.cs

zivkan

sorry about the late feedback. Too many things going on :(

src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs

zivkan · 2020-07-07T03:08:12Z

src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs

+        {
+            if (frameworkIdentifier == null)
+            {
+                throw new ArgumentNullException("frameworkIdentifier");


Suggested change

throw new ArgumentNullException("frameworkIdentifier");

throw new ArgumentNullException(nameof(frameworkIdentifier));

This was intentional, in order to keep it consistent with the existing API. Are you sure I should change it? I thought we had this discussion before. I'm happy to make this change, though, if you want.

I remember a discussion where someone was using nameof for strings that are compared to command line arguments customers pass in. I argued that it shouldn't use nameof because renaming the variable in code should not break functional behaviour. I lost that argument.

But for validating method parameters, I'd expect it to match the parameter name. If I use NuGet.Frameworks in my app, and I get an argument null exception saying that parameter x should not be null, but none of the method parameters are named x, then it's confusing.

You're right that the exception message will change between package versions if we change the parameter name, so it's a tradeoff of making the error message be consistent with previous versions, or be consistent with the current code. In either case we'll break anyone using named parameters, so it's ideal to avoid renaming parameters if at all possible.

overall, it's more of a nitpick, so I don't care very much. The mandatory/optional TPV is a much more important issue.

zivkan · 2020-07-07T03:08:24Z

src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs

+
+            if (frameworkVersion == null)
+            {
+                throw new ArgumentNullException("frameworkVersion");


Suggested change

throw new ArgumentNullException("frameworkVersion");

throw new ArgumentNullException(nameof(frameworkVersion));

zivkan · 2020-07-07T03:08:33Z

src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs

+
+            if (platform == null)
+            {
+                throw new ArgumentNullException("platform");


Suggested change

throw new ArgumentNullException("platform");

throw new ArgumentNullException(nameof(platform));

zivkan · 2020-07-07T03:08:41Z

src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs

+
+            if (platformVersion == null)
+            {
+                throw new ArgumentNullException("platformVersion");


Suggested change

throw new ArgumentNullException("platformVersion");

throw new ArgumentNullException(nameof(platformVersion));

zivkan · 2020-07-07T03:29:58Z

....Tests/NuGet.VisualStudio.Implementation.Test/Extensibility/VsFrameworkCompatibilityTests.cs

+            Assert.Throws<System.ArgumentException>(() => target.GetNearest(targetFramework, frameworks));
+


tailing whitespace should be removed.

But also, why isn't .NETCoreApp,Version=v4.0 compatible with net5? I mean, I know it's not a real .NET Core version, but according to the rule of supporting the same platform identifier with a lower version number, it should be compatible.

This is just to verify that we do, indeed, fail on .NET5+ TFMs with this API. This is one of the IVs APIs that need to be updated or have a sibling API created that supports Platforms.

(the error it checks for is the argument error for trying to do new FrameworkName(nearest.DotNetFrameworkName))

probably not a big deal, but I still don't understand.

net4.0 is not compatible with net5.0, but that's because net4.0 == .NETFramework,Version=4.0, whereas net5.0 == .NETCoreApp,Version=5.0. In other words the TargetFrameworkIdentifier (TFI) are different, even though the "short folder name" looks the same.

But when the TFI is the same (.NETCoreApp), version 5.0 should generally be compatible with lower versions, including 4.0.

If net5.0 (.NETCoreApp,Version=5.0) isn't compatible with netcoreapp3.1 (.NETCoreApp,Version=3.1) then we have a bug that will impact customers.

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs

zivkan · 2020-07-07T03:33:31Z

test/NuGet.Core.Tests/NuGet.Frameworks.Test/CompatibilityTests.cs

-        [InlineData("net5.0-watchos", "net5.0", false)]
-        [InlineData("net5.0-windows", "net5.0", false)]
+        // net5.0-<osname> is compatible with net5.0 but not the inverse
+        [InlineData("net5.0-android", "net5.0", true)]


as commented elsewhere, are TFMs with a platform identifier, but no platform version even supported/allowed? How can NuGet do compat checks, when we don't know the version?

They are indeed supported, and NuGetFrameworks without an OsVersion are perfectly valid -- they're evaluated later based on information we get from the SDK (that's tbd and it's one of the future things to do)

This is the occasion where the NuGetFramework API needs to support one thing, but restore will actually require the min version :)

I really don't want to delay this PR being merged, but just as Nikolche wrote, what needs to work from a user point of view working with their csproj file is not necesserily the same as what the NuGetFramework class needs to implement.

Another way to look at it: I'm writing a custom desktop app that knows nothing about MSBuild or the .NET SDK, but uses NuGet.Framework from the package on nuget.org. I want to check if two TFMs are compatible. If we don't enforce TPV being mandatory, how can we do the check?

In my opinion, the TPV being optional for csproj restore is something we should handle in the msbuild "goo", but by the time we start using NuGetFramework instances, we always have the version.

zivkan · 2020-07-07T03:35:33Z

test/NuGet.Core.Tests/NuGet.Frameworks.Test/NuGetFrameworkParseTests.cs

@@ -224,6 +226,14 @@ public void NuGetFramework_ProfileName(string folder, string expected)
        [InlineData("netcoreapp1.5", ".NETCoreApp,Version=v1.5")]
        [InlineData("netcoreapp2.0", ".NetCoreApp,Version=v2.0")]
        [InlineData("netcoreapp3.0", ".NetCoreApp,Version=v3.0")]
+        [InlineData("net5.0-android", "net5.0-android")]


I'm repeating myself many times :) are net5.0 TFMs with platform identifier, but no platform version allowed?

See above :)

zivkan · 2020-07-07T03:36:21Z

test/NuGet.Core.Tests/NuGet.Packaging.Test/PackageBuilderTest.cs

@@ -186,7 +240,7 @@ public void CreatePackageWithNuspecIncludeExclude()
      <group targetFramework="".NETFramework4.5"">
        <dependency id=""packageB"" version=""1.0.0"" exclude=""z"" />
      </group>
-      <group targetFramework="".NETFramework4.6"">
+      <group targetFramework=""net5.0-windows"">


nuspecs should definately never allow a net5.0 TFM with a TPI, but no TPV.

This is actually totally valid! There's going to be a default version provided by the sdk for each major .net version

My understanding is that it's valid for a project file (csproj). But in the future when we restore a project targeting net6.0-windows10.0.12345, and the package has binaries for net5.0-windows, we won't know what the default platform version was for windows for net5.0-windows at the time that the package author packed the project.

So, the csproj uses net5.0-windows, but restore gets the default version from the SDK, and writes that in the assets file. I'm not sure if pack reads the project TFMs from the assets file, or if it also needs to get the default platform version from the SDK, but it's resolved at pack time and burned into the nuspec and the package.

Otherwise every year, the SDK will need to pass an array of every previous .net's version default platform versions. Imagine net20 trying to figure out what the net5.0-windows default platform version was.

I think @zivkan brings out a good point.
Pack needs to hard code the version here.

....or maybe I misunderstood :( I guess this is THE place where we shouldn't allow it?

We have a meeting invite for later today, so I hope we can discuss it more quickly than these comments going back and forth, but @dsplaisted's comment in another issue: NuGet/Home#9444 (comment) appears to be evidence that once NuGet starts reading the 4/5 MSBuild properties, rather than parsing TargetFramework directly, by the time we get to C# code, TPV will always be defined. My understanding is still that the c# class NuGet.Frameworks.NuGetFramework can treat TPV as mandatory when TPI is not the empty string/null.

My understanding is still that the c# class NuGet.Frameworks.NuGetFramework can treat TPV as mandatory when TPI is not the empty string/null.

@zivkan I don't think that will work. We're going to be using NuGetFramework to parse out those MSBuild properties from the TargetFramework. For example (from this PR):

<TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier> <TargetPlatformVersion>$([MSBuild]::GetTargetPlatformVersion('$(TargetFramework)', 3))</TargetPlatformVersion>

Those intrinsic MSBuild functions will be implemented by calling the NuGetFramework APIs, so they need to work when you have the platform name but not the version.

You might be able to require that the platform version be set if the platform name is set when doing compatibility checks for a NuGetFramework.

I see. I was reading the SDK's master branch targets file, which does string manipulation, so thought the SDK wouldn't be using our API. This indeed makes things more complex, but for a very good reason.

@zkat @nkolev92 is adding a new ParseProjectTargetFramework(string) that allows optional TPV, but all the other Parse* methods keeping it mandatory a horrible idea?

Maybe I should give up on this idea and accept that the rest of our code is going to have higher risk of bugs if we don't check if TPV is defined correctly.

Apparently NuGetFramework.Parse("netcoreapp") works and returns an object with TargetFrameworkfVersion=0.0. So maybe this isn't a good suggestion. I feel like I needlessly delayed this PR now.

I think it would be good for the nuspec and nupkg's lib/tfm directory to contain the version 0.0 TPV. It's a signal that something went wrong. I don't know if GetShortFolderName should always return TPV, or if we should limit it to pack. My feeling is always, but I don't know if netcoreapp0.0 or netcoreapp is returned by GetShortFoldName in the example above.

See #3479 (comment) :(

zivkan

Given the SDK is going to start using NuGetFramework to parse project's TargetFramework, my hope to have a model that doesn't allow invalid states is not possible. We're just going to have to be careful everywhere we use it (particularly pack, but maybe other places too)

nkolev92

I think we should not expose the IsNet5Era property.

The rest looks fine.

nkolev92 · 2020-07-09T22:43:31Z

src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs

@@ -350,7 +431,7 @@ public bool IsSpecificFramework
        /// <summary>
        /// True if this framework is Net5 or later, until we invent something new.
        /// </summary>
-        internal bool IsNet5Era { get; set; }
+        public bool IsNet5Era { get; private set; }


Can we please keep this internal?

This is a very low level API and I'd like us to keep it as clean as possible.
Personally I'd rather have code duplication than expose a concept that's really a consequence of a business decision.

@zivkan thought?

I missed that this changed. I agree, we should keep it internal. If we get feedback from the SDK team or others that they hate having to do the TFI & TFV comparisons themselves and duplicating this check, we can add it without it being a breaking change (and also try to come up with a better name)

sfoslund · 2020-07-10T18:36:32Z

@zkat great to see this has been merged! When can we expect a new build to be promoted?

rrelyea · 2020-07-10T19:56:44Z

this has been merged into our dev branch. We will likely be starting a VS master branch (16.8) insertion early next week. And once that succeeds, then following up with SDK insertions. Feel free to start an internal conversation with @aortiz-msft , @zkat and myself if that timing doesn't work for any reason.

zivkan · 2020-07-16T00:31:31Z

@sfoslund It's in now! dotnet/sdk#12496 🥳

zkat requested review from aortiz-msft, cristinamanum, dominoFire, donnie-msft, dtivel, heng-liu, kartheekp-ms, nkolev92, rrelyea and zivkan as code owners June 26, 2020 20:56

wli3 mentioned this pull request Jun 26, 2020

Support runtimecopylocal to enable cswinrt dotnet/sdk#12232

Merged

zivkan reviewed Jun 26, 2020

View reviewed changes

sfoslund mentioned this pull request Jun 29, 2020

Target Platform Intrinsic Functions dotnet/msbuild#5429

Merged

erdembayar reviewed Jun 29, 2020

View reviewed changes

nkolev92 reviewed Jul 1, 2020

View reviewed changes

zkat force-pushed the dev-zkat-net5.0-platforms branch from 1a5176b to c43aa89 Compare July 1, 2020 21:56

zkat requested review from nkolev92, erdembayar and zivkan July 2, 2020 21:00

zkat force-pushed the dev-zkat-net5.0-platforms branch 2 times, most recently from 3c35510 to 0fb3255 Compare July 3, 2020 02:41

add support for net5.0-<platform>

6e1356b

Fixes: NuGet/Home#9682

zkat force-pushed the dev-zkat-net5.0-platforms branch from 0fb3255 to 6e1356b Compare July 3, 2020 02:52

nkolev92 previously approved these changes Jul 7, 2020

View reviewed changes

zivkan reviewed Jul 7, 2020

View reviewed changes

PR feedback

a509f5d

zivkan approved these changes Jul 9, 2020

View reviewed changes

nkolev92 approved these changes Jul 9, 2020

View reviewed changes

make it internal again

5ab918a

zkat merged commit 1f12139 into dev Jul 10, 2020

zkat deleted the dev-zkat-net5.0-platforms branch July 10, 2020 18:22

zivkan mentioned this pull request Jul 11, 2020

Undelete NuGet.Frameworks.NuGetFramework.NuGetFramework(string frameworkIdentifier, System.Version frameworkVersion, string frameworkProfile) -> void NuGet/Home#9784

Closed


		effectivePath = path;

		if (String.IsNullOrEmpty(targetFrameworkString))

	throw new ArgumentNullException("frameworkIdentifier");
	throw new ArgumentNullException(nameof(frameworkIdentifier));

	throw new ArgumentNullException("platform");
	throw new ArgumentNullException(nameof(platform));

		Assert.Throws<System.ArgumentException>(() => target.GetNearest(targetFramework, frameworks));

net5.0-platform support for Pack #3479

net5.0-platform support for Pack #3479

Conversation

zkat commented Jun 26, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 commented Jul 6, 2020

zkat commented Jul 6, 2020

wli3 commented Jul 7, 2020

nkolev92 left a 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

zivkan left a 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

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

zivkan Jul 7, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkan left a comment

Choose a reason for hiding this comment

nkolev92 left a 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

sfoslund commented Jul 10, 2020

rrelyea commented Jul 10, 2020

zivkan commented Jul 16, 2020

zkat commented Jun 26, 2020 •

edited

Loading

zivkan Jul 7, 2020 •

edited

Loading