[GitHub] [avro] KalleOlaviNiemitalo commented on a diff in pull request #2101: AVRO-3673: improve msbuild task output

["KalleOlaviNiemitalo (via GitHub)" <gi...@apache.org>]

KalleOlaviNiemitalo commented on code in PR #2101:
URL: https://github.com/apache/avro/pull/2101#discussion_r1108267101


##########
lang/csharp/src/apache/msbuild/Apache.Avro.MSBuild.props:
##########
@@ -0,0 +1,19 @@
+<!--
+   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>
+  <UsingTask TaskName="Avro.msbuild.AvroGenTask" AssemblyFile="$(MSBuildThisFileDirectory)..\tasks\$(TargetFramework)\Avro.msbuild.dll" />

Review Comment:
   Please don't use `$(TargetFramework)` in the AssemblyFile parameter.
   
   This is apparently included in the Apache.Avro.MSBuild package and imported by projects that reference the package. `$(TargetFramework)` is wrong for these reasons:
   
   * If my project targets net6.0 but is built using MSBuild.exe on .NET Framework, then the MSBuild process cannot load an Avro.msbuild.dll that was built for net6.0. It needs an Avro.msbuild.dll that was built for .NET Framework or .NET Standard 2.0.
   * If my project targets net45, then the `UsingTask` won't find Avro.msbuild.dll because there is no `tasks\net45` directory in the package.
   * If my project has multiple target frameworks, then I want to run AvroGenTask just once in the crosstargeting build in which `$(TargetFramework)` is empty, rather than separately for each target framework; the generated files should be identical anyway and this lets the target-framework-specific builds run in parallel without trying to write the same files.
   * TargetFramework might be a custom alias that my project maps to some standard TargetFrameworkIdentifier and TargetFrameworkVersion. <https://github.com/NuGet/Home/issues/5154>
   
   It would be easiest would be to always use netstandard2.0 in AssemblyFile.  Alternatively, add conditions on `$(MSBuildRuntimeType)` and `$(MSBuildVersion)`.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

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