-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AVRO-4087: Support .NET 9 #3231
base: main
Are you sure you want to change the base?
Conversation
.NET 9 is released on November 12, 2024. This can be merged once that happens |
<MicrosoftBuildFrameworkVersion>17.12.6</MicrosoftBuildFrameworkVersion> | ||
<MicrosoftBuildUtilitiesCoreVersion>17.12.6</MicrosoftBuildUtilitiesCoreVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These MSBuild version changes worry me. The properties are used in lang/csharp/src/apache/msbuild/Avro.msbuild.csproj
so does the custom task assembly become impossible to load into an older version of MSBuild, e.g. in an older version of Visual Studio? There is a comment above the PropertyGroup that claims these are safe to upgrade, but I'm not sure it is true.
In contrast, the Microsoft.CodeAnalysis APIs are used in tests only, so those packages are safe to uograde.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MSBuilkd package is not distributed to nuget and that source was not maintained for a long time. Additionally IMO it should be retireed from the source code, because avrogen is the tool for code generation and MS recommends not using MSBuild task for more complex cases. Additionally to this AFAIK MSBuild tasks are backwards compatible with older (to a certain level) VS IDEs. Mostly they are netstandard2.0 libraries with standard interfaces and the MSBuild.Framework librariries do not come really into play with the executing VS IDE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Avro.MSBuild is not being published in a NuGet package, then I suppose the version changes are fine and the project is OK to delete altogether.
If it were so published, then I'd prefer requiring only 15.something versions of MSBuild, for Visual Studio 2017 compatibility as per Choose the MSBuild API version to reference
.
As I understand it, the main difficulties with running complex code in an MSBuild task are assembly version conflicts and NuGet packaging. And the benefits are support for more complex output parameters (multiple properties, and items with metadata, for subsequent processing in MSBuild) and better integration with the MSBuild logging system (verbosity settings and HelpLink, which MSBuild itself cannot parse from stdout/stderr streams).
There seems to be some progress towards making custom MSBuild tasks easier to create and package: dotnet/msbuild#10733.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger. I will leave the MSBuild project here for now and lets deal with what version of nuget package we will need once we succesfully revive it.
m
Just to share my memory so at least 2 of us might remeber in the future :) The original issue I faced years ago when I tried to make this a published nupkg was that MSBild tasks have trouble loading external dll dependencies properly. In our case, it was our own Avro main dll. All compiles and builds fine of course, until you need to load the Task nuget package. Lets hope they need some improvement. I remember there was some discussion back then to create a "fat" msbuild task package which contained the avro dlls, however the final decision was to leave it for now.
What is the purpose of the change
AVRO-4087
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Manually verified the change by building the website and checking the new redirect
Documentation