Sync Csc invocations from CoreCompile to XamlPreCompile#9786
Sync Csc invocations from CoreCompile to XamlPreCompile#9786rainersigwald merged 2 commits intodotnet:mainfrom
Conversation
rainersigwald
left a comment
There was a problem hiding this comment.
@jaredpar Here are some comments on the changes I'm proposing from CoreCompile. Would love to get compiler team review on them please.
Exact copy/paste of * https://github.com/dotnet/roslyn/blob/39848683a068122de144949117a9e5111bfd42ba/src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets#L84-L169 * https://github.com/dotnet/roslyn/blob/39848683a068122de144949117a9e5111bfd42ba/src/Compilers/Core/MSBuildTask/Microsoft.VisualBasic.Core.targets#L43-L135
4e3e135 to
daabcbf
Compare
daabcbf to
ccedf57
Compare
|
@RikkiGibson, @chsienki, @jjonescz FYI |
|
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. |
|
@RikkiGibson part of my motivation for pinging is making you all aware that this exists. It was something I was unaware of.
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).
ccedf57 to
8fc0664
Compare
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. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
JanKrivanek
left a comment
There was a problem hiding this comment.
Looks good.
Though I do not feel 100% qualified to assess all the csc task adjustments. More details about whythose are made might be helpful.
The
Csctask invocation inXamlPreCompilehas drifted from the one inCoreCompilein 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&Vbccalls 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 caa45ead28a51cbd6322b79bec72b29102c04ce1, making a new commit of a clean update copy/paste of the task, and then merging with the subsequent edits.