AVRO-3673: improve msbuild task output #2101
Conversation
… revive-avro-msbuild
| limitations under the License. | ||
| --> | ||
| <Project> | ||
| <UsingTask TaskName="Avro.msbuild.AvroGenTask" AssemblyFile="$(MSBuildThisFileDirectory)..\tasks\$(TargetFramework)\Avro.msbuild.dll" /> |
There was a problem hiding this 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
UsingTaskwon't find Avro.msbuild.dll because there is notasks\net45directory 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. Treat TargetFramework(s) values as aliases NuGet/Home#5154
It would be easiest would be to always use netstandard2.0 in AssemblyFile. Alternatively, add conditions on $(MSBuildRuntimeType) and $(MSBuildVersion).
There was a problem hiding this comment.
OTOH, this props file wouldn't even be imported in the crosstargeting build, because it is packed with PackagePath="build\" rather than PackagePath="buildMultiTargeting\".
There was a problem hiding this comment.
Re using $(MSBuildVersion), the idea is that although MSBuild provides neither a built-in property nor a property function for getting the version of .NET on which MSBuild is running, each version of MSBuild on .NET requires some minimum version of .NET, so if $(MSBuildVersion) shows you have that version of MSBuild, then you can assume it can load assemblies built for the corresponding version of .NET. But I don't really see what advantage this would have over always using netstandard2.0.
There was a problem hiding this comment.
understood - pushed change
|
Why is a new PR needed to improve an existing PR? |
Not the same author - so needed to fetch zcsizmadia:revive-avro-msbuild branch to publish my own changes? |
|
The impovement PR should be a PR into the original PR'a branch it is extending instead of avro:master. |
|
so kill this PR and submit to your branch? (actually I already did - I was not sure how active https://github.com/zcsizmadia/avro was. See: https://github.com/zcsizmadia/avro/pull/1) |
|
Yes. This PR will become automatically a multi-author PR, so you your commits will be credited to you. |
What is the purpose of the change
Improve upon #1966 to allow build task chaining
Verifying this change
This change added tests and can be verified by running the C# based unit tests.
Documentation
Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (sample in README.md)