-
Notifications
You must be signed in to change notification settings - Fork 19
Conversation
Some outstanding issues: The avrogen.exe tool build is failing with the following on mac os and linux (however, this is just a build problem, the executable works fine on all platforms):
Some of the Confluent.Kafka.Avro.Integration tests are failing with errors like the below (a type loading error). However, the AvroSpecific and AvroGeneric examples work as expected (and test the same functionality).
|
<PropertyGroup> | ||
<AssemblyOriginatorKeyFile>..\..\..\Avro.snk</AssemblyOriginatorKeyFile> | ||
<VersionPrefix>1.7.7.4</VersionPrefix> | ||
<TargetFramework>netcoreapp2.0</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when 2.1 comes out of preview, we'll target 2.1 and PackAsTool == true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to review csproj files and get an overall picture, but I tried to pick at low hanging fruit.
Removing licenses from files seems questionable.
Some references to the dotnet client.
Otherwise LGTM
@@ -1,166 +1,24 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<!-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are the license headers removed in this file and others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A mistake, glad you caught that one.
<Description>Fork of Apache.Avro.ipc maintained by Confluent</Description> | ||
<Copyright>Apache Foundation</Copyright> | ||
<PackageIconUrl>https://raw.githubusercontent.com/confluentinc/confluent-kafka-dotnet/master/confluent_logo.png</PackageIconUrl> | ||
<PackageReleaseNotes>https://github.com/confluentinc/confluent-kafka-dotnet/releases</PackageReleaseNotes> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably point to avro repo, not dotnet client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whatever logic I was using to make that decision was not sound.
<Description>Fork of Apache.Avro maintained by Confluent</Description> | ||
<Copyright>Apache Foundation</Copyright> | ||
<PackageIconUrl>https://raw.githubusercontent.com/confluentinc/confluent-kafka-dotnet/master/confluent_logo.png</PackageIconUrl> | ||
<PackageReleaseNotes>https://github.com/confluentinc/confluent-kafka-dotnet/releases</PackageReleaseNotes> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ndito
@@ -840,7 +842,7 @@ public virtual void WriteTypes(string outputdir) | |||
for (int j = 0; j < types.Count; j++) | |||
{ | |||
var ctd = types[j]; | |||
string file = dir + "\\" + CodeGenUtil.Instance.UnMangle(ctd.Name) + ".cs"; | |||
string file = Path.Combine(dir, CodeGenUtil.Instance.UnMangle(ctd.Name) + ".cs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
<Description>Fork of Apache.Avro.msbuild maintained by Confluent</Description> | ||
<Copyright>Apache Foundation</Copyright> | ||
<PackageIconUrl>https://raw.githubusercontent.com/confluentinc/confluent-kafka-dotnet/master/confluent_logo.png</PackageIconUrl> | ||
<PackageReleaseNotes>https://github.com/confluentinc/confluent-kafka-dotnet/releases</PackageReleaseNotes> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mdito
<Description>Fork of Apache.Avro.perf maintained by Confluent</Description> | ||
<Copyright>Apache Foundation</Copyright> | ||
<PackageIconUrl>https://raw.githubusercontent.com/confluentinc/confluent-kafka-dotnet/master/confluent_logo.png</PackageIconUrl> | ||
<PackageReleaseNotes>https://github.com/confluentinc/confluent-kafka-dotnet/releases</PackageReleaseNotes> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adito
<Description>Fork of Apache.Avro.test maintained by Confluent</Description> | ||
<Copyright>Apache Foundation</Copyright> | ||
<PackageIconUrl>https://raw.githubusercontent.com/confluentinc/confluent-kafka-dotnet/master/confluent_logo.png</PackageIconUrl> | ||
<PackageReleaseNotes>https://github.com/confluentinc/confluent-kafka-dotnet/releases</PackageReleaseNotes> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ydito
@@ -50,15 +50,15 @@ public void FingerprintTest(string input, string expectedOutput) | |||
|
|||
private static List<object[]> ProvideFingerprintTestCases() | |||
{ | |||
using (StreamReader reader = new StreamReader("../../../../../share/test/data/schema-tests.txt")) | |||
using (StreamReader reader = new StreamReader("../../../../../../../../share/test/data/schema-tests.txt")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So many dots on the dance floor!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH this could just be an artifact of how I'm running the tests, but the tests all need to be upgraded to work well with new tooling anyway, so I didn't spend much time thinking about it.
System.CodeDom
is what we needed to be able to target netstandard2.0 (for .net core compatibility). It's been available for 7 months, but I only just realized it's here.The tests use NUnit 2.6.4, not the lates since NUnit 3.x is a complete rewrite, and it's quite an effort to convert all the tests over. For future reference, to run tests, I used this: https://github.com/nunit/nunit-console/releases/tag/3.8
Unfortunately, there are 11 test failures, so we shouldn't merge until we get to the bottom of them. This is unexpected - there were no code changes required to get the library compiling, and it should have just worked.