Use New JSON Config File; Add .NET Standard 2.0 Target to CKAN-core #2820
Conversation
DasSkelett
added
Architecture
Build
That's the reason it's failing. |
|
Yep. It appears that I may have messed up some of the conditional directives. The .NET 45 target is building successfully, though, which is a good sign. I will hopefully publish a corrected version relatively soon. |
|
Well, from a quick first look, all three UIs still work fine. |
|
And mono < 5.16.0 has some problems... And so on. |
|
@HebaruSan I discussed my personal motivation for this in #2799, and the most immediate consequence is that CKAN.dll becomes significantly more portable. My intention was that the current .NET 4.5 target is unaffected. However, there is a lot of subtlety with build files so there may be unintentional consequences, which I hope should be caught during the review process now. @DasSkelett The mono <5.16.0 issue seems to be a bit more thorny, so I will probably not be able to address it until tomorrow (I'm in the US) Edit: It appears that this issue also only affects the .NET Standard 2.0 build. My first intuition is that the issue stems from the older versions of mono not completely supporting the new build syntax. This should be a relatively easy fix once I configure my dev environment to use the older version—which will take some time. |
|
Allow me to rephrase: I don't know what .NET Standard 2.0 is or why it matters. It's not something I've had to worry about before. |
|
.NET Standard is just a list of APIs. C# code gets compiled into IL (Intermediate Language) which is interpreted by an implementation of Microsoft's CLI (Common Language Infrastructure). Currently, there exist several such implementations. The most commonly known are the traditional .NET Framework and Mono. Some less common implementations include Xamarin.Droid, Xamarin.iOS, and .NET Core (essentially a faster Mono, but made by Microsoft). This creates a problem: each of these implementations supports a slightly different set of the .NET APIs, which means writing code that runs on all of them is challenging—even though this is something developers would really like, especially for core libraries. .NET Standard is Microsoft's attempt to solve this problem. It is a list of the .NET APIs Microsoft considers essential. The way it works is that developers trying to write portable code only use the APIs that are in .NET Standard, and then each of the implementations of the CLI makes sure that they support all of the .NET Standard APIs. Thus, if your code compiles against .NET Standard, it can be run on all of the .NET Standard compliant CLI implementations. Currently, all of the major CLI implementations (every one I listed earlier) is .NET Standard 2.0 compliant, so by making CKAN.dll compile against .NET Standard, you can now use CKAN.dll in each of these implementations. This is relevant to me personally because I am trying to create a Xamarin GUI, which runs on a different CLI implementation than the currently targeted .NET Framework. Another user a few weeks ago wanted to run CKAN on .NET Core, ostensibly for the performance gains, and this also allows them to do that—although, the front end would need to be adapted to work, which is an entirely separate beast. Hopefully this clears things up a little bit, please let me know if any of this doesn't make sense and I can try to clarify. :) |
|
Thanks for the primer! That brings me (hopefully) up to speed. Can we update the .travis.yml file to make it build and test on these other implementations? Currently it would still just try four versions of Mono, so if we added a new dependency that didn't work in .NET Core, we wouldn't know. |
|
It should build for the versions of mono in the travis.yml now (at least it did on my machine—famous last words, I know). Configuring the CKAN.dll unit tests to run on .NET core seems reasonable, and I will look into that in the coming days. In theory, everything except for the Winforms GUI should be able to be ported to .NET core, but that is a bit of an undertaking and would require rewriting every components .csproj in an analogous fashion to how I rewrote CKAN-core.csproj. I'm not strictly opposed to doing this, but I'm not super gung-ho about it either. I agree the #if directives are not ideal. I will look into if there is another way to deal with them. Currently, they are necessary because the .NET Standard and .NET Framework targets depend on slightly different versions of TxFilemanager, which results in the difference in namespace. I will try to either unify the version referenced, or find a way to reduce the needed #if directives, preferably to only one location. Edit: I was able to remove the need for the #if directives by adjusting the TxFilemanager version referenced. At this point, I think all that's left to do is to make the CKAN.dll test cases also run on .NET Core. :) It's worth noting that the .NET Standard and .NET Framework targets also depend on slightly different versions of CurlSharp, but these versions are drop-in replacements, and so don't require any code accommodations. |
|
Soooo... how can I specify which target I want to compile Core against? gives me Equally with |
|
What’s published right now is not quite right, mainly because you can’t do what you just tried to do. My next commit will finish implementing the ability to target .Net Core. It’s taking me longer than I expected to get it working, though, because of some difficulties with NUnit on .Net Core. At this point, I expect I won’t be ready to publish this commit until much later today or early tomorrow. |
|
From a build configuration perspective, I think the addition of .Net Core support is now complete. However, despite things compiling, they don't actually work just yet. The problem is that the Win32 Registry is not supported by .Net Core. Fortunately, barring one instance in KSPPathUtils, all direct interactions with the registry are located in the Win32Registry class. Therefore, it would not be a particularly large undertaking to replace the registry internally. My gut reaction is to ensure all uses go through the IWin32Registry interface, and then do some kind of dependency injection to keep things working the same way on windows, while using a config file on the unix-like platforms. This seems like slightly beyond the stated goals of this pull request, though, so it probably makes sense to create a separate issue and pull request for discussing what to do about the registry. I see three paths forward: 1) Rework the registry as part of this pull request; 2) Mark the .Net Core builds in the travis.yml with allow-failures, complete this pull request, and then rework the registry in a separate pull request; and 3) Rework the registry in a separate pull request, and keep this pull request in limbo until after the registry pull request gets merged. A fourth, unfortunate option is to say that the registry interface should not be touched and abandon this whole undertaking, although I would really prefer not to do that. 😉 |
|
Supposedly https://github.com/dotnet/corefx/issues/31516#issuecomment-409764023 I've tried to read through that issue and I can't quite follow it enough to say. |
Regarding next steps, I think we generally try to avoid leaving any of our projects' master branches in a "broken" state. You never know when the project lead might pop in to publish a new release. So that would imply going for option 1 or 3 in your list. |
|
I'm getting confused again as to the point of this "core" stuff. Wasn't .NET already supposed to be cross platform from the beginning? And indeed, we already have Mono for non-Windows platforms. It seems like we're just hunting for a library that does what Mono already does to simulate the Windows registry. |
|
@jbrot, these last few commits look great. One thought occurred to me: We could migrate all users to the new JSON file, so the registry would no longer be needed after a few releases. The Windows version could load from the registry (as a fallback) and then save to the new JSON file. Any thoughts? Good/bad idea? |
|
@HebaruSan That seems reasonable. It just occurred to me that I need to write migration code, anyways, so that on non-Windows operating systems the new config file gets deployed seamlessly. |
|
I've identified the issue with the .Net Core unit test. It turns out that the test URL "https://spacedock.info/mod/132/Contract%20Reward%20Modifier/download/2.1" redirects to "http://spacedock.info/content/DMagic_247/Contract_Reward_Modifier/Contract_Reward_Modifier-2.1.zip". Importantly, the https url gets redirected to http. The .Net Core implementation of HttpWebRequest correctly identifies this as a security issue, and refuses to redirect automatically. When the HttpWebRequest fails, CKAN then tries to use curl. Both on my computer and on an Ubuntu VM I set up, the curl fallback works flawlessly. The only reason why I noticed something was wrong at all was because on the build server, the curl fallback fails with a cryptic SslConnectError. Note that in this build, I forced CKAN to always use the curl fallback, and this SslConnectError showed up in the .NET Framework build, too. I believe this may be a bug in the version of curl used on the build server, and can probably be fixed by updating curl. So, I see three ways around this. The most direct way would be to create a subclass of WebClient which will follow the redirect and ignore the change from https to http. This is not terribly involved, but if I'm doing a refactor of Net.cs, it should probably be to make it use HttpClient instead of WebClient. Note that the comment around line 390 of Net.cs says "Maybe one day we'll be able to use HttpClient" indicating that, at the time this code was written, HttpClient was not an option. As far as I can tell, HttpClient is available in .NET 4.5 / Mono 4.0.0, so it should be fair game—but perhaps there is some other reason I'm not seeing. An easier way to fix the issue is to simply change the version of curl on the build server, fixing the SslConnect Error. This just masks the issue and doesn't actually resolve it. Furthermore, on Windows, there is no curl fallback, so this won't work everywhere. However, the issue only shows up on the .NET Core version, which will not be seeing widespread use in the immediate future, and certainly not on Windows—so this is not as problematic as it first appears. N.B., it might be a good idea to have the build server run the unit tests on windows and macOS as well, but that is definitely out of scope here. Finally, if we are willing to just mask the issue with a curl update, then there's an even easier way to mask the issue: just disable this specific unit test on .NET Core. Please let me know your thoughts. |
|
Works for me on Linux - even with mono 6.0 ;) - too. |
Also renamed GUI/Configuratino to GUI/GUIConfiguration to avoid a namespace clash, and renamed Win32RegistryJson and Win32RegistryReal to JsonConfiguration and RegistryConfiguration respectively.
Also changed the --where option so that .NET Core also understands it (this was causing occasional build failures).
|
Just did another squash. Let me know if there are any other changes you'd like made, or if it needs to be squashed further. |
| new FileInfo(configFile).Directory.Create(); | ||
|
|
||
| // Write the configuration to the disk | ||
| SaveConfig(); |
There was a problem hiding this comment.
Migrate will also call SaveConfig. Should this be in an #else block?
There was a problem hiding this comment.
I'd like to keep the code in preprocessor blocks to a minimum, so I'd rather just call SaveConfig twice if we are migrating. If you think it's important, though, I'm more than happy to put it in an #else block.
|
We will need to update the pull request validation scripts after this, they currently assume they can reset this config by clearing Mono's fake registry:
(Not to be fixed in this PR, just a note for the future.) |
Also renamed RegistryConfiguration to Win32RegistryConfiguration to clarify this isn't a configuration for the CKAN Registry, and added checks to not create new registry keys when migrating.
|
@HebaruSan The last commit resolves nearly all of your comments. The one area that I did not resolve was with saving the config file excessively when migrating. I think I may have an elegant solution to that, which I will try to implement tomorrow. |
Also added more locks, since I learned that a thread can acquire a lock multiple times.
|
Anything else I can do to facilitate this getting merged? |
Nope, two reviewer approvals is plenty. I just merged it, but GitHub hasn't figured that out yet. Thanks for taking this on! |
This pull request creates two major changes: first, it replaces the use of the Windows registry with a JSON configuration file. This is more idiomatic on platforms other than Windows, and is more portable as the registry is not included in .NET Standard.
The second major change made is to allow CKAN-core to be compiled for .NET Standard 2.0. .NET Standard is an API specification that allows for broad compatibility across .NET implementations. Note that .NET Standard is not a runtime, and so to actually run (and test) the .NET Standard version of CKAN-core, we must pick a .NET Standard 2.0 compliant implementation.
The default target of CKAN, .NET Framework 4.5 is not .NET Standard 2.0 compliant. The first .NET Standard 2.0 compliant version of .NET Framework is .NET Framework 4.6.1. To that end, I added a second target to the Tests project in the same way I added a second target to CKAN-core. The new Tests target runs the .NET Standard 2.0 version of CKAN-core on Microsoft's new cross-platform .NET Core 2.1.
You can switch between the already existing .NET Framework 4.5 target and the new .NET Core 2.1 target through the build configuration. The
DebugandReleaseconfigurations build all of the components against .NET Framework 4.5 (nothing has changed here). The newDebug_NetCoreandRelease_NetCoreconfigurations will just build the .NET Standard 2.0 version of CKAN-core and the .NET Core 2.1 version of Tests.The new configurations work perfectly with Cake. For instance, you can build and test CKAN-core against .NET Core 2.1 by running
./build test --configuration=Debug_NetCore.Visual Studio Mac does not yet support projects with multiple target frameworks, so it can only build the old configurations. If you select theDebug_NetCoreorRelease_NetCoreconfiguration in Visual Studio Mac, it will get confused and not build. I have not tested the new configurations in Visual Studio on Windows, but I suspect it may not like the new configurations either.Note that this pull request lays the groundwork for running the entirety of CKAN on .NET Core, but only actually configures CKAN-core to run on .NET Core. To actually run CKAN on .NET Core, one would need to modify CKAN-ConsoleUI to allow it to be built for .NET Standard 2.0 and modify CKAN-cmdline to run on .NET Core. If more people show interest in #2771 I can make these changes, but at the moment I am more interested in creating a better macOS GUI and will leave them for someone else.
Fixes #2799