Fix MilCodeGen #6135

ThomasGoulet73 · 2022-02-16T04:37:39Z

Description

This PR aims to fix MilCodeGen which seems to be very outdated and it doesn't seem like it ever worked in this repo (It probably worked in the internal repo for Microsoft).

Since it does not seem like it ever worked, I did these changes just by deducing how it's supposed to work so this PR might be totally wrong.

I noticed a couple of changes in the generated code other than white-space and license header where I don't know whether the source of truth is the new generated code or the existing generated code (It's mostly comments that differ between both).

This PR unblocks PRs like this one: #5651.

Customer Impact

None.

Regression

No.

Testing

I tested this fix locally using this command build -projects "src\Microsoft.DotNet.Wpf\src\WpfGfx\codegen\mcg\mcg.proj" and by looking at the changes and doing a normal build afterwards.

Risk

I think the risk is pretty low since it's only used for development in this repo and the worse that can happen is to be unable to run MilCodeGen (It didn't work before this PR so it can only be an improvement). All the generated code is checked out in Git so it's easier to validate that the generated code is good.

ThomasGoulet73 · 2022-02-16T04:44:57Z

src/Microsoft.DotNet.Wpf/src/WpfGfx/codegen/mcg/tools/InvokeCSP.cmd

@@ -21,17 +21,8 @@ pushd %~dp0\..

 call %~dp0\SetClrPath.cmd

-copy %CspExePath%\csp.exe %clrpath%


I don't know the context for copying csp to clrpath but it seemed unnecessary.

ThomasGoulet73 · 2022-02-16T04:46:39Z

src/Microsoft.DotNet.Wpf/src/WpfGfx/codegen/mcg/tools/SetClrPath.cmd

@@ -1,6 +1 @@
 set clrpath=%MANAGED_TOOLS_ROOT%\%MANAGED_REFS_VERSION%


I didn't know how to get clrpath automatically. It's the path used by sn.exe but I couldn't find where to get it.

ThomasGoulet73 · 2022-02-16T04:48:57Z

src/Microsoft.DotNet.Wpf/src/WpfGfx/codegen/mcg/tools/GenerateElements.cmd

-
-:: Revert any generated files which haven't changed
-if {%_NoRevertFlag%} EQU {} call %~dp0\tf_GeneratedFiles %OutputDir% %~dp0\GeneratedElements.txt revert -a
+call %~dp0\InvokeCSP.cmd %Options% -rsp:main\Elements.rsp -enableCsPrime -r:"System.Xml.dll" -- %_SdFlag% -xmlFile:xml\Elements.xml -dataType:MS.Internal.MilCodeGen.ResourceModel.CG -o:"%OutputDir%"


I removed the argument -clrdir and the full path in the argument -r. It works without it and avoids the need of finding the path for .Net Framework reference assemblies.

ThomasGoulet73 · 2022-02-16T04:50:48Z

src/Microsoft.DotNet.Wpf/src/WpfGfx/codegen/mcg/main/Elements.rsp

@@ -0,0 +1,19 @@
+-s:main\SimpleGenerator.cs


I created this file and Resources.rsp by hand because they didn't exist. They were probably missed when adding the code in this repo because *.rsp files are excluded in the gitignore.

ThomasGoulet73 · 2022-02-16T04:52:25Z

src/Microsoft.DotNet.Wpf/src/WpfGfx/tools/csp/Project.cs

@@ -96,7 +96,7 @@ Parameters parameters

            // Add some implicit referenced assemblies. 

-            referencedAssemblies.Add(Path.Combine(parameters.ClrDir, "System.dll"));
+            referencedAssemblies.Add(string.IsNullOrEmpty(parameters.ClrDir) ? "System.dll" : Path.Combine(parameters.ClrDir, "System.dll"));


It says in the usage of this project that parameters.ClrDir is optional but it failed here.

kant2002 · 2022-02-16T06:48:18Z

You stole my next target 😄

ThomasGoulet73 · 2022-02-18T00:09:21Z

I pushed a bunch of commit to try to reduce the diff but I couldn't reduce it much further because the bulk of the changes in generated code come from whitespace added because of conditional code which adds an empty line (Example: This code generates these whitespaces).

omariom · 2022-03-08T03:42:28Z

With 308 changed files it's almost impossible to understand what has changed :/

kant2002

I think would be worth to keep build changes from style formatting changes separately. I have hopes that this allow speedup of the review, or at least schedule it for testing.

kant2002 · 2022-03-09T17:24:16Z

...crosoft.DotNet.Wpf/src/PresentationCore/System/Windows/Generated/TextDecorationCollection.cs

@@ -250,12 +250,15 @@ internal void RemoveAtWithoutFiringPublicEvents(int index)

                WritePreamble();

-                if (!Object.ReferenceEquals(_collection[index], value))
+                if (!Object.ReferenceEquals(_collection[ index ], value))


These kind of changes is against current code style. I assume that it would be hard to convince WPF team on this kind of changes. Also they are reduce ability understand what's going on.

kant2002 · 2022-03-09T17:24:32Z

