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

Revert setter.value changes #9584

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

anjali-wpf
Copy link
Member

@anjali-wpf anjali-wpf commented Aug 16, 2024

Issue : #84

Reverted PRs : #745 and #9229

Description

Reverting a changeset that we've shipped in .NET 9 preview 7 to define Value property of Setter as XAML content property.

The above change increments internal indexing when creating the binary. When a customer installs a new version of the SDK, but the app still targets the old frameworks (.NET 8 and below), the binary created with the new SDK (with the new index information) is not understood by older runtime, which is why the below exception gets thrown.
System.Windows.Markup.XamlParseException with inner IndexOutOfRangeException: Index was outside the bounds of the array.

Customer Impact

All VS users using the new version of SDK and targeting old framework are unable to run their WPF applications. (BLOCKER)

Regression

Yes

Testing

Local VS testing

Risk

Low. Reverted functionality.

Microsoft Reviewers: Open in CodeFlow

@anjali-wpf anjali-wpf requested review from a team as code owners August 16, 2024 04:30
@dotnet-policy-service dotnet-policy-service bot added the PR metadata: Label to tag PRs, to facilitate with triage label Aug 16, 2024
@h3xds1nz
Copy link
Contributor

No way this is being reverted once again :/

@lindexi
Copy link
Member

lindexi commented Aug 16, 2024

Oh, bad news

@anjali-wpf
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@anjali-wpf
Copy link
Member Author

/backport to release/9.0

Copy link

Started backporting to release/9.0: https://github.com/dotnet/wpf/actions/runs/10419998191

@anjali-wpf
Copy link
Member Author

/backport to release/9.0-rc1

@anjali-wpf
Copy link
Member Author

/backport to release/9.0-preview7

Copy link

Started backporting to release/9.0-rc1: https://github.com/dotnet/wpf/actions/runs/10420009398

Copy link

Started backporting to release/9.0-preview7: https://github.com/dotnet/wpf/actions/runs/10420010377

@miloush
Copy link
Contributor

miloush commented Aug 16, 2024

So what is the learning here, are we banning adding ContentProperty attribute to existing classes for backwards compatibility reasons?

@pchaurasia14
Copy link
Member

So what is the learning here, are we banning adding ContentProperty attribute to existing classes for backwards compatibility reasons?

Absolutely not. We need to figure out a better way to manage the compat scenario. In fact, this issue won't be limited to just ContentProperty, but anytime we decide to add new things for PresentationBuildTasks (like x:bind support and more), we will run into the same scenario again.

Right now, given the blocking nature of the issue and the immediate release deadline, we had to make this choice.

We will figure out a way to isolate SDK dependencies with PresentationBuildTasks involved moving forward.

@h3xds1nz
Copy link
Contributor

@pchaurasia14 Happy to hear that; this should also mean that PBT could get rid of .NET 4.7.2 targeting, correct?

@pchaurasia14
Copy link
Member

PBT could get rid of .NET 4.7.2 targeting?

@h3xds1nz - I'm not sure about that yet. During our investigation, we found out that VS uses net472 version of PBT during WPF appl build (even when it is building for dotnet). I am unclear what risk would our removal (for targeting 472) create. My assumption was that net472 version was solely used during netfx appl builds. However, it looks like that's not the case.

But yes, we need to figure out how do we isolate such changes.

@h3xds1nz
Copy link
Contributor

@pchaurasia14 Ouch, I see, thanks for the response. I also thought its just for netfx builds.

Hopefully we get there soon.

@singhashish-wpf
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@singhashish-wpf singhashish-wpf merged commit 5b29d94 into dotnet:main Aug 16, 2024
8 checks passed
@miloush
Copy link
Contributor

miloush commented Aug 22, 2024

PBT could get rid of .NET 4.7.2 targeting?

@h3xds1nz - I'm not sure about that yet. During our investigation, we found out that VS uses net472 version of PBT during WPF appl build (even when it is building for dotnet). I am unclear what risk would our removal (for targeting 472) create. My assumption was that net472 version was solely used during netfx appl builds. However, it looks like that's not the case.

cc @KirillOsenkov

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants