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

Opt into IncludePackageReferencesDuringMarkupCompilation #15465

Merged
merged 3 commits into from
Jan 26, 2021

Conversation

sfoslund
Copy link
Member

@sfoslund sfoslund commented Jan 22, 2021

Fixes #15395

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@clairernovotny clairernovotny left a comment

Choose a reason for hiding this comment

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

I don't know where the right place in the SDK is for this, so I can't speak to that, but otherwise LGTM

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

Normally I would put this in a .targets file, though it would need to come before Microsoft.WinFX.targets where it's defaulted to false.

Is it possible to add a test for the end-to-end scenario here, ie using source generators in a WPF project or one of the other scenarios this unblocks?

@clairernovotny
Copy link
Member

True, a targets file would make it easier for a user to disable it in their csproj if they wanted to.

It should be possible to test for this.

@dsplaisted
Copy link
Member

Note that testing this might require updating to a stage 0 version which has the WPF fix.

@sfoslund
Copy link
Member Author

sfoslund commented Jan 25, 2021

Note that testing this might require updating to a stage 0 version which has the WPF fix.

@dsplaisted it looks like there isn't a 5.0.200 build that can be used for stage 0 that has all the changes. Specifically I'm looking at the latest 5.0.200 build locally (5.0.200-preview.21074.4) and it doesn't contain any of the changes from https://github.com/dotnet/wpf/pull/3846/files

Edit: we haven't updated our wpf version since December 10th, the day before the WPF changes were merged: https://github.com/dotnet/sdk/blame/release/5.0.2xx/eng/Versions.props#L135

@sfoslund
Copy link
Member Author

Test should pass when we get a build to use as stage 0 from this PR: #15492

@dsplaisted
Copy link
Member

I was confused about stage 0, I think we don't actually need a stage 0 round-trip, we just need the WPF update merged.

@sfoslund
Copy link
Member Author

@dsplaisted this is passing locally with the wpf update, are we good to merge once it's green?

@dsplaisted
Copy link
Member

@sfoslund LGTM

@wli3 wli3 merged commit 93993e7 into dotnet:release/5.0.2xx Jan 26, 2021
@sfoslund sfoslund deleted the OptIntoWPFFix branch January 26, 2021 16:28
@sfoslund
Copy link
Member Author

Thanks for merging @wli3!

@bording
Copy link

bording commented Jan 26, 2021

Normally I would put this in a .targets file, though it would need to come before Microsoft.WinFX.targets where it's defaulted to false.

Just curious, if Microsoft.WinFX.targets is defaulting it to false, why not change make the change there to default to true instead of effectively doing that with the change here?

@clairernovotny
Copy link
Member

clairernovotny commented Jan 26, 2021

Normally I would put this in a .targets file, though it would need to come before Microsoft.WinFX.targets where it's defaulted to false.

Just curious, if Microsoft.WinFX.targets is defaulting it to false, why not change make the change there to default to true instead of effectively doing that with the change here?

The reason is that WPF is part of the runtime, ships in 5.0.2/5.0.3 and they can't change the default behavior there. The SDK is expected to change features (in a compatible way) from release to release, so we can flip the default here. WPF has an issue to make it on by default for .NET 6 here: dotnet/wpf#3974

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

Successfully merging this pull request may close these issues.

5 participants