...crosoft.DotNet.Wpf/src/PresentationCore/System/Windows/Generated/TextDecorationCollection.cs

                    OnFreezablePropertyChanged(oldValue, value);

-                    _collection[index] = value;
+                    _collection[ index ] = value;
+


Do these empty lines needed?

kant2002 · 2022-03-09T17:25:34Z

src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/Generated/DashStyle.cs

@@ -236,8 +245,11 @@ void DUCE.IResource.ReleaseOnChannel(DUCE.Channel channel)

                if (_duceResource.ReleaseOnChannel(channel))
                {
+


Why these two lines? I see these a lot. That's confusing.

ThomasGoulet73 · 2022-05-13T21:53:20Z

@kant2002 @omariom Sorry about the delay, I forgot about this PR. All the files are generated using MilCodeGen without any modifications by hand so all the changes are necessary.

It looks like a lot of generated files were formatted when adding the files to this repo.
For example, TextDecorationCollection from referencesource.microsoft.com is a lot closer to this repo with the changes in this PR than without this PR. TextDecorationCollection in this repo with the changes in this PR vs TextDecorationCollection in this repo without the changes in this PR. TextDecorationCollection did not change since the initial commit of PresentationCore.

ThomasGoulet73 · 2024-05-01T01:29:22Z

I rebased, fixed the conflicts and synced MilCodeGen to the generated code since changes to generated files was merged into main.

@dotnet/wpf-developers: Could this be considered for the next test pass ? MilCodeGen is outdated compared to the generated files in the main branch because generated files get changed manually because we cannot run MilCodeGen.

ThomasGoulet73 requested a review from a team as a code owner February 16, 2022 04:37

ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Feb 16, 2022

ghost requested review from dipeshmsft, singhashish-wpf and SamBent February 16, 2022 04:37

ThomasGoulet73 commented Feb 16, 2022

View reviewed changes

kant2002 reviewed Mar 9, 2022

View reviewed changes

ThomasGoulet73 mentioned this pull request Mar 17, 2022

Run some built-in .NET SDK perf analyzers over several WPF assemblies #6274

Merged

ThomasGoulet73 mentioned this pull request May 9, 2022

Remove FriendAccessAllowedAttribute usage #6561

Merged

dipeshmsft self-assigned this Jun 18, 2022

ghost assigned ThomasGoulet73 Jun 18, 2022

pchaurasia14 added the Community Contribution A label for all community Contributions label Jul 20, 2022

ThomasGoulet73 mentioned this pull request Feb 9, 2023

Replace resource constants with properties in PresentationCore #7519

Merged

ThomasGoulet73 added 8 commits April 30, 2024 21:20

Fix milcodegen build

c80ba78

Fix MilCodeGen

4a2e179

Fix whitespace

786fae7

Fix C# license generation

d8c21d5

Fix AnimationTemplate whitespace

eeec3d6

Fix FrameworkElementTemplate.cs

eaa56f3

Sync MilCodeGen to generated code

b1b09e5

Run MilCodeGen

4fdf462

ThomasGoulet73 force-pushed the fix-milcodegen-build branch from 3b939e3 to 4fdf462 Compare May 1, 2024 01:21

ThomasGoulet73 mentioned this pull request Dec 16, 2024

Add readonly to members in Rect #10156

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MilCodeGen #6135

Fix MilCodeGen #6135

ThomasGoulet73 commented Feb 16, 2022 •

edited

Loading

ThomasGoulet73 Feb 16, 2022

ThomasGoulet73 Feb 16, 2022

ThomasGoulet73 Feb 16, 2022

ThomasGoulet73 Feb 16, 2022

ThomasGoulet73 Feb 16, 2022

kant2002 commented Feb 16, 2022

ThomasGoulet73 commented Feb 18, 2022

omariom commented Mar 8, 2022

kant2002 left a comment

kant2002 Mar 9, 2022

kant2002 Mar 9, 2022

kant2002 Mar 9, 2022

ThomasGoulet73 commented May 13, 2022

ThomasGoulet73 commented May 1, 2024 •

edited

Loading

		@@ -21,17 +21,8 @@ pushd %~dp0\..

		call %~dp0\SetClrPath.cmd

		copy %CspExePath%\csp.exe %clrpath%

		@@ -1,6 +1 @@
		set clrpath=%MANAGED_TOOLS_ROOT%\%MANAGED_REFS_VERSION%

		@@ -236,8 +245,11 @@ void DUCE.IResource.ReleaseOnChannel(DUCE.Channel channel)

		if (_duceResource.ReleaseOnChannel(channel))
		{

Fix MilCodeGen #6135

Are you sure you want to change the base?

Fix MilCodeGen #6135

Conversation

ThomasGoulet73 commented Feb 16, 2022 • edited Loading

Description

Customer Impact

Regression

Testing

Risk

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kant2002 commented Feb 16, 2022

ThomasGoulet73 commented Feb 18, 2022

omariom commented Mar 8, 2022

kant2002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasGoulet73 commented May 13, 2022

ThomasGoulet73 commented May 1, 2024 • edited Loading

ThomasGoulet73 commented Feb 16, 2022 •

edited

Loading

ThomasGoulet73 commented May 1, 2024 •

edited

Loading