Skip to content
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

Remove workaround for https://github.com/dotnet/source-build/issues/3121 #15254

Closed
wants to merge 2 commits into from

Conversation

mmitche
Copy link
Member

@mmitche mmitche commented Jan 10, 2023

@mmitche mmitche requested a review from a team as a code owner January 10, 2023 23:47
@@ -119,16 +118,6 @@
<WriteLinesToFile File="$(CompletedSemaphorePath)BuildXPlatTasks.complete" Overwrite="true" />
</Target>

<!-- TODO: Remove this when the .NET 8 artifacts tarball no longer includes MicrosoftAspNetCoreAppRuntimePackageVersion -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can be removed until we update the previous source build artifacts to be an 8.0 version. This property is still defined because we are using a 7.0 version of PSB.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you may be right. I think this is the key comment here: dotnet/source-build#3121 (comment)

dotnet/installer#14938 implements it. It combines my patch with @crummel's PR. For 8.0 we should be able to change back to the original property name (MicrosoftAspNetCoreAppRuntimePackageVersion) when the Artifacts tarball no longer includes this name.

I'll check it out.

Copy link
Member

@MichaelSimons MichaelSimons Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I updated PSB in #15324 and merged the changes in here to see if it passes CI now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!!! woo!

@MichaelSimons
Copy link
Member

I guess this was already removed with the PVP work in #15267.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants