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

AVRO-2112: Target .NET Standard/Core in C# #307

Merged
merged 15 commits into from
Jan 12, 2019
Merged

AVRO-2112: Target .NET Standard/Core in C# #307

merged 15 commits into from
Jan 12, 2019

Conversation

blachniet
Copy link
Member

Differences from apache/avro master

Target Frameworks

All projects targeted .NET Framework 3.5 both before and after these changes. Some projects target additional frameworks after these changes:

  • Executable projects (including the unit test project) targets .NET Core 2.0
  • Library projects target .NET Standard 2.0
  • All projects target .NET Framework 3.5 & 4.0

Project Target Framework Table

Project Type .NET Standard 2.0 .NET Core 2.0 .NET Framework 3.5 .NET Framework 4.0
Avro.codegen Exe ✔️ ✔️ ✔️
Avro.ipc Library ✔️ ✔️
Avro.ipc.test Unit Tests ✔️ ✔️
Avro.main Exe ✔️ ✔️ ✔️
Avro.msbuild Library ✔️ ✔️ ✔️
Avro.perf Exe ✔️ ✔️ ✔️
Avro.test Unit Tests ✔️ ✔️ ✔️

IPC

I kept running into various issues with unit tests for the IPC project. Instead of holding up the transition to .NET Standard for the
for the other projects. I decided, to move the IPC tests into their own project, Avro.ipc.test. This new project, as well as the
Avro.ipc test, only target .NET Framework 3.5 and 4.0.

See commit 24a5d9c.

Targeting .NET Framework 4.0

We are targeting .NET Framework 4.0 in addition to 3.5 so that end users are not required to install .NET Framework 3.5 just for the
Avro library. From Targeting and running apps for older versions:

In addition, if your app targets version 2.0, 3.0, or 3.5, your users may be required to enable the .NET Framework 3.5 on a Windows 8 or Windows 8.1 computer before they can run your app.

By targeting both 3.5 and 4.0:

  • An app that targets 3.5 still works
  • An app that targets 4.0+ does not require that 3.5 is installed on the end user's machine

NUnit 3

This PR includes all changes from PR #299 (AVRO-2161). At the time of this writing, PR #299 has not been merged into the
apache/avro master branch (PR is still open).

See commit d21d754

NuGet Dependencies

Removed DLLs that we depend on from the lib/ folder and instead refernce their NuGet package equivalents.

Unit Tests

Disabled unit tests that rely on System.CodeDom compilation when targeting .NET Core. See
this comment for a description of why this is necessary. These
tests are enabled when targeting the .NET Framework. See commit 0544c03

The table below shows the number of passing unit tests before and after for each target framework.

.NET 3.5 .NET 4.0 .NET Core 2.0
Before 520 N/A N/A
After 520 520 498*

*.NET Core 2.0 has fewer tests because IPC is not currently targeting .NET Standard/Core, and a few tests relied on System.CodeDom
compilation.

 520
-  3 System.CodeDom compilation tests not supported by .NET Core
- 19 IPC tests - IPC not targeting .NET Standard/Core at this time
====
 498

Support for Later Versions of Newtonsoft.Json

In Newtonsoft.Json v3.5, JToken.ToString() returned the raw JSON representation of the token. In later versions of
Newtonsoft.Json, JToken.ToString() returns a simple string representation of the value. For example:

JToken token = "Hello World";
Console.WriteLine(token.ToString()); 

The code block above prints different values depending on the version of Newtonsoft.Json in use:

  • v3.5: \"Hello World\"
  • Later versions: Hello World

I've updated the project to work with later versions of Newtonsoft.Json as well as v3.5. I've replaced some usages
of JToken.ToString(). When we need the raw JSON representation, we use JsonConvert.Serialize(). When we need the string value
of a string JToken, we use JToken.Value<string>().

See commit d2ce55c

Note that the project still references Newtonsoft.Json v3.5. The changes I describe above enable clients to use later versions of
Newtonsoft.Json without breaking compatibility with Newtonsoft.Json v3.5.

This change fixes the two TestAliases unit tests that were failing in PR #300.

Differences from PR #300

Other Reference Material

@SidShetye
Copy link

hey @blachniet - changes look fantastic. But when running dotnet test we're seeing some issues related with <TargetFrameworks>net35;net40;netcoreapp2.0</TargetFrameworks> inside Avro.test.csproj. Basically net35 doesn't exist on newer Windows systems and net35 and net40 don't exist on *nix platforms.

I'm not sure if this is a limitation of current tools in .net and I don't have any definitive best path. What do you think of having netcoreapp2.0 as the default or pass target framework over the dotnet CLI (if it's even possible)?

Details:

New windows machines lacking .net35

PS D:\projects\github\avro\lang\csharp\src\apache\test> dotnet test
Build started, please wait...
C:\Program Files\dotnet\sdk\2.1.104\Microsoft.Common.CurrentVersion.targets(1126,5): error MSB3644: The reference assemblies for framework ".NETFramework,Version=v3.5" were not found. To resolve this, install the SDK or Targeting Pack for this framework version or retarget your application to a version of the framework for which you have the SDK or Targeting Pack installed. Note that assemblies will be resolved from the Global Assembly Cache (GAC) and will be used in place of reference assemblies. Therefore your assembly may not be correctly targeted for the framework you intend. [D:\projects\github\avro\lang\csharp\src\apache\test\Avro.test.csproj]

Linux, running dotnet test we get

user@linux:/mnt/d/projects/github/avro/lang/csharp/src/apache/test$ dotnet test
Build started, please wait...
/usr/share/dotnet/sdk/2.1.101/Microsoft.Common.CurrentVersion.targets(1126,5): error MSB3644: The reference assemblies for framework ".NETFramework,Version=v4.0" were not found. To resolve this, install the SDK or Targeting Pack for this framework version or retarget your application to a version of the framework for which you have the SDK or Targeting Pack installed. Note that assemblies will be resolved from the Global Assembly Cache (GAC) and will be used in place of reference assemblies. Therefore your assembly may not be correctly targeted for the framework you intend. [/mnt/d/projects/github/avro/lang/csharp/src/apache/test/Avro.test.csproj]

@blachniet
Copy link
Member Author

Hey @SidShetye, I got the same results on my machines (both Windows and Linux) when working on this. The dotnet CLI attempts to build using all frameworks listed in the .csproj. However, you can override this with the --framework argument.

When using dotnet test on either Windows or Linux, you should provide the --framework netcoreapp2.0 argument (mentioned in the README here).

For more details on this error message, see this issue in the MSBuild project. In short, it seems that there is no plans to add support for targeting earlier versions of the .NET Framework (3.5, 4.0) from the dotnet CLI.

Side Note

In this pull request I aimed for additive changes only. That's why I did not deprecate support for .NET Framework 3.5. That being said, I'm happy to deprecate support for earlier versions of the .NET Framework in this project if the rest of the community is happy with that. I'm perfectly happy to target .NET Standard 2.0 (.NET Framework 4.6.1+ according to the chart here). I'm not sure who makes that kind of decision on this project.

@SidShetye
Copy link

@blachniet Thanks, dotnet test --framework netcoreapp2.0 and dotnet test --framework net40 both work without fiddling with the .csproj files. I think your README is very clear too, for some reason I had to run it twice back to back since it failed on first run (likely our test box).

Frankly, I don't really see a big use case where someone will be doing a new project with code from this PR but then targets .NET35.

@busbey Can you please take a look at this PR and fold it in? Also who is responsible for publishing the nuget packages?

@HaroonSaid
Copy link

HaroonSaid commented May 9, 2018

We are looking forward to get .netstandard compatible avro as we looking forward to writing consumer using AWS Lambda and C#

Do you guys have a timeframe when this will be published or how we can build locally?

@blachniet
Copy link
Member Author

I'm not sure when these changes will be available in an official release. There's a lot of changes here, so I imagine it will take some time for the maintainers to review it.

My team is going to need these features soon, as well. I've been releasing some of the features I've been adding recently in the blachniet.Avro NuGet package. I have not released the .NET Standard support in this package yet, but will soon (especially if others need it). You can see some of my additions in the release notes here.

@blachniet
Copy link
Member Author

@HaroonSaid, I pushed a new release of my NuGet package here:

That release contains the changes included in this PR (plus some others). That release has support for targeting .NET Standard 2.0, in addition to .NET Framework 4.5 and 4.6 (see the Target Frameworks table here.

@Fokko
Copy link
Contributor

Fokko commented Nov 12, 2018

@blachniet Can you rebase onto latest master?

@blachniet
Copy link
Member Author

Done, but the build is failing due to some "Files with unapproved licenses". These failures are in the README and generated test files.

@iemejia iemejia added the C# label Nov 29, 2018
@Fokko
Copy link
Contributor

Fokko commented Dec 4, 2018

@blachniet Do you think to either add an exception in pom.xml or add the license.

@Fokko
Copy link
Contributor

Fokko commented Jan 7, 2019

Thanks for picking this up again @blachniet

There are still some issues with the build:

+ msbuild '/t:restore;build' /p:Configuration=Release
./build.sh: line 29: msbuild: command not found

@blachniet
Copy link
Member Author

Hi @Fokko,

Yes, I'm still trying to work thru the build for this guy. When I install Mono in a local container, msbuild is available. I haven't quite figured out what's going wrong yet, but I'm still digging.

blachniet and others added 14 commits January 11, 2019 20:03
In Newtonsoft.Json v3.5, JToken.ToString() returned the raw JSON
representation of the token. In later versions of
Newtonsoft.Json, JToken.ToString() returns a simple string
representation of the value. See the examples below:
- v3.5: "\"Hello World\""
- Later versions: "Hello World"

In this commit, I've updated the project to work with later versions
of Newtonsoft.Json as well as v3.5. I've replaced some usages of
JToken.ToString(). When we need the raw JSON representation, we use
JsonConvert.Serialize(). When we need the string value of a string
JToken, we use JToken.Value<string>().
This is what we were doing on master. I was trying to run the IPC tests,
but that has not seemed to work well in our automated builds.
@Fokko Fokko merged commit 9a05375 into apache:master Jan 12, 2019
@Fokko
Copy link
Contributor

Fokko commented Jan 12, 2019

Thanks for picking this up @blachniet

I also noticed that some tests take a LONG time to run. Can you create a ticket for this to optimize and re-enable the tests again? Thanks!

@blachniet
Copy link
Member Author

Thanks @Fokko! I created AVRO-2301 to address optimizing the tests that I disabled in f035c4c.

@blachniet blachniet deleted the AVRO-2112 branch January 12, 2019 11:21
CalvinKirs added a commit to CalvinKirs/avro that referenced this pull request Nov 20, 2024
https://issues.apache.org/jira/browse/AVRO-1769 The implementation of Jansson has been removed.
apache#307  The implementation of nunit.framework.dll,Newtonsoft.Json.dll and Castle.Core.dll has been removed.
apache#2756 The implementation of m4 macros has been removed.
apache@8a42cd0 Boost.hhThe implementation of Boost.hh has been removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants