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

Wpf depends on packages no longer produced in runtime #3173

Closed
mmitche opened this issue Jun 24, 2020 · 10 comments
Closed

Wpf depends on packages no longer produced in runtime #3173

mmitche opened this issue Jun 24, 2020 · 10 comments
Assignees
Labels
Bug Product bug (most likely)
Milestone

Comments

@mmitche
Copy link
Member

mmitche commented Jun 24, 2020

WPF has two dependencies that are no longer produced in runtime for 5.0:

System.Reflection.Emit: https://github.com/dotnet/wpf/blob/master/eng/Version.Details.xml#L34-L37
Microsoft.NETCore.Runtime.CoreCL: https://github.com/dotnet/wpf/blob/master/eng/Version.Details.xml#L82-L85

Both of these are stuck at alpha.1 and cannot move forward.

@mmitche
Copy link
Member Author

mmitche commented Jun 24, 2020

@fabiant3 @ryalanms

@mmitche mmitche changed the title Wpf depends on packages no longe produced in runtime Wpf depends on packages no longer produced in runtime Jun 24, 2020
@fabiant3 fabiant3 added this to the 5.0.0 milestone Jun 24, 2020
@fabiant3 fabiant3 added the Bug Product bug (most likely) label Jun 24, 2020
@fabiant3
Copy link
Member

@mmitche - is there a process, tasks filled, how can we be part of these runtime changes that are breaking the WPF repo, so in the future we don't have to react to them?

@mmitche
Copy link
Member Author

mmitche commented Jun 24, 2020

Not really. Keep in mind that this isn't actually breaking the repo. These got stuck at alpha.1 from way way back. This is more a heads up as to these not being produced anymore. I'm not sure what you should do here. I'm guessing pinning S.R.E at a 4.7.0 version is right, though not sure about the Coreclr dep. Now you might think that not producing a binary would be something that would be a break, but it's actually quite common. In servicing, packages pop in and out of being built all of the time for very normal reasons (incremental servicing).

There are of a course another class of breaks. One where a new runtime build actually breaks WPF or winforms or whatnot. Again, the system flowing dependencies has no insight (nor can it) as to what is a breaking change or not. A bug can be a breaking change, as well as API change or removal. For these kinds of changes I think it's important that we just get better at communicating between teams, as well as staying on top of the dependency update PRs (where these breaks typically show up)

@vatsan-madhavan
Copy link
Member

You can remove Microsoft.NETCore.Runtime.CoreCLR dependency since the latest versions of Microsoft.NetCore.ILAsm/ILDasm produce self-contained binaries that do not require separate deployment of CoreCLR. Older versions required ILASM (in 3.x timeframe) to be assembled with corresponding CoreCLR separately before it could be used. InjectModuleInitializer.targets/CopyILTools target can be simplified to take this new information into account, for example.

Technically, these are not even in use today. UseNetCoreILTools is not set to true, since .NET Core version ILAsm didn't support generating PDB's with debug information. If this has been fixed now (someone should follow-up), then the build should be switched to use these .NET Core based IL tools (the older IL tools - from .NET Framework toolset - could prevent the build from supporting newer concepts like nullables in PresentationCore. In general, the older tools may fail if they can't successfully decompile + recompile binaries from the latest C# compiler. Thus it's better to switch to .NET Core version of IL tools as soon as their PDB support is fixed).

Ultimately, its best to remove InjectModuleInitializer.targets entirely and switch to the upcoming C# 9 support for module initializers. dotnet/csharplang#2608

@rladuca
Copy link
Member

rladuca commented Jul 14, 2020

@ryalanms IL(D)Asm now supports generating PDBs, so it may be possible to switch off of the .NET Framework IL(D)Asm used for generation of the module initialize in PresentationCore. You'll have to weigh this against waiting for the C# support for module initializers, but I believe you could swap at this point.

@vatsan-madhavan
Copy link
Member

@AaronRobinsonMSFT
Copy link
Member

dotnet/csharplang#2608 doesn't seem to be making a lot of progress.

@vatsan-madhavan Module initializers should be in Roslyn main this week. The codegen was done at dotnet/roslyn#43281. The work all up can be found at dotnet/roslyn#40500.

Also note that moving over to the NET Core IL tools will eventually permit the use of the new unmanaged keyword for function pointers - yay!

@rladuca
Copy link
Member

rladuca commented Jul 14, 2020

@AaronRobinsonMSFT If WPF can rely on the module initializer work in the near future there is no need for IL tools at all. First class C# module initializer support wholly replaces the WPF use case. That will also allow WPF to march forward with new language features in its code base without the worry of breaking the older IL tools.

@vatsan-madhavan
Copy link
Member

vatsan-madhavan commented Jul 14, 2020

Agree with Rob - if C# 9 module initializers can be used, getting rid of these targets entirely is the right move.

@ryalanms
Copy link
Member

The IL injection for the module initializer has been removed in 6.0.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Product bug (most likely)
Projects
None yet
Development

No branches or pull requests

6 participants