You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2020/11/12 15:08:42 UTC

[GitHub] [avro] zcsizmadia opened a new pull request #981: Avro 2961 support net 5

zcsizmadia opened a new pull request #981:
URL: https://github.com/apache/avro/pull/981


   Make sure you have checked _all_ steps below.
   
   ### Jira
   
   - [] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO/) issues and references them in the PR title. For example, "AVRO-1234: My Avro PR"
     - https://issues.apache.org/jira/browse/AVRO-XXX
     - In case you are adding a dependency, check if the license complies with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   
   ### Tests
   
   - [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   
   ### Commits
   
   - [ ] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
     1. Subject is separated from body by a blank line
     1. Subject is limited to 50 characters (not including Jira issue reference)
     1. Subject does not end with a period
     1. Subject uses the imperative mood ("add", not "adding")
     1. Body wraps at 72 characters
     1. Body explains "what" and "why", not "how"
   
   ### Documentation
   
   - [ ] In case of new functionality, my PR adds documentation that describes how to use it.
     - All the public functions and the classes in the PR contain Javadoc that explain what it does
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523851474



##########
File path: lang/csharp/src/apache/test/Reflect/TestFromAvroProject.cs
##########
@@ -189,7 +189,8 @@ public Z SerializeDeserialize(Z z)
             catch (Exception ex)
             {
                 Console.WriteLine(ex.ToString());
-                throw ex;
+                // Note: Re-throwing caught exception changes stack information
+                throw;

Review comment:
       Will do :) I ran into probl;ems when I was rethrowing with ex, and my call stack was very misleading ;)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523866904



##########
File path: lang/csharp/src/apache/main/Schema/SchemaName.cs
##########
@@ -64,7 +64,7 @@ public SchemaName(String name, String space, String encspace)
                 this.EncSpace = encspace;   // need to save enclosing namespace for anonymous types, so named types within the anonymous type can be resolved
             }
 #pragma warning disable CA1307 // Specify StringComparison
-            else if (name.IndexOf('.') == -1)
+            else if (!name.Contains("."))

Review comment:
       ```
   error CA2249: Use 'string.Contains' instead of 'string.IndexOf' to improve readability
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-726153291


   Travis builds are failing... Hmmm. Seems unrelated to the PR


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r526533237



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,63 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+
+<!--
+  See lang/csharp/README.md for tool and library dependency update strategy
+-->
+
+<Project>
+  <!--
+    These package versions are the latest, which are tools are packaged with. 

Review comment:
       ```suggestion
       These package versions are the latest, which our tools are packaged with. 
   ```

##########
File path: lang/csharp/README.md
##########
@@ -12,9 +12,16 @@ Install-Package Apache.Avro
 
 ## Build & Test
 
-1. Install [.NET Core SDK 2.2+](https://www.microsoft.com/net/download/linux)
+1. Install [.NET SDK 5.0+](https://dotnet.microsoft.com/download/dotnet-core)
 2. `dotnet test`
 
+## Dependency package version strategy
+
+1. Use [versions.props](./versions.props) to specify package versions. PackageReference in csproj files should use only version properties defined in [versions.props](./versions.props)

Review comment:
       Just some formatting preferences suggestions 😄 
   ```suggestion
   1. Use [`versions.props`](./versions.props) to specify package versions. `PackageReference` elements in `.csproj` files should use only version properties defined in [`versions.props`](./versions.props).
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet edited a comment on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet edited a comment on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-727567257


   I compiled the following table of updated dependencies in this PR. We should probably list these out in the commit message and/or Jira issue.
   
   | Package                               | Old     | New     |
   |---------------------------------------|---------|---------|
   | Microsoft.CodeAnalysis.FxCopAnalyzers |  2.9.8  |  3.3.1  |
   | Newtonsoft.Json                       | 10.0.3  | 12.0.3  |
   | System.CodeDom                        |  4.4.0  |  5.0.0  |
   | System.Reflection.Emit.ILGeneration   |  4.3.0  |  4.7.0  |
   | System.Reflection.Emit.Lightweight    |  4.3.0  |  4.7.0  |
   | Microsoft.Build.Framework             | 15.6.82 | 16.8.0  |
   | Microsoft.Build.Utilities.Core        | 15.6.82 | 16.8.0  |
   | nunit3testadapter                     |  3.16.1 |  3.17.0 |
   | NUnit.ConsoleRunner                   |  3.10.0 |  3.11.1 |
   | Microsoft.NET.Test.Sdk                | 16.4.0  | 16.8.0  |
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523851771



##########
File path: lang/csharp/src/apache/main/Schema/SchemaName.cs
##########
@@ -64,7 +64,7 @@ public SchemaName(String name, String space, String encspace)
                 this.EncSpace = encspace;   // need to save enclosing namespace for anonymous types, so named types within the anonymous type can be resolved
             }
 #pragma warning disable CA1307 // Specify StringComparison
-            else if (name.IndexOf('.') == -1)
+            else if (!name.Contains("."))

Review comment:
       This was another msbuild 16.8 warning/error. Use Contains instead of cmparing IndexOf() to -1, to make the code "more readable". It is kinda cool how many new warnings they introduced with 16.8 msbuild/net 5




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-730110629


   Did you have something similar in mind as a first iteration? I thought it was kind of important and tied together with the NET 5 release to cleanup up the versioning.
   
   the aspnet or runtime dotnet github repos use many (or more complex package referencing, with similar property tricks, as you said, dependabot might have a way to follow the imported props file.
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r528047610



##########
File path: lang/csharp/README.md
##########
@@ -12,9 +12,16 @@ Install-Package Apache.Avro
 
 ## Build & Test
 
-1. Install [.NET Core SDK 2.2+](https://www.microsoft.com/net/download/linux)
+1. Install [.NET SDK 5.0+](https://dotnet.microsoft.com/download/dotnet-core)
 2. `dotnet test`
 
+## Dependency package version strategy
+
+1. Use [`versions.props`](./versions.props) to specify package versions. `PackageReference` elements in `.csproj` files should use only version properties defined in [`versions.props`](./versions.props).
+
+2. By updating the versions in our libraries, we require users of the library to update to a version equal to or greater than the version we reference. For example, if a user were to reference an older version of Newtonsoft.Json, the would be forced to update to a newer version before they could use a new version of the Avro library.

Review comment:
       Whoops, sorry, this is my typo, and I missed it the last go round.
   ```suggestion
   2. By updating the versions in our libraries, we require users of the library to update to a version equal to or greater than the version we reference. For example, if a user were to reference an older version of Newtonsoft.Json, they would be forced to update to a newer version before they could use a new version of the Avro library.
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-789350309


   @blachniet Thanks fro the merge! Btw it looks like dependabot has no trouble with the versions.props. [](https://github.com/apache/avro/pull/1116/files)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-777749745


   @blachniet
   1. Fixing merge conflicts
   2. [FxCopAnalyzers package has been deprecated in favor of Microsoft.CodeAnalysis.NetAnalyzers](https://docs.microsoft.com/en-us/visualstudio/code-quality/migrate-from-fxcop-analyzers-to-net-analyzers?view=vs-2019)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525835153



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       1. Do you prefer to keep the version.props way of defining package versions? If yes how does that would work with dependabot? I have never used it, so I have no expereince with it at all.
   
   2. Avro.codegen has no explicit package dependency besides Avro.main, so it will inherit the dependencies of Avro.main. If we keep certain packages at a lower version in Avro.main, but still want to use the latest version in Avro.codegen, we must force to use the latest version by explicitly referencing those packages in Avro.codegen.csproj with the latest version.
   If we use versions.props we would need 2 versions for such packages, 1 for the minimum version (e.g. Newtonsoft 10.0.3), and 1 for the latest (e.g. Newtonsoft 12.0.3)
   E.g.
   Avro.main.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="10.0.3" />```
   Avro.codegen.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="12.0.3" />```
   
   This is a legit thing do to IMO, I did forcing packages to keep at a lower version in other projects. This is the opposite, forsing them to use the latest.
    I question if the versiosns.props is a good solution here with the above minimum/latest versions + by using the dependabot.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] RyanSkraba commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525889572



##########
File path: lang/csharp/common.props
##########
@@ -45,6 +45,7 @@
 
   <ItemGroup>
     <None Include="$(MSBuildThisFileDirectory)\LICENSE" Pack="true" Visible="false" PackagePath=""/>
+    <None Include="$(MSBuildThisFileDirectory)\..\..\doc\src\resources\images\avro-logo.png" Pack="true" PackagePath=""/>

Review comment:
       Thanks!  It looks like this change is necessary to build the 1.10.1 release artifacts -- but I managed to cherry-pick your commits into https://github.com/apache/avro/pull/1006, so it should be easy to rebase once they're merged!
   
   (At least, I think the cherry-pick origin info survives the merge-squash... I know your authorship does!)
   
   In any case, thanks for the help.  Especially thanks for the fine-grained and detailed commits :+1: 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-727567257


   I compiled the following table of updated dependencies in this PR. We should probably list these out in the commit message and/or Jira issue.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r526140735



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       @blachniet Modded versions.props and the csproj files to support the dependency update strategy. It makes perfect sense what you described with it. Added comments to README and the csproj files as well.
   
   Checked the Apache.Avro.nuspec (Apache.Avro.nupkg) and it contains the minimum version as expected, and checked Avro.Tools.nupkg and it contains the latest dlls.
   
   However, this might have to be reshuffled and/or turned upside down to work with dependabot ;)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525845580



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       Avro.codegen.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="*" />``` would work too, however, I am not sure how I feel about using an unbound latest version.
   In this case Avro.main is nuspec will specifiy 10.0.3, but Avro.codegen will pick up the latest. Maybe this does not sound too bad? :)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525736588



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       Yea, let's revert the non-private dependencies back to their original versions.
   
   It's perfectly safe to update the private dependencies (e.g. StyleCop and FxCop) and any unit-testing-only dependencies (e.g. NUnit*).
   
   I created [AVRO-2981](https://issues.apache.org/jira/browse/AVRO-2981) to update the dependencies in Avrogen.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-727698481


   > @blachniet BTW do you have any objection or concern about building Avro.codegen for netcoreapp2.1, netcoreapp3.1 and net5.0? I do believe that is the best choice, however the nupkg will be 3x as large.
   
   Added to the JIRA ticket. Let me know how you feel about creating a versions.props file (or adding them to common.props?) with the PACKAGENAMEVersion properties.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet merged pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet merged pull request #981:
URL: https://github.com/apache/avro/pull/981


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-727694409


   @blachniet BTW do you have any objection or concern about building Avro.codegen for netcoreapp2.1, netcoreapp3.1 and net5.0? I do believe that is the best choice, however the nupkg will be 3x as large.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-779314409


   @RyanSkraba Would you consider to approve and merge this PR? I keep fixing merge conflicts , since the  depend bot keeps upgrading minor changes to the used package versions?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525835153



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       1. Do you prefer to keep the version.props way of defining package versions? If yes how does that would work with dependabot? I have never used it, so I have no expereince with it at all.
   
   2. Avro.codegen has no explicit package dependency besides Avro.main, so it will inherit the dependencies of Avro.main. If we keep certain packages at a lower version in Avro.main, but still want to use the latest version in Avro.codegen, we must force to use the latest version by explicitly referencing those packages in Avro.codegen.csproj with the latest version.
   If we use versions.props we would need 2 versions for such packages, 1 for the minimum version (e.g. Newtonsoft 10.0.3), and 1 for the latest (e.g. Newtonsoft 12.0.3)
   E.g.
   Avro.main.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="10.0.3" />```
   Avro.codegen.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="12.0.3" />```
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-754042561


   Merge conflict fixed


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523867124



##########
File path: lang/csharp/src/apache/main/Schema/SchemaName.cs
##########
@@ -64,7 +64,7 @@ public SchemaName(String name, String space, String encspace)
                 this.EncSpace = encspace;   // need to save enclosing namespace for anonymous types, so named types within the anonymous type can be resolved
             }
 #pragma warning disable CA1307 // Specify StringComparison
-            else if (name.IndexOf('.') == -1)
+            else if (!name.Contains("."))

Review comment:
       I  think I added some description to the actual commit message when those files were checked in, however those are kinda lost when you are going through the diff




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r526098778



##########
File path: lang/csharp/common.props
##########
@@ -45,6 +45,7 @@
 
   <ItemGroup>
     <None Include="$(MSBuildThisFileDirectory)\LICENSE" Pack="true" Visible="false" PackagePath=""/>
+    <None Include="$(MSBuildThisFileDirectory)\..\..\doc\src\resources\images\avro-logo.png" Pack="true" PackagePath=""/>

Review comment:
       no worries. I will take care of the merging if there will be any




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523862229



##########
File path: lang/csharp/src/apache/test/Avro.test.csproj
##########
@@ -17,8 +17,8 @@
 <Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
-    <TargetFrameworks Condition="'$(OS)'!='Windows_NT'">netcoreapp3.1</TargetFrameworks>
-    <TargetFrameworks Condition="'$(OS)'=='Windows_NT'">net461;netcoreapp3.1</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)'!='Windows_NT'">net5.0</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)'=='Windows_NT'">net461;net5.0</TargetFrameworks>

Review comment:
       > It may be good for us to continue testing against .NET Core 3.1, just in case there was some compatibility issue between the two.
   
   Good catch. Now thinking about this. I think it should be netcoreapp2.1 added to it (or 2.1 and 3.1). So here is my logic, let me know if I miss or misunderstand something.
   
   Avro.main is compiled for netstandard2.0, netstandard2.1 and netcoreapp2.1. Netstandard2.1 and netcoreapp2.1 (which is btw is very much like netstandard2.1, without having netstandard2.1 support). There is no other framework in Avro.main, since MS recommends to add netcoreapp or other frameweorks only if the library is really compiled with some specific framweork feature/API (using ifdef). SO we are good there.
   
   When Avro.test is tested, all frameworks will be tested in the TargetFramworks list (if no --framework is specified). 
   netf461 will use netstandard2.0 so netstandard2.0 version of Avro.main is tested.
   netcoreapp2.1 fw of Avro.test would be linked against the netcoreapp2.1 version of Avro.main since that specific fw exists in the nuget package and Avro.main.
   netcoreapp3.1 or net5.0 will link against netstandard2.1, since both of them support it, and msbuild will be pick the most appropiate and (best) choice of matching fw.
   
   Honsetly, since the test is very fast, I would add all supported frtameworks to Avro.test. net461,netcoreapp2.1, netcoreepp3.1 and net5.0 (it might test netstandard2.1 2x.).
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523860118



##########
File path: lang/csharp/src/apache/test/Avro.test.csproj
##########
@@ -17,8 +17,8 @@
 <Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
-    <TargetFrameworks Condition="'$(OS)'!='Windows_NT'">netcoreapp3.1</TargetFrameworks>
-    <TargetFrameworks Condition="'$(OS)'=='Windows_NT'">net461;netcoreapp3.1</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)'!='Windows_NT'">net5.0</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)'=='Windows_NT'">net461;net5.0</TargetFrameworks>

Review comment:
       > I compiled the following table of updated dependencies in this PR. We should probably list these out in the commit message and/or Jira issue.
   > 
   > Package	Old	New
   > Microsoft.CodeAnalysis.FxCopAnalyzers	2.9.8	3.3.1
   > Newtonsoft.Json	10.0.3	12.0.3
   > System.CodeDom	4.4.0	5.0.0
   > System.Reflection.Emit.ILGeneration	4.3.0	4.7.0
   > System.Reflection.Emit.Lightweight	4.3.0	4.7.0
   > Microsoft.Build.Framework	15.6.82	16.8.0
   > Microsoft.Build.Utilities.Core	15.6.82	16.8.0
   > nunit3testadapter	3.16.1	3.17.0
   > NUnit.ConsoleRunner	3.10.0	3.11.1
   > Microsoft.NET.Test.Sdk	16.4.0	16.8.0
   
   Usually what I do in .net projects, I add a Versions.prtops file int h eroot and all csproj files include it, just like the Common.props. This file only have PACKAGENAMEVersion defines in them. e.g. "NewtonsoftJsonVersion" = "12.0.3". Every csproj in the source tree will use only PACKAGENAMEVersion defines in the PackageReference, so the version "upgrade" is clean and simple, only change in one place.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-731505442


   Merged my branch with the upstream master. There wasn't any conflict. It should be good to go. Thanks for all your help and suggestion!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525705750



##########
File path: lang/csharp/common.props
##########
@@ -45,6 +45,7 @@
 
   <ItemGroup>
     <None Include="$(MSBuildThisFileDirectory)\LICENSE" Pack="true" Visible="false" PackagePath=""/>
+    <None Include="$(MSBuildThisFileDirectory)\..\..\doc\src\resources\images\avro-logo.png" Pack="true" PackagePath=""/>

Review comment:
       Actually, @RyanSkraba is tackling the logo issue in #1006/([AVRO-2980](https://issues.apache.org/jira/browse/AVRO-2980)).




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523545792



##########
File path: lang/csharp/common.props
##########
@@ -45,6 +45,7 @@
 
   <ItemGroup>
     <None Include="$(MSBuildThisFileDirectory)\LICENSE" Pack="true" Visible="false" PackagePath=""/>
+    <None Include="$(MSBuildThisFileDirectory)\icon.png" Pack="true" PackagePath=""/>

Review comment:
       Could we use the `avro-logo.png` in [avro/doc/src/resources/images/](https://github.com/apache/avro/tree/master/doc/src/resources/images) rather than adding another copy of it here?

##########
File path: lang/csharp/src/apache/main/Schema/SchemaNormalization.cs
##########
@@ -157,7 +157,7 @@ private static StringBuilder Build(IDictionary<string, string> env, Schema s, St
                     {
                         if (!firstTime)
                         {
-                            o.Append(",");
+                            o.Append(',');

Review comment:
       Why all the changes from double to single quotes? Is this necessary? Was there a warning or code hint that was suggesting this?

##########
File path: lang/csharp/src/apache/test/Avro.test.csproj
##########
@@ -17,8 +17,8 @@
 <Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
-    <TargetFrameworks Condition="'$(OS)'!='Windows_NT'">netcoreapp3.1</TargetFrameworks>
-    <TargetFrameworks Condition="'$(OS)'=='Windows_NT'">net461;netcoreapp3.1</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)'!='Windows_NT'">net5.0</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)'=='Windows_NT'">net461;net5.0</TargetFrameworks>

Review comment:
       It may be good for us to continue testing against .NET Core 3.1, just in case there was some compatibility issue between the two.
   
   ```suggestion
       <TargetFrameworks Condition="'$(OS)'!='Windows_NT'">netcoreapp3.1;net5.0</TargetFrameworks>
       <TargetFrameworks Condition="'$(OS)'=='Windows_NT'">net461;netcoreapp3.1;net5.0</TargetFrameworks>
   ```

##########
File path: lang/csharp/src/apache/test/Reflect/TestFromAvroProject.cs
##########
@@ -189,7 +189,8 @@ public Z SerializeDeserialize(Z z)
             catch (Exception ex)
             {
                 Console.WriteLine(ex.ToString());
-                throw ex;
+                // Note: Re-throwing caught exception changes stack information
+                throw;

Review comment:
       Great catch 😄 ! I think you can safely remove the comment here.
   ```suggestion
                   throw;
   ```

##########
File path: lang/csharp/src/apache/main/Generic/GenericRecord.cs
##########
@@ -239,7 +239,7 @@ public override string ToString()
                 sb.Append(contents[field.Pos]);
                 sb.Append(", ");
             }
-            sb.Append("}");
+            sb.Append('}');

Review comment:
       Is this change necessary?

##########
File path: lang/csharp/src/apache/main/Schema/SchemaName.cs
##########
@@ -64,7 +64,7 @@ public SchemaName(String name, String space, String encspace)
                 this.EncSpace = encspace;   // need to save enclosing namespace for anonymous types, so named types within the anonymous type can be resolved
             }
 #pragma warning disable CA1307 // Specify StringComparison
-            else if (name.IndexOf('.') == -1)
+            else if (!name.Contains("."))

Review comment:
       Just out of curiosity, did this come up in a code hint or warning?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia edited a comment on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia edited a comment on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-789350309


   @blachniet Thanks fro the merge! Btw it looks like dependabot has no trouble with the versions.props. [https://github.com/apache/avro/pull/1116/files](https://github.com/apache/avro/pull/1116/files)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523868325



##########
File path: lang/csharp/src/apache/test/Reflect/TestFromAvroProject.cs
##########
@@ -189,7 +189,8 @@ public Z SerializeDeserialize(Z z)
             catch (Exception ex)
             {
                 Console.WriteLine(ex.ToString());
-                throw ex;
+                // Note: Re-throwing caught exception changes stack information
+                throw;

Review comment:
       That is great that MS added the warning in this case. 
   ```
   error CA2200: Re-throwing caught exception changes stack information
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r526140735



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       @blachniet Modded versions.props and the csproj files to support the dependency update strategy. It makes perfect sense what you described with it. Added comments to README and the csproj files as well.
   
   Checked the Apache.Avro.nuspec (Apache.Avro.nupkg) and it contains the minimum version as expected, and checked Avro.Tools.nupkg and it contains the latest dlls.
   
   However, this might have to be reshuffled and/or turned upside down to work with dependabot ;)
   
   + Added some categories to versions.props, just to make easier to recognize which groups those packages belong to




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523867237



##########
File path: lang/csharp/src/apache/main/Schema/SchemaNormalization.cs
##########
@@ -157,7 +157,7 @@ private static StringBuilder Build(IDictionary<string, string> env, Schema s, St
                     {
                         if (!firstTime)
                         {
-                            o.Append(",");
+                            o.Append(',');

Review comment:
       ```
   error CA1834: Use 'StringBuilder.Append(char)' instead of 'StringBuilder.Append(string)' when the input is a constant unit string
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523850745



##########
File path: lang/csharp/common.props
##########
@@ -45,6 +45,7 @@
 
   <ItemGroup>
     <None Include="$(MSBuildThisFileDirectory)\LICENSE" Pack="true" Visible="false" PackagePath=""/>
+    <None Include="$(MSBuildThisFileDirectory)\icon.png" Pack="true" PackagePath=""/>

Review comment:
       My bad :) I checked in the roots but not everywhere I will make the changes to use the existing image.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on pull request #981:
URL: https://github.com/apache/avro/pull/981#issuecomment-735907464


   It seems the latest dockerfile changes made the Travis build much more stable.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525651211



##########
File path: lang/csharp/src/apache/test/Avro.test.csproj
##########
@@ -17,8 +17,8 @@
 <Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
-    <TargetFrameworks Condition="'$(OS)'!='Windows_NT'">netcoreapp3.1</TargetFrameworks>
-    <TargetFrameworks Condition="'$(OS)'=='Windows_NT'">net461;netcoreapp3.1</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)'!='Windows_NT'">net5.0</TargetFrameworks>
+    <TargetFrameworks Condition="'$(OS)'=='Windows_NT'">net461;net5.0</TargetFrameworks>

Review comment:
       Wow, that's great! Yes, I  agree that we should just run all the supported frameworks in Avro.test (net461, netcoreapp2.1, netcoreapp3.1 and net5.0) for the sake of simplicity and since the tests are so quick.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523864832



##########
File path: lang/csharp/common.props
##########
@@ -45,6 +45,7 @@
 
   <ItemGroup>
     <None Include="$(MSBuildThisFileDirectory)\LICENSE" Pack="true" Visible="false" PackagePath=""/>
+    <None Include="$(MSBuildThisFileDirectory)\icon.png" Pack="true" PackagePath=""/>

Review comment:
       Done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523866642



##########
File path: lang/csharp/src/apache/main/Generic/GenericRecord.cs
##########
@@ -239,7 +239,7 @@ public override string ToString()
                 sb.Append(contents[field.Pos]);
                 sb.Append(", ");
             }
-            sb.Append("}");
+            sb.Append('}');

Review comment:
       ```
   error CA1834: Use 'StringBuilder.Append(char)' instead of 'StringBuilder.Append(string)' when the input is a constant unit string
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525722502



##########
File path: lang/csharp/common.props
##########
@@ -45,6 +45,7 @@
 
   <ItemGroup>
     <None Include="$(MSBuildThisFileDirectory)\LICENSE" Pack="true" Visible="false" PackagePath=""/>
+    <None Include="$(MSBuildThisFileDirectory)\..\..\doc\src\resources\images\avro-logo.png" Pack="true" PackagePath=""/>

Review comment:
       Done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525835153



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       1. Do you prefer to keep the version.props way of defining package versions? If yes how does that would work with dependabot? I have never used it, so I have no expereince with it at all.
   
   2. Avro.codegen has no explicit package dependency besides Avro.main, so it will inherit the dependencies of Avro.main. If we keep certain packages at a lower version in Avro.main, but still want to use the latest version in Avro.codegen, we must force to use the latest version by explicitly referencing those packages in Avro.codegen.csproj with the latest version.
   If we use versions.props we would need 2 versions for such packages, 1 for the minimum version (e.g. Newtonsoft 10.0.3), and 1 for the latest (e.g. Newtonsoft 12.0.3)
   E.g.
   Avro.main.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="10.0.3" />```
   Avro.codegen.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="12.0.3" />```
   This is a legit thing do to IMO, I did forcing packages to keep at a lower version in other projects. This is the opposite, forsing them to use the latest.
   

##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       1. Do you prefer to keep the version.props way of defining package versions? If yes how does that would work with dependabot? I have never used it, so I have no expereince with it at all.
   
   2. Avro.codegen has no explicit package dependency besides Avro.main, so it will inherit the dependencies of Avro.main. If we keep certain packages at a lower version in Avro.main, but still want to use the latest version in Avro.codegen, we must force to use the latest version by explicitly referencing those packages in Avro.codegen.csproj with the latest version.
   If we use versions.props we would need 2 versions for such packages, 1 for the minimum version (e.g. Newtonsoft 10.0.3), and 1 for the latest (e.g. Newtonsoft 12.0.3)
   E.g.
   Avro.main.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="10.0.3" />```
   Avro.codegen.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="12.0.3" />```
   
   This is a legit thing do to IMO, I did forcing packages to keep at a lower version in other projects. This is the opposite, forsing them to use the latest.
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r526140735



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       @blachniet Modded versions.props and the csproj files to support the dependency update strategy. It makes perfect sense what you described with it. Added comments to README and the csproj files as well.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r526140735



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       @blachniet Modded versions.props and the csproj files to support the dependency update strategy. It makes perfect sense what you described with it. Added comments to README and the csproj files as well.
   
   Checked the Apache.Avro.nuspec (Apache.Avro.nupkg) and it contains the minimum version as expected, and checked Avro.Tools.nupkg and it contains the latest dlls.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523851230



##########
File path: lang/csharp/src/apache/main/Generic/GenericRecord.cs
##########
@@ -239,7 +239,7 @@ public override string ToString()
                 sb.Append(contents[field.Pos]);
                 sb.Append(", ");
             }
-            sb.Append("}");
+            sb.Append('}');

Review comment:
       The latest msbuild 16.8 has many new warnings (which are mostly seful/valid). The compiler was complaining that StringBuilder.Append(char) should be used if the string arg is a single character. It is more efficient I guess, sinve only a char has to be passed on the stack to the function. There were many Append(string with 1 char) calls, and all of them caused a warning-> error since all warnings are treated as errors.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525669225



##########
File path: lang/csharp/common.props
##########
@@ -45,6 +45,7 @@
 
   <ItemGroup>
     <None Include="$(MSBuildThisFileDirectory)\LICENSE" Pack="true" Visible="false" PackagePath=""/>
+    <None Include="$(MSBuildThisFileDirectory)\..\..\doc\src\resources\images\avro-logo.png" Pack="true" PackagePath=""/>

Review comment:
       Could you set `Visible="false"` so this file doesn't show up in the file tree in Visual Studio (like we did with the `LICENSE` file above)?
   ```suggestion
       <None Include="$(MSBuildThisFileDirectory)\..\..\doc\src\resources\images\avro-logo.png" Pack="true" Visible="false" PackagePath=""/>
   ```
   ![Screen Shot 2020-11-17 at 9 34 47 PM](https://user-images.githubusercontent.com/785131/99475579-d4be5a80-291c-11eb-87ba-a17ebb1320fe.png)
   

##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       👍  I like the idea of the `versions.props` file. I also like that you broke this out from the `common.props` since some projects need to reference things in `versions.props` but not `common.props.
   
   We require users of the libraries to include the following dependencies:
   
   - Avro.main
     - Newtonsoft.Json
     - System.CodeDom
     - System.Reflection
     - System.Reflection.Emit.ILGeneration
     - System.Reflection.Emit.Lightweight
   - Avro.msbuild
     - Microsoft.Build.Framework
     - Microsoft.Build.Utilities.Core
   
   By updating the versions in our libraries, we require users of the library to update to a version equal to or greater than the version we reference. For example, if a user were to reference an older version of Newtonsoft.Json, the would be forced to update to a newer version before they could use a new version of the Avro library.
   
   In short, we should only update the version of the dependencies in our libraries if we absolutely must for functionality that we require. We leave it up to the users of the library as to whether or not they want the latest and greatest of a particularly dependency. We're only going to require the bare minimum.
   
   That said, we should use the latest and greatest in any executables we ship (i.e. avrogen/Avro.codegen) so that we have the latest security fixes in there.
   
   Does all that make sense?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r523865285



##########
File path: lang/csharp/src/apache/test/Reflect/TestFromAvroProject.cs
##########
@@ -189,7 +189,8 @@ public Z SerializeDeserialize(Z z)
             catch (Exception ex)
             {
                 Console.WriteLine(ex.ToString());
-                throw ex;
+                // Note: Re-throwing caught exception changes stack information
+                throw;

Review comment:
       Done.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] blachniet commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
blachniet commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525647353



##########
File path: lang/csharp/src/apache/main/Generic/GenericRecord.cs
##########
@@ -239,7 +239,7 @@ public override string ToString()
                 sb.Append(contents[field.Pos]);
                 sb.Append(", ");
             }
