-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Refactor wasmappbuilder's System.Text.Json reference to use a versions.props property #70582
Refactor wasmappbuilder's System.Text.Json reference to use a versions.props property #70582
Conversation
… versions.props property
Why doesn't this fail for the non-source-build? Also, this needs to be tested on windows too. @lambdageek do you think this might cause any issues on windows? |
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsThe current hardcoded version reference causes issues with source-build:
Refactoring the wasmappbuilder.csproj to utilize a versions.props property for the System.Text.Json version will correct this issue.
|
Linux/AOT test failing with:
|
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
The issue on windows in the past was when we requested a version that was newer than what comes with the visual studio version of msbuild - which caused problems with running the task from inside VS. |
This pull request has been automatically marked |
@@ -21,7 +21,7 @@ | |||
<PackageReference Include="Microsoft.Build" Version="$(MicrosoftBuildVersion)" /> | |||
<PackageReference Include="Microsoft.Build.Tasks.Core" Version="$(MicrosoftBuildTasksCoreVersion)" /> | |||
<PackageReference Include="System.Reflection.MetadataLoadContext" Version="4.7.1" /> | |||
<PackageReference Include="System.Text.Json" Version="6.0.0" PrivateAssets="all" /> | |||
<PackageReference Include="System.Text.Json" Version="$(SystemTextJsonVersion)" PrivateAssets="all" /> |
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.
<PackageReference Include="System.Text.Json" Version="$(SystemTextJsonVersion)" PrivateAssets="all" /> | |
<!-- When we requested a version that is newer than what comes with Visual Studio version of msbuild, it causes problems with | |
running the task from inside VS. Therefore, we are using the moving version only for source-build. --> | |
<PackageReference Include="System.Text.Json" Condition="'$(DotNetBuildFromSource)' != 'true'" Version="6.0.0" PrivateAssets="all" /> | |
<PackageReference Include="System.Text.Json" Condition="'$(DotNetBuildFromSource)' == 'true'" Version="$(SystemTextJsonVersion)" PrivateAssets="all" /> |
@lambdageek this?
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.
That seems reasonable to me, thanks!
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.
I applied the suggested pattern.
This pull request has been automatically marked |
@@ -20,8 +20,12 @@ | |||
|
|||
<PackageReference Include="Microsoft.Build" Version="$(MicrosoftBuildVersion)" /> | |||
<PackageReference Include="Microsoft.Build.Tasks.Core" Version="$(MicrosoftBuildTasksCoreVersion)" /> | |||
<PackageReference Include="System.Reflection.MetadataLoadContext" Version="4.7.1" /> | |||
<PackageReference Include="System.Text.Json" Version="6.0.0" PrivateAssets="all" /> | |||
<PackageReference Include="System.Reflection.MetadataLoadContext" Version="$(SystemReflectionMetadataLoadContextVersion)" /> |
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.
I realized we need to do the same for the MetadataLoadContext. Can this reference a shared versions.props property of should if follow the suggested source-build conditioned pattern?
@kasperk81, @lambdageek - Any objects to merging this PR? |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
The current hardcoded version reference causes issues with source-build:
Refactoring the wasmappbuilder.csproj to utilize a versions.props property for the System.Text.Json version will correct this issue.