#711 Updated nuget.core to 2.14

Barsonax commented 5 years ago

TODO:

[x] Update nuget.core to 2.14 to support netstandard (discovered in #710)
[x] Find out why IsLatestVersion is wrongly set to true for AdamsLair.Duality.TestPlugin 1.0.0.0 in PackageManagerTests.GetLatestDualityPackages
[x] Come up with a fix for the above problem
[x] Verify package installs with the official sample packages work as expected.
[x] Verify package installs with multi-framework targets work as expected (for example Singularity v0.9)
[x] Build a binary download .zip with the new NuGet and verify both install and restore work as expected.
[x] After the merge, upload a new binary .zip to replace the current one under releases.

ilexp commented 5 years ago

Related issue is #711, to be closed when this is merged.

ilexp commented 5 years ago

Cleared block from issue #716.

ilexp commented 5 years ago

What's the state of this PR, e.g. which are the ToDos that are still to be done before review? Only seeing "Test Test Test" in the main comment, not sure what exactly that means 😄 (Especially since the build / tests appear to be failing right now)

Barsonax commented 5 years ago

There seems to be a breaking change in nuget:

Also VertexBatchTest.Locking fails for me locally (but not on appveyor?) not sure why.

Barsonax commented 5 years ago

IsLatestVersion is somehow wrongly set on the 1.0.0.0 TestPlugin package:

Barsonax commented 5 years ago

Modified code in PackageManager.cs a bit to catch the bug earlier in the process. Seems to have something to do with the LazyLocalPackageRepository:

So atleast that rules out a bug in our custom NuGetTargetedPackageManager class since this happens before this class is even created.

Barsonax commented 5 years ago

Nuget v2.8.5 is the last version that doesn't exhibit this behavior. All versions from v2.8.6 on seem to have this bug.

EDIT: LazyLocalPackageRepository is just a lazy wrapper around LocalPackageRepository. However there are changes in LocalPackagesRepository. See the comparison here: https://github.com/Barsonax/NuGet2/pull/1/files#diff-7c6f995120e8d40ffdb3cfc81c06c279R1

Barsonax commented 5 years ago

Ah I see we have a custom piece of code that sets the is latest. This piece of code expects a LocalPackageRepository to be present. However from nuget 2.8.6 on its now wrapped in a LazyLocalPackageRepository so it fails to spot its a local package repo. Fix should be easy.

EDIT: next build should succeed...

Barsonax commented 5 years ago

TODO:

perform some real tests to verify the package manager is working as expected.

ilexp commented 5 years ago

Great work, looking forward to the test results 👍

Also VertexBatchTest.Locking fails for me locally (but not on appveyor?) not sure why.

This could be related to semi-unsafe code, we should verify it's not an x86 / x64 disparity or something like that. Can you open a new issue detailing the exact failure, as well as the test init logs regarding processor architecture and process modes?

Barsonax commented 5 years ago

Did the following tests:

Install singularity 0.9, this is a multi framework package that targets net 4.5.2 and netstandard 2.0. It picks the net 4.5.2 version and does not complain about the netstandard target which is correct!
Installed some other package such as tilemaps or the sample packages.

@ilexp I guess you want to take a look as well before merging this?

ilexp commented 5 years ago

@ilexp I guess you want to take a look as well before merging this?

Will do as soon as I manage.

ilexp commented 5 years ago

Progress

Fixed an unrelated, but blocking bug in master branch package management.
Debug-stepped through unit tests for packages and verified the new netstandard framework can now be parsed properly by NuGet.
Tested package install, uninstall and restore with various official and non-official plugin and sample packages.

ToDo

Merge back to master, bump version numbers and trigger a new binary release.
After the merge, upload a new binary .zip to replace the current one under releases.
Do a final "out in the wild" test.

Looks fine to me, I'd say we're ready for merge. Will proceed when I find the next sufficiently large time frame to do so without a rush.

ilexp commented 5 years ago

Also VertexBatchTest.Locking fails for me locally (but not on appveyor?) not sure why.

@Barsonax Any info on this? Is this still an issue?

Barsonax commented 5 years ago

No that test still fails. Created issue about it: https://github.com/AdamsLair/duality/issues/719

Seems to have nothing to do with this PR so lets merge this and look into that issue afterwards.

ilexp commented 5 years ago

Seems to have nothing to do with this PR so lets merge this and look into that issue afterwards.

Agreed. Proceeding as outlined above and as time allows 👍

ilexp commented 5 years ago

Merged, triggered a package release, and updated the download package.

AdamsLair / duality

#711 Updated nuget.core to 2.14 #715

Progress

ToDo