-            sb.Append("}");
+            sb.Append('}');

Review comment:
       Ah, that makes sense. Thanks 👍 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525845580



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       Avro.codegen.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="*" />``` would work too, however, I am not sure how I feel about using an unbound latest version.
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525845580



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       Avro.codegen.csproj: ```<PackageReference Include="Newtonsoft.Json" Version="*" />``` would work too, however, I am not sure how I feel about usinn an unbound latest version.
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [avro] zcsizmadia commented on a change in pull request #981: AVRO-2961: Support .NET 5.0

Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on a change in pull request #981:
URL: https://github.com/apache/avro/pull/981#discussion_r525716998



##########
File path: lang/csharp/versions.props
##########
@@ -0,0 +1,33 @@
+<!--
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       https://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+-->
+<Project>
+  <PropertyGroup Label="Package Versions">
+    <MicrosoftBuildFrameworkVersion>16.8.0</MicrosoftBuildFrameworkVersion>
+    <MicrosoftBuildUtilitiesCoreVersion>16.8.0</MicrosoftBuildUtilitiesCoreVersion>
+    <MicrosoftCodeAnalysisFxCopAnalyzersVersion>3.3.1</MicrosoftCodeAnalysisFxCopAnalyzersVersion>
+    <MicrosoftNETTestSdkVersion>16.8.0</MicrosoftNETTestSdkVersion>
+    <NewtonsoftJsonVersion>12.0.3</NewtonsoftJsonVersion>
+    <NUnitVersion>3.12.0</NUnitVersion>
+    <NUnitConsoleRunnerVersion>3.11.1</NUnitConsoleRunnerVersion>
+    <NUnit3TestAdapterVersion>3.17.0</NUnit3TestAdapterVersion>
+    <StyleCopAnalyzersVersion>1.1.118</StyleCopAnalyzersVersion>
+    <SystemCodeDomVersion>5.0.0</SystemCodeDomVersion>
+    <SystemReflectionVersion>4.3.0</SystemReflectionVersion>
+    <SystemReflectionEmitILGenerationVersion>4.7.0</SystemReflectionEmitILGenerationVersion>
+    <SystemReflectionEmitLightweightVersion>4.7.0</SystemReflectionEmitLightweightVersion>
+  </PropertyGroup>
+</Project>

Review comment:
       Makes sense. Should I downgrade some of the libs? e.g. NewtonsoftJson?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org