Convert from PCL to .Net Standard 1.1

[ghost]

Currently it compiles, and runs without any errors. However only basic testing has been done so far, but everything appears to be to be working. See #573 for more info. Note: FarseerDuality, and NVorbis have also been convert to .Net Standard 1.1. But because they are kept in different repos, the changes to them are not included. @ilexp when your ready, I'll create pull requests for FarseerDuality, and NVorbis. Note: Some of the edits may or may not be needed, but were included in case they are needed.

[ilexp]

It seems we've got a problem with continuous integration builds using the new netstandard builds. Might be an old build environment. I'll check out how to fix this before we proceed.

[ilexp]

I've adjusted the AppVeyor config to use a VS 2017 setup for building. Can you add another commit to trigger the PRs build checks? Another merge from the netstandard branch into your fork should be fine.

[Barsonax]

You can rebuild the commit in appveyor @ilexp. No need for messy extra commits to trigger it I think.

EDIT: you do need rights to do it so only you could do that.

[ilexp]

You can rebuild the commit in appveyor @ilexp. No need for messy extra commits to trigger it I think.

Rebuilt the PR. Thanks!

Still getting the following error after switching to the VS 2017 environment though, as seen when clicking on the failed build details:

C:\projects\duality\Source\Core\Duality\Duality.csproj(2,1): error MSB4041: The default XML namespace of the project must be the MSBuild XML namespace. If the project is authored in the MSBuild 2003 format, please add xmlns="http://schemas.microsoft.com/developer/msbuild/2003" to the element. If the project has been authored in the old 1.0 or 1.2 format, please convert it to MSBuild 2003 format.

Not exactly sure what this means, or why it happens - any idea?

[Barsonax]

Hmm strange that shouldnt happen in VS2017

https://stackoverflow.com/questions/47714411/appveyoyr-fails-to-build-asp-net-core-webapi-project

Looking at the answer maybe you have to specify the MSbuild version as well? Though I dont recall having to do that in pathfindax and I do use C# 7.x features. Maybe its in the csproj sln?

[ilexp]

Judging from this comment from the thread you linked, it seems like the issue might be a hardcoded msbuild version in the solution file:

Ok so the problem was very simple. It was in fact MsBuild but it was switcing to version 12 no matter what because Rider hardcodes version 12 into sln file, just change the appropriate version targets inside sln file and everything compiles successfully.

In fact, the .sln does contain version numbers for VS 2013.

@Caylis1397 Can you adjust the .sln file so it matches VS 2017?

[Barsonax]

To be doubly sure: In duality its this: # Visual Studio 2013 VisualStudioVersion = 12.0.40629.0

In a new core project its this (which is also the same as in pathfindax): # Visual Studio 15 VisualStudioVersion = 15.0.27130.2027

[ilexp]

Also, there seems to be an additional format version and minimum VS version - do we need to adjust them as well?

[Barsonax]

Well I dont see those being different in a Core project.

Its using version 14? I see this MSBuild auto-detection: using msbuild version '14.0' from 'C:\Program Files (x86)\MSBuild\14.0\bin\amd64' in the log

EDIT: in pathfindax its 15: MSBuild auto-detection: using msbuild version '15.5.180.51428' from 'C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\bin'.

EDIT2: I cant view the appveyor settings to see if anything related to this is different from pathfindax. Is it possible to give me (readonly if you want) rights?

EDIT3: maybe try this?: https://stackoverflow.com/questions/46121024/build-on-appveyor-net-standard-2-0

[ilexp]

@Barsonax Can you export your AppVeyor settings as a YAML file and share the non-secret parts of it?

[Barsonax]

I did it through the UI so I don't have a yml file. I don't know of any export functionality either.

[ilexp]

I did it through the UI so I don't have a yml file. I don't know of any export functionality either.

There's an "Export YAML" option next to the settings :

[Barsonax]

Ah thanks sometimes things are simple :). So here it is:

image: Visual Studio 2017
configuration: Release
shallow_clone: true
assembly_info:
  patch: true
  file: '**\AssemblyInfo.*'
  assembly_version: '{version}'
  assembly_file_version: '{version}'
  assembly_informational_version: '{version}'
