Updates solution to build for .NET Core #37
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@conniey So how do I build this? My usual |
|
Figured it out. I had to run |
|
What error are you getting? Do you have Visual Studio 2017 Community Edition installed with .NET Core? If you execute |
|
@conniey Got it working on Windows. Do you know how to build this on Linux? |
|
Hey, this looks pretty good. Thanks! A few comments. Reviewed 34 of 34 files at r1. build/icu-dotnet.proj, line 67 at r1 (raw file):
Can we split this into two/three targets? e.g. Compile, CompileStandard, CompileCore where Compile depends on the other two. That way we could at least build part of this project on Linux, even when we can't yet build everything because of missing compatible msbuild version (unless you know how to build on Linux, of course).x source/icu.net/NativeMethods.cs, line 83 at r1 (raw file):
Can this be source/icu.net/NativeMethods.cs, line 253 at r1 (raw file):
This should be source/icu.net/NativeMethods.cs, line 254 at r1 (raw file):
I'm not familiar how source/icu.net/NativeMethods.NetCore.cs, line 1 at r1 (raw file):
File should have the copyright header source/icu.net/NativeMethods.NetCore.cs, line 310 at r1 (raw file):
Can you move this class to a separate file? source/icu.net/Platform.cs, line 1 at r1 (raw file):
Missing copyright header source/icu.net.netstandard/SortKey.cs, line 1 at r1 (raw file):
Missing copyright header source/icu.net/Collation/Collator.cs, line 268 at r1 (raw file):
Looks like an unintended whitespace change here... source/icu.net/Exceptions/BreakException.cs, line 1 at r1 (raw file):
Missing copyright header source/icu.net/Exceptions/IDNAException.cs, line 1 at r1 (raw file):
Missing copyright header source/icu.net/Exceptions/MissingResourceException.cs, line 1 at r1 (raw file):
Missing copyright header source/icu.net/Exceptions/RegexException.cs, line 1 at r1 (raw file):
Missing copyright header source/icu.net/Exceptions/SyntaxErrorException.cs, line 1 at r1 (raw file):
Missing copyright header source/icu.net/Exceptions/TransliteratorParseException.cs, line 1 at r1 (raw file):
Missing copyright header source/icu.net/Exceptions/WarningException.cs, line 1 at r1 (raw file):
Missing copyright header Comments from Reviewable |
|
Can you please rebase on latest master? Thanks! |
Urgh, having troubles building it on Linux due to the GitVersion dependency and full .NET Framework builds.. but I am researching how to build it. In the meantime, I'll split those targets. Related issues: |
conniey
force-pushed
the
netcore
branch
4 times, most recently
from
9a709c9
to
7aeabc9
Compare
|
Review status: 12 of 39 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed. source/icu.net/NativeMethods.cs, line 254 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Yes, the error code from GetLastWin32Error will be the same as long as no other native method has set that value. General rule of thumb is to call it immediate after the native function call, just in case another native method, sets the value to something else. I updated the code. :) Comments from Reviewable |
|
@ermshiperete I updated the PR to support .NET Core building on Linux. The PR description has the updated instructions. |
|
Review status: 12 of 39 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed. build/icu-dotnet.proj, line 67 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
I split these targets into three targets Compile, CompileNetCore and CompileNet40Solution. I also updated the PR description with instructions on how to set-up a build for Linux. source/icu.net/NativeMethods.cs, line 83 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/NativeMethods.cs, line 253 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed. source/icu.net/NativeMethods.NetCore.cs, line 1 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/NativeMethods.NetCore.cs, line 310 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/Platform.cs, line 1 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net.netstandard/SortKey.cs, line 1 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/Collation/Collator.cs, line 268 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/Exceptions/BreakException.cs, line 1 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/Exceptions/IDNAException.cs, line 1 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/Exceptions/MissingResourceException.cs, line 1 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/Exceptions/RegexException.cs, line 1 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/Exceptions/SyntaxErrorException.cs, line 1 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/Exceptions/TransliteratorParseException.cs, line 1 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed source/icu.net/Exceptions/WarningException.cs, line 1 at r1 (raw file): Previously, ermshiperete (Eberhard Beilharz) wrote…
Fixed Comments from Reviewable |
|
What more needs to happen to get official .NET Core support? The temporary icu-dotnet package we are using has fallen behind in features. |
|
@NightOwl888 Mostly, to get this building on their Jenkins CI. The issue is that msbuild (through mono) has a bunch of bugs so I had created a workaround (ie. msbuild,sh) to build the product.. I had checked back a couple of times to get this building through a released version of mono but encountered these issues:
Since they are fixed now... I'll try again to see if I can clean up/fix the Jenkins CI. |
|
@conniey - Thanks! |
|
@ermshiperete The tooling has improved a lot since this PR was opened and I updated it. Would you mind testing it now? Prerequisites
Build/Test on Linux and Windows
NuGet packages |
|
Thanks for doing this. I made a build based on this branch and put together a temporary feed @ (https://www.myget.org/F/lucene-icu-dotnet/api/v2) and integrated with Collation. However, there are a few bugs using my current working branch (https://github.com/NightOwl888/lucenenet/tree/benchmark).
Lucene.Net.Benchmarks.ByTask.TestPerfTasksLogic.TestCollator.NET Framwork 4.5.1With your icu-dotnet MyGet feed this test passes, but a build based on this branch gets a "type initialization exception". .NET Standard 1.5/.NET Core 1.0On .NET Core 1.0 we get a different error message. In this case the "expected" value (since it is mostly zeros I suspect is the incorrect one) is generated by the RuleBasedCollator. Lucene.Net.Collation.TestICUCollationDocValuesField.TestRangesOnly fails on .NET Core 1.0. Fails when calling RuleBasedCollator.Compare() on line 117 of TestICUCollationDocValuesField.cs. It is returning -1, when it should be returning +1. Lucene.Net.Collation.TestICUCollationKeyAnalyzer.TestThreadSafeFailing on .NET Core 1.0 - throws an exception that cannot be caught in .NET Core and crashes NUnit. MyGet FeedI setup a new MyGet account for Lucene.Net so we can have multiple team members manage the feeds. If you don't consider the You can transfer ownership by logging into MyGet, going to the “icu-dotnet” feed, selecting “FEED SECURITY”, click on “Add feed privileges…”, enter “lucenenet” and select “Owner of this feed”. All of the URLs, API keys, etc. remain the same and you will still be able to manage packages there and feed owners. But it won't count against your MyGet storage space anymore and we will be able to manage the packages as well. |
|
BTW - is there any reason this still has a "WIP don't merge yet" label on it? |
|
@conniey, @NightOwl888 sorry this is taking so long to get to it. I'll have to make some changes to our Jenkins infrastructure in order to be able to build it. Unfortunately I didn't get to it before my vacation and longish time away from the office. I'll try to get to it ASAP, but that'll be another few weeks... |
|
Hey Shad, I believe I've fixed the errors you've mentioned. The main issue is that there wasn't a reference to the native icu binaries package. I'll have to double check these on the .NET Core version because I don't have VS2015 installed on this machine. |
|
It seems Microsoft is using an approach for multi-targeting platforms that would be beneficial here. Rather than having the end user download all of the low-level dependencies depending on what platform they are on, there are separate NuGet packages containing the platform-specific dependencies. For example, when you install System.Runtime.Extensions on Windows, it has a dependency on runtime.win.System.Runtime.Extensions. On Linux, it will install runtime.linux.System.Runtime.Extensions. Couldn't this same approach be used to install the full ICU dependency on Windows, but not install the dependency on Linux (by either omitting the dependency or leaving it empty)? Then the end user won't be burdened with having to download a dependency that may or may not be required depending on their environment. Sure, we lose the ability to get the "partial" package, but the only concern with that seems to be a bit of disk space. However, with a NuGet package that only installs on Windows, it would be possible to install the dll in a shared location specific to Windows only in the case it is required. Thoughts? |
|
@NightOwl888 I wonder how Microsoft does that. I don't see anything in the System.Runtime.Extensions nuget package that would specify that dependency. If we can get it to work then it'd be a great improvement to the package. |
… value for building on Linux
…the TargetFrameworks have been built. (If I do it after Build, it'll try to build the package 3 times.)
…lready specified in AssemblyInfo.cs
…efully we can drop this in the future.
|
Thanks @conniey ! Reviewed 3 of 6 files at r2, 19 of 20 files at r3, 1 of 2 files at r4, 4 of 4 files at r5, 7 of 16 files at r6, 13 of 13 files at r7. build/icu-dotnet.proj, line 82 at r7 (raw file):
This comment is really helpful. 👍 build/NuGet.targets, line 65 at r7 (raw file):
One could add the condition: Comments from Reviewable |
|
The icu.net.netstandard.tests fail if Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke. Comments from Reviewable |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
icu.net.netstandard.csprojresults in multiple assemblies that target net40, net451, and netstandard1.6. It is possible to remove icu.net.sln and icu.net.csproj because icu.net.netstandard.csproj builds assemblies that support net40 and more.icu.net.netstandard.testrunner.csprojwas added because NUnit (through dotnet test) does not work on VS2017, yet. Support for .Net Core projects nunit/nunit3-vs-adapter#297Building on Windows
Prerequisites
Build/Package
msbuild /t:Build .\build\icu-dotnet.proj.source\NuGetBuild\netstandard\with the appropriate GitVersion.Test
Option 1
msbuild /t:Test .\build\icu-dotnet.projOption 2
cd source\icu.net.netstandard.testrunner\dotnet runto run testsdotnet run --project <Full path to csproj>Building on Linux (tested on Ubuntu 16.04)
Prerequisites
sudo apt-get install mono-completesudo apt-get install libcurl3sudo apt-get install msbuilddotnet-install.shor throughapt-get.bash <(curl -s https://raw.githubusercontent.com/dotnet/cli/rel/1.0.0/scripts/obtain/dotnet-install.sh) --version 1.0.3Build/Package/Test
If you installed .NET Core using apt-get
export DOTNETSDK=/usr/share/dotnet/sdk/1.0.3chmod +x build/msbuild.sh./build/msbuild.sh /t:Test build/icu-dotnet.projIf you installed .NET Core using dotnet-install.sh
export DOTNETSDK=$HOME/.dotnet/sdk/1.0.3export DOTNETCLI=$HOME/.dotnet/dotnetchmod +x build/msbuild.sh./build/msbuild.sh /t:Test build/icu-dotnet.projIssues
Fixes #23, #8
This change is