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

Sync Csc invocations from CoreCompile to XamlPreCompile #9786

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rainersigwald
Copy link
Member

@rainersigwald rainersigwald commented Feb 26, 2024

The Csc task invocation in XamlPreCompile has drifted from the one in CoreCompile in the Roslyn repo. I'm attempting to bring them back into sync.

This will fix #9785.

I did this in two parts to make future updates easier:

  • 18671e8 is a clean copy/paste of the Csc & Vbc calls from Roslyn.

  • The subsequent commit modifies them to (hopefully) have the differences we expect here.

In the future, we can update from Roslyn by syncing to caa45ea, making a new commit of a clean update copy/paste of the task, and then merging with the subsequent edits.

@rainersigwald rainersigwald requested a review from a team February 26, 2024 18:14
Copy link
Member Author

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

@jaredpar Here are some comments on the changes I'm proposing from CoreCompile. Would love to get compiler team review on them please.

Comment on lines 256 to 257
EmbedAllSources="$(EmbedAllSources)"
EmbeddedFiles="@(EmbeddedFiles)"
Copy link
Member Author

Choose a reason for hiding this comment

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

My reasoning here is that embedding stuff is for final outputs and isn't needed here. Should we explicitly pass false/empty though?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's important that we keep all of the properties in <Csc here. That way when we look at the targets file we can quickly assert whether or not properties are missing.

Features="$(Features)"
InterceptorsPreviewNamespaces="$(InterceptorsPreviewNamespaces)"
FileAlignment="$(FileAlignment)"
GeneratedFilesOutputPath="$(CompilerGeneratedFilesOutputPath)"
Copy link
Member Author

Choose a reason for hiding this comment

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

I assume this isn't relevant to the precompilation step but would like confirmation.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately everything but FileAlignment is necessary. The rest all impact compilation success / failure.

Nullable="$(Nullable)"
Optimize="$(Optimize)"
Deterministic="$(Deterministic)"
PublicSign="$(PublicSign)"
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we keep PublicSign?

Deterministic="$(Deterministic)"
PublicSign="$(PublicSign)"
OutputAssembly="@(IntermediateAssembly)"
OutputRefAssembly="@(IntermediateRefAssembly)"
Copy link
Member Author

Choose a reason for hiding this comment

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

I think ref assembly outputs aren't useful here.

PreferredUILang="$(PreferredUILang)"
ProjectName="$(MSBuildProjectName)"
ProvideCommandLineArgs="$(ProvideCommandLineArgs)"
References="@(ReferencePathWithRefAssemblies)"
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why this was using the old @(ReferencePath) before. Scared to change it but filed #9787.

ProjectName="$(MSBuildProjectName)"
ProvideCommandLineArgs="$(ProvideCommandLineArgs)"
References="@(ReferencePathWithRefAssemblies)"
RefOnly="$(ProduceOnlyReferenceAssembly)"
Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely need to not be RefOnly--should we explicitly pass false?

References="@(ReferencePathWithRefAssemblies)"
RefOnly="$(ProduceOnlyReferenceAssembly)"
ReportAnalyzer="$(ReportAnalyzer)"
ReportIVTs="$(ReportIVTs)"
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what this does, should it run in this context?

RuntimeMetadataVersion="$(RuntimeMetadataVersion)"
SharedCompilationId="$(SharedCompilationId)"
SkipAnalyzers="$(_SkipAnalyzers)"
SkipCompilerExecution="$(SkipCompilerExecution)"
Copy link
Member Author

Choose a reason for hiding this comment

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

Likewise SkipCompilerExecution seems like it wouldn't be necessary.

Comment on lines 319 to 320
PathMap="$(PathMap)"
SourceLink="$(SourceLink)">
Copy link
Member Author

Choose a reason for hiding this comment

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

These feel like they belong only in final outputs so no need to pass them here, right?

References="@(ReferencePath)"
RemoveIntegerChecks="$(RemoveIntegerChecks)"
ReportAnalyzer="$(ReportAnalyzer)"
Resources="@(_CoreCompileResourceInputs);@(CompiledLicenseFile)"
ResponseFiles="$(CompilerResponseFile)"
RootNamespace="$(RootNamespace)"
PdbFile="$(PdbFile)"
Copy link
Member Author

Choose a reason for hiding this comment

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

This shouldn't have been there right? We don't generally want to produce symbols for the Xaml precompilation.

Copy link
Member

Choose a reason for hiding this comment

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

Logically yes I agree there is no reason for PDB to be produced here. At the same time what is the best way to do this? I think controlling DebugType="None" is the more holistic approach then also pass an empty string here.

@jaredpar
Copy link
Member

@RikkiGibson, @chsienki, @jjonescz FYI

@RikkiGibson
Copy link

I do not think I understand what the XamlPreCompile step is doing well enough to say which arguments can be left out or not. I would lean toward including all the same arguments from the original task invocation from Roslyn as much as possible, except for those where there is a specific detrimental effect to keeping them.

@jaredpar
Copy link
Member

@RikkiGibson part of my motivation for pinging is making you all aware that this exists. It was something I was unaware of.

I would lean toward including all the same arguments from the original task invocation from Roslyn as much as possible, except for those where there is a specific detrimental effect to keeping them.

Largely agree. I'm okay with a few items like disable PDB generation but overall would keep same where possible. In cases we do change arguments would use explicit values vs. implicit defaults to make it clear the item was considered.

These are the adjustments to the standard Csc invocation that we want to
apply to this precompilation process. Most important are the
OutputAssembly adjustment (to avoid overwriting final output) and
SkipAnalyzers=true (for performance and warning deduplication).
@rainersigwald
Copy link
Member Author

Largely agree. I'm okay with a few items like disable PDB generation but overall would keep same where possible. In cases we do change arguments would use explicit values vs. implicit defaults to make it clear the item was considered.

This makes total sense and I've just tried to do so. I think it's probably best at this point to try an experimental VS with this contents and validate that we can build some WPF projects--I'll do that. Marking this as "draft" until that's done.

@rainersigwald rainersigwald marked this pull request as draft April 25, 2024 16:13
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.

Error CS9137 Occurred When Building a MAUI Project Targetting to Windows
3 participants