nuget:
  project_feed: true
before_build:
- cmd: nuget restore C:\Projects\Pathfindax\Source\Code
build:
  publish_nuget: true
  publish_nuget_symbols: true
  include_nuget_references: true
  verbosity: minimal
artifacts:
- path: Source\Code\Binaries\Release\Pathfindax.dll
  name: Pathfindax.dll
- path: Source\Code\Binaries\Release\Duality.Plugins.Pathfindax.core.dll
  name: Pathfindax.Duality.core.dll
- path: Source\Code\Binaries\Release\Duality.Plugins.Pathfindax.Tilemaps.core.dll
  name: Pathfindax.Duality.Tilemaps.dll
deploy:
- provider: NuGet
  api_key:
    secure: <apikeyhere>
  on:
    APPVEYOR_REPO_TAG: true

[ilexp]

Thanks! Will look into this and see if I can find out where the difference is.

[ilexp]

While investigating this, I found that the solution doesn't compile in VS 2017 on my own machine either. I'm getting different compile errors, but I think it would be worth fixing them first. Maybe they're even throwing off some VS version / SDK auto detection - not sure why exactly that would happen, but you never know.

From what I can see, the NuGet / dependency config is somewhat strange. Take Duality.csproj for example:

It uses the new .csproj format, as expected. However, it also uses a packages.config, which is probably wrong in that context. Instead, it should use PackageReference items in the project file itself. When they're in place, the old assembly references with HintPath items should be removed.

I'm assuming that something similar might be going on with the other converted project. @Caylis1397: Please check! For a simple repro, download your feature branch as a .zip, extract, open the .sln in VS 2017 and try to build.

[ghost]

Sorry about that, I forgot that I had used temporary paths for Farseer, and NVorbis.

Edit: Hmm...It still did not pass the check... Also the reason that I had used temporary paths was that Farseer, and NVorbis were both updated to .net standard 1.1, and their NuGet packages have not yet been updated.

[ilexp]

Edit: Hmm...It still did not pass the check... Also the reason that I had used temporary paths was that Farseer, and NVorbis were both updated to .net standard 1.1, and their NuGet packages have not yet been updated.

Farseer should no longer be a dependency at all, since it was merged into the main repo during 3.0 development. Not sure how it ended up in the feature branch, but that would be a bug.

NVorbis could be tricky though. We probably can't get the project to build without errors as long as there is no compatible netstandard AdamsLair.NVorbis package out there, but we can't publish one either, if that would break the existing Duality release. The regular way of providing different assemblies for different frameworks within the same NuGet package might work, but we'll need to double-check that the old NuGet version that Duality v2.x uses will be able to grab the PCL binaries, while our experimental branch build will get the netstandard ones.

As far as I remember, the current file mapping implementation in the Duality editor PackageManager is rudimentary at best and only test for regular ".NET Framework" cases, so we'll need to check what works and what doesn't and patch it when required.

Wrapping up:

ToDo:

• @Caylis1397: Investigate and fix what happened to this PRs feature branch that it doesn't include Duality.Physics project and still relies on FarseerDuality. Was it branched off from an old version?
• Find out how the NVorbis package spec would need to look like to be supporting both the existing PCL profile, as well as netstandard 1.1
• In a feature branch, update AdamsLair.NVorbis to do a dual build for both PCL Profile 111 and netstandard 1.1, and include both in the package with the proper target frameworks.
• Check what the Duality v2.x package manager will do when confronted with the resulting package.
• If it can't deal with it, fix it and binary-release a patch for v2.x. Make sure to verify it works properly in a local mock setup, potentially add unit tests too.
• Merge back the AdamsLair.NVorbis feature branch, release the package and verify that Duality v2.x installations still work properly.
• Update this netstandard PR feature branch to use the new NVorbis package dependency.
• Re-introduce "aggregate binaries" build steps in each project that were accidentally removed from the csproj files during netstandard conversion.
• Make sure the PR feature branch compiles fine manually.
• Make sure the AppVeyor CI build compiles fine.
• Verify and review this PRs changes to be ready for merge to the main repo feature branch.
• Merge this PR, so the main repo has an up-to-date feature branch for netstandard.
• Close this PR after merge, independently decide when to actually switch to netstandard later on.

I'd like to assign the first item to you @Caylis1397, but would keep the other items open for anyone interested to grab. Will probably look into them myself at some point, but might take a while as this issue isn't high-priority on my current todo.

[ilexp]

Some links that might help with the AppVeyor build issue:

• https://help.appveyor.com/discussions/problems/6894-msbuild-version-for-vs-2017
• https://www.appveyor.com/docs/build-configuration/#appveyoryml-and-ui-coexistence

Especially this part from the second link sounds interesting:

It’s worth noticing that both appveyor.yml and UI configuration are mutually exclusive. It’s always either YAML or UI - the settings from each are not merged. If you have appveyor.yml in your repo it will override all settings made on the UI unless explicitly disabled by Ignore appveyor.yml.

So that's probably the culprit. Will have to update the .yml file with the new build environment, changing project settings is not enough.

ToDo:

• Update appveyor.yml to use a VS 2017 build environment.
• Find out how the NVorbis package spec would need to look like to be supporting both the existing PCL profile, as well as netstandard 1.1
• In a feature branch, update AdamsLair.NVorbis to do a dual build for both PCL Profile 111 and netstandard 1.1, and include both in the package with the proper target frameworks.
• Check what the Duality v2.x package manager will do when confronted with the resulting package.
• If it can't deal with it, fix it and binary-release a patch for v2.x. Make sure to verify it works properly in a local mock setup, potentially add unit tests too.
• Merge back the AdamsLair.NVorbis feature branch, release the package and verify that Duality v2.x installations still work properly.
• Update this netstandard PR feature branch to use the new NVorbis package dependency.
• Re-introduce "aggregate binaries" build steps in each project that were accidentally removed from the csproj files during netstandard conversion.
• Make sure the PR feature branch compiles fine manually.
• Make sure the AppVeyor CI build compiles fine.
• Verify and review this PRs changes to be ready for merge to the main repo feature branch.
• Merge this PR, so the main repo has an up-to-date feature branch for netstandard.
• Close this PR after merge, independently decide when to actually switch to netstandard later on.

[ilexp]

Okay, it was in fact the AppVeyor.yml-overrides-settings issue. Fixed that and now we're getting somewhere. The build still fails, but for different reasons this time. I'm suspecting the package resolve not working correctly due to the inconsistency that I commented above, but not 100% sure. Can you look into this?

Excerpt from the build log:

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets(2041,5): warning MSB3245: Could not resolve this reference. Could not locate the assembly "AdamsLair.WinForms, Version=1.1.14.0, Culture=neutral, processorArchitecture=MSIL". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors. [C:\projects\duality\Source\Editor\DualityEditor\DualityEditor.csproj]

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets(2041,5): warning MSB3245: Could not resolve this reference. Could not locate the assembly "Aga.Controls, Version=1.7.6.0, Culture=neutral, processorArchitecture=MSIL". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors. [C:\projects\duality\Source\Editor\DualityEditor\DualityEditor.csproj]

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets(2041,5): warning MSB3245: Could not resolve this reference. Could not locate the assembly "NuGet.Core". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors. [C:\projects\duality\Source\Editor\DualityEditor\DualityEditor.csproj]

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets(2041,5): warning MSB3245: Could not resolve this reference. Could not locate the assembly "PopupControl". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors. [C:\projects\duality\Source\Editor\DualityEditor\DualityEditor.csproj]

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets(2041,5): warning MSB3245: Could not resolve this reference. Could not locate the assembly "WeifenLuo.WinFormsUI.Docking". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors. [C:\projects\duality\Source\Editor\DualityEditor\DualityEditor.csproj]

[ilexp]

Merging this into feature/3.0/netstandard, as the ToDo list is too big for a single PR, and this looks like it's in good shape for a first step. Moved the ToDo points to issue #573, potentially to be split up into multiple issues later on.