Skip to content
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

#711 Updated nuget.core to 2.14 #715

Merged
merged 3 commits into from
Apr 27, 2019
Merged

#711 Updated nuget.core to 2.14 #715

merged 3 commits into from
Apr 27, 2019

Conversation

Barsonax
Copy link
Member

@Barsonax Barsonax commented Apr 3, 2019

TODO:

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

@ilexp ilexp added this to the General milestone Apr 7, 2019
@ilexp ilexp added Editor Area: Duality editor or support libraries Task ToDo that's neither a Bug, nor a Feature labels Apr 7, 2019
@ilexp
Copy link
Member

ilexp commented Apr 7, 2019

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

@ilexp
Copy link
Member

ilexp commented Apr 14, 2019

Cleared block from issue #716.

@ilexp
Copy link
Member

ilexp commented Apr 16, 2019

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
Copy link
Member Author

There seems to be a breaking change in nuget:
image

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

@Barsonax
Copy link
Member Author

IsLatestVersion is somehow wrongly set on the 1.0.0.0 TestPlugin package:
image

@Barsonax
Copy link
Member Author

Barsonax commented Apr 16, 2019

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

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

@Barsonax
Copy link
Member Author

Barsonax commented Apr 16, 2019

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
Copy link
Member Author

Barsonax commented Apr 16, 2019

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...

…sitory because of LazyLocalPackageRepository
@Barsonax
Copy link
Member Author

TODO:

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

@ilexp
Copy link
Member

ilexp commented Apr 17, 2019

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
Copy link
Member Author

Barsonax commented Apr 17, 2019

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
Copy link
Member

ilexp commented Apr 17, 2019

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

Will do as soon as I manage.

@ilexp
Copy link
Member

ilexp commented Apr 20, 2019

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
Copy link
Member

ilexp commented Apr 20, 2019

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
Copy link
Member Author

No that test still fails. Created issue about it: #719

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

@ilexp
Copy link
Member

ilexp commented Apr 24, 2019

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 ilexp merged commit 3435ba2 into AdamsLair:master Apr 27, 2019
@ilexp
Copy link
Member

ilexp commented Apr 27, 2019

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

@ilexp ilexp modified the milestones: General, C# / .NET Upgrade Jul 28, 2019
@Barsonax Barsonax deleted the feature/#711_nuget_2.14_upgrade branch April 28, 2020 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editor Area: Duality editor or support libraries Task ToDo that's neither a Bug, nor a Feature
Development

Successfully merging this pull request may close these issues.

2 participants