-
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
Open
zcsizmadia
wants to merge
4
commits into
apache:main
Choose a base branch
from
zcsizmadia:avro-4087-add-net9
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -134,6 +134,7 @@ jobs: | |
6.0.x | ||
7.0.x | ||
8.0.x | ||
9.0.x | ||
|
||
- name: 'Create Interop Data Directory' | ||
working-directory: . | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.