Skip to content

Small fix to evaluate ASP.NET targeting patch dynamically. #801

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 8, 2018
Merged

Small fix to evaluate ASP.NET targeting patch dynamically. #801

merged 3 commits into from
Oct 8, 2018

Conversation

crummel
Copy link
Contributor

@crummel crummel commented Oct 5, 2018

Previously we were ending up accidentally setting this to a constant. Also add a bit more detail to the comment about why this is needed.

crummel added 2 commits October 5, 2018 16:15
@crummel
Delay evaluating TargetLatestAspNetCoreRuntimePatch until build time.
013507b 
If this is evaluated in the production build, it ends up being a constant in the final build.
@crummel
Update patch comment to include more details.
ac70fb6
@crummel crummel requested review from omajid and dseefeld October 5, 2018 21:33
@omajid omajid requested a review from tmds October 5, 2018 21:40
@crummel crummel self-assigned this Oct 5, 2018
@omajid
Copy link
Member

omajid commented Oct 5, 2018 •
edited
Loading

For more context, if source-build was built using a source-built sdk, this would previously evaluate to this line in the final Microsoft.NETCoreSdk.BundledVersions.props file:

<TargetLatestAspNetCoreRuntimePatch Condition="'true' == ''">true</TargetLatestAspNetCoreRuntimePatch>"

The condition would evaluate to false, and so the the default value for TargetLatestAspNetCoreRuntimePatch would not be set to true. So some builds of source-build would target ASP.NET Core 2.1.1 by default, which includes known security issues that are fixed in newer versions of ASP.NET Core.

(As for building source-build using the result of a previously-built source-build, we will need to get it to work for #782. This error doesn't seem to happen when source-build is built using the default dotnetcli used by source-build)

omajid
omajid approved these changes Oct 5, 2018
View reviewed changes
Copy link
Member

@omajid omajid left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for taking care of this so quickly!

tmds
tmds approved these changes Oct 6, 2018
View reviewed changes
Copy link
Member

@tmds tmds left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks!

@crummel
Copy link
Contributor Author

crummel commented Oct 8, 2018

@dotnet-bot test OSX10.12 Release please.

2 similar comments
@crummel
Copy link
Contributor Author

crummel commented Oct 8, 2018

@dotnet-bot test OSX10.12 Release please.

@crummel
Copy link
Contributor Author

crummel commented Oct 8, 2018

@dotnet-bot test OSX10.12 Release please.

@crummel crummel merged commit 1e0534a into dotnet:release/2.1 Oct 8, 2018
@omajid omajid mentioned this pull request Oct 31, 2018
Merged
@crummel crummel mentioned this pull request Jan 9, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omajid omajid omajid approved these changes

@tmds tmds tmds approved these changes

@dseefeld dseefeld dseefeld approved these changes

Assignees

@crummel crummel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@crummel @omajid @tmds @dseefeld