Disable TargetingPack Analyzers - #92648

Closed
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers
Closed

Disable TargetingPack Analyzers#92648
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers

Conversation

@ericstj

Copy link
Copy Markdown
Member

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

Author:ericstj
Assignees:-
Labels:

area-Infrastructure-libraries

Milestone:-

@ericstj

Copy link
Copy Markdown
MemberAuthor

I'll need to reduce the condition down to just the latest framework.

@ericstj

Copy link
Copy Markdown
MemberAuthor

This might not actually fix the problem. @tarekgh noticed that he still sees a failure with this change. That's due to the failure happening when loading the net8.0 generator.

For some reason the net8.0 generator is binding to the live built version of Microsoft.Interop.SourceGeneration. This could be happening if we're passing the live-built generator bits to downlevel projects. I remember the interop generator does this to add support for interop generation to downlevel frameworks. @jkoritzinsky@elinor-fung

@ViktorHofer

Copy link
Copy Markdown
Member

Doesn't this change partially undo 07ae197 which was observed as general goodness?

Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

Shouldn't this infrastructure prevent that?

</ItemGroup>
</Target>

<!-- Don't use the TargetingPack Analyzers for current .NET version unless explicitly enabled -->

@ViktorHoferViktorHoferSep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I read this, it wasn't obvious to me that the target only applies to projects that don't use the local targeting / runtime pack. You want to point that out in the code comment.

For which projects should this target run? Microsoft.NETCore.Platforms doesn't use the local targeting pack but doesn't disable implicit framework references. Are there other such projects under src/libraries?

@ericstj

Copy link
Copy Markdown
MemberAuthor

Here's the reason this was failing:

/analyzer:C:\oss\runtime\artifacts\bin\Microsoft.Interop.SourceGeneration\Release\netstandard2.0\Microsoft.Interop.SourceGeneration.dll
/analyzer:C:\oss\runtime\artifacts\bin\LibraryImportGenerator\Release\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.ComInterfaceGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.JavaScript.JSImportGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

So during the runtime build, we include 8.0 analyzers from the ref pack, but the live built LibraryImportGenerator and Microsoft.Interop.SourceGeneration.dll. Roslyn will force all generators to use that live built Microsoft.Interop.SourceGeneration.dll - even the 8.0 ones. Tarek might be hitting this because he has an RC2 SDK installed, others may hit this as soon as they have RC2. RC2 may be observing this due to a dependency on the removed type - though I haven't found that just yet.

@jkoritzinsky

Copy link
Copy Markdown
Member

The area you're looking for is in generators.targets. However, I wonder if this is the correct fix. The generators that are included in the ref pack are designed for that particular TFM and they generally aren't designed to work downlevel. LibraryImportGenerator is the exception here, though that support is limited. It might be better to change generators.targets to not include the references to LibraryImportGenerator and Microsoft.Interop.SourceGeneration for downlevel netX.0 TFMs (we still need it for netstandard2.0).

@ViktorHofer

Copy link
Copy Markdown
Member

/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

I believe a better fix would be to make sure that when that shared live built source generator is used (Microsoft.Interop.SourceGeneration.dll), the dependent live built source generator are also referenced so that there isn't a mix.

@ericstj

ericstj commented Sep 26, 2023

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky can you take a crack at fixing this in the way that makes the most sense for the interop source generators?

My initial goal here was to minimize the use of those LKG generators when they'd be in torn state. Given we have very few projects targeting LKG for the latest .NET it seemed removing the generators for those would fix it - but I had missed the downlevel cases which seem to be more important.

@ericstjericstj closed this Sep 26, 2023
@jkoritzinsky

Copy link
Copy Markdown
Member

I'll take a look!

@ericstj

Copy link
Copy Markdown
MemberAuthor

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

Agreed for net8.0 - I wasn't proposing we drop source generators from older frameworks. For latest we should really opt for using the code we are building rather than some past snapshot of the SDK. Not doing so might lead us to miss bugs or run in unusual torn state situations.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ericstj@ViktorHofer@jkoritzinsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Disable TargetingPack Analyzers - #92648

Closed
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers
Closed

Disable TargetingPack Analyzers#92648
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers

Conversation

@ericstj

Copy link
Copy Markdown
Member

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

Author:ericstj
Assignees:-
Labels:

area-Infrastructure-libraries

Milestone:-

@ericstj

Copy link
Copy Markdown
MemberAuthor

I'll need to reduce the condition down to just the latest framework.

@ericstj

Copy link
Copy Markdown
MemberAuthor

This might not actually fix the problem. @tarekgh noticed that he still sees a failure with this change. That's due to the failure happening when loading the net8.0 generator.

For some reason the net8.0 generator is binding to the live built version of Microsoft.Interop.SourceGeneration. This could be happening if we're passing the live-built generator bits to downlevel projects. I remember the interop generator does this to add support for interop generation to downlevel frameworks. @jkoritzinsky@elinor-fung

@ViktorHofer

Copy link
Copy Markdown
Member

Doesn't this change partially undo 07ae197 which was observed as general goodness?

Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

Shouldn't this infrastructure prevent that?

</ItemGroup>
</Target>

<!-- Don't use the TargetingPack Analyzers for current .NET version unless explicitly enabled -->

@ViktorHoferViktorHoferSep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I read this, it wasn't obvious to me that the target only applies to projects that don't use the local targeting / runtime pack. You want to point that out in the code comment.

For which projects should this target run? Microsoft.NETCore.Platforms doesn't use the local targeting pack but doesn't disable implicit framework references. Are there other such projects under src/libraries?

@ericstj

Copy link
Copy Markdown
MemberAuthor

Here's the reason this was failing:

/analyzer:C:\oss\runtime\artifacts\bin\Microsoft.Interop.SourceGeneration\Release\netstandard2.0\Microsoft.Interop.SourceGeneration.dll
/analyzer:C:\oss\runtime\artifacts\bin\LibraryImportGenerator\Release\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.ComInterfaceGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.JavaScript.JSImportGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

So during the runtime build, we include 8.0 analyzers from the ref pack, but the live built LibraryImportGenerator and Microsoft.Interop.SourceGeneration.dll. Roslyn will force all generators to use that live built Microsoft.Interop.SourceGeneration.dll - even the 8.0 ones. Tarek might be hitting this because he has an RC2 SDK installed, others may hit this as soon as they have RC2. RC2 may be observing this due to a dependency on the removed type - though I haven't found that just yet.

@jkoritzinsky

Copy link
Copy Markdown
Member

The area you're looking for is in generators.targets. However, I wonder if this is the correct fix. The generators that are included in the ref pack are designed for that particular TFM and they generally aren't designed to work downlevel. LibraryImportGenerator is the exception here, though that support is limited. It might be better to change generators.targets to not include the references to LibraryImportGenerator and Microsoft.Interop.SourceGeneration for downlevel netX.0 TFMs (we still need it for netstandard2.0).

@ViktorHofer

Copy link
Copy Markdown
Member

/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

I believe a better fix would be to make sure that when that shared live built source generator is used (Microsoft.Interop.SourceGeneration.dll), the dependent live built source generator are also referenced so that there isn't a mix.

@ericstj

ericstj commented Sep 26, 2023

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky can you take a crack at fixing this in the way that makes the most sense for the interop source generators?

My initial goal here was to minimize the use of those LKG generators when they'd be in torn state. Given we have very few projects targeting LKG for the latest .NET it seemed removing the generators for those would fix it - but I had missed the downlevel cases which seem to be more important.

@ericstjericstj closed this Sep 26, 2023
@jkoritzinsky

Copy link
Copy Markdown
Member

I'll take a look!

@ericstj

Copy link
Copy Markdown
MemberAuthor

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

Agreed for net8.0 - I wasn't proposing we drop source generators from older frameworks. For latest we should really opt for using the code we are building rather than some past snapshot of the SDK. Not doing so might lead us to miss bugs or run in unusual torn state situations.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ericstj@ViktorHofer@jkoritzinsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Disable TargetingPack Analyzers - #92648

Closed
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers
Closed

Disable TargetingPack Analyzers#92648
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers

Conversation

@ericstj

Copy link
Copy Markdown
Member

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

Author:ericstj
Assignees:-
Labels:

area-Infrastructure-libraries

Milestone:-

@ericstj

Copy link
Copy Markdown
MemberAuthor

I'll need to reduce the condition down to just the latest framework.

@ericstj

Copy link
Copy Markdown
MemberAuthor

This might not actually fix the problem. @tarekgh noticed that he still sees a failure with this change. That's due to the failure happening when loading the net8.0 generator.

For some reason the net8.0 generator is binding to the live built version of Microsoft.Interop.SourceGeneration. This could be happening if we're passing the live-built generator bits to downlevel projects. I remember the interop generator does this to add support for interop generation to downlevel frameworks. @jkoritzinsky@elinor-fung

@ViktorHofer

Copy link
Copy Markdown
Member

Doesn't this change partially undo 07ae197 which was observed as general goodness?

Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

Shouldn't this infrastructure prevent that?

</ItemGroup>
</Target>

<!-- Don't use the TargetingPack Analyzers for current .NET version unless explicitly enabled -->

@ViktorHoferViktorHoferSep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I read this, it wasn't obvious to me that the target only applies to projects that don't use the local targeting / runtime pack. You want to point that out in the code comment.

For which projects should this target run? Microsoft.NETCore.Platforms doesn't use the local targeting pack but doesn't disable implicit framework references. Are there other such projects under src/libraries?

@ericstj

Copy link
Copy Markdown
MemberAuthor

Here's the reason this was failing:

/analyzer:C:\oss\runtime\artifacts\bin\Microsoft.Interop.SourceGeneration\Release\netstandard2.0\Microsoft.Interop.SourceGeneration.dll
/analyzer:C:\oss\runtime\artifacts\bin\LibraryImportGenerator\Release\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.ComInterfaceGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.JavaScript.JSImportGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

So during the runtime build, we include 8.0 analyzers from the ref pack, but the live built LibraryImportGenerator and Microsoft.Interop.SourceGeneration.dll. Roslyn will force all generators to use that live built Microsoft.Interop.SourceGeneration.dll - even the 8.0 ones. Tarek might be hitting this because he has an RC2 SDK installed, others may hit this as soon as they have RC2. RC2 may be observing this due to a dependency on the removed type - though I haven't found that just yet.

@jkoritzinsky

Copy link
Copy Markdown
Member

The area you're looking for is in generators.targets. However, I wonder if this is the correct fix. The generators that are included in the ref pack are designed for that particular TFM and they generally aren't designed to work downlevel. LibraryImportGenerator is the exception here, though that support is limited. It might be better to change generators.targets to not include the references to LibraryImportGenerator and Microsoft.Interop.SourceGeneration for downlevel netX.0 TFMs (we still need it for netstandard2.0).

@ViktorHofer

Copy link
Copy Markdown
Member

/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

I believe a better fix would be to make sure that when that shared live built source generator is used (Microsoft.Interop.SourceGeneration.dll), the dependent live built source generator are also referenced so that there isn't a mix.

@ericstj

ericstj commented Sep 26, 2023

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky can you take a crack at fixing this in the way that makes the most sense for the interop source generators?

My initial goal here was to minimize the use of those LKG generators when they'd be in torn state. Given we have very few projects targeting LKG for the latest .NET it seemed removing the generators for those would fix it - but I had missed the downlevel cases which seem to be more important.

@ericstjericstj closed this Sep 26, 2023
@jkoritzinsky

Copy link
Copy Markdown
Member

I'll take a look!

@ericstj

Copy link
Copy Markdown
MemberAuthor

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

Agreed for net8.0 - I wasn't proposing we drop source generators from older frameworks. For latest we should really opt for using the code we are building rather than some past snapshot of the SDK. Not doing so might lead us to miss bugs or run in unusual torn state situations.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ericstj@ViktorHofer@jkoritzinsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Disable TargetingPack Analyzers - #92648

Closed
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers
Closed

Disable TargetingPack Analyzers#92648
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers

Conversation

@ericstj

Copy link
Copy Markdown
Member

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

Author:ericstj
Assignees:-
Labels:

area-Infrastructure-libraries

Milestone:-

@ericstj

Copy link
Copy Markdown
MemberAuthor

I'll need to reduce the condition down to just the latest framework.

@ericstj

Copy link
Copy Markdown
MemberAuthor

This might not actually fix the problem. @tarekgh noticed that he still sees a failure with this change. That's due to the failure happening when loading the net8.0 generator.

For some reason the net8.0 generator is binding to the live built version of Microsoft.Interop.SourceGeneration. This could be happening if we're passing the live-built generator bits to downlevel projects. I remember the interop generator does this to add support for interop generation to downlevel frameworks. @jkoritzinsky@elinor-fung

@ViktorHofer

Copy link
Copy Markdown
Member

Doesn't this change partially undo 07ae197 which was observed as general goodness?

Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

Shouldn't this infrastructure prevent that?

</ItemGroup>
</Target>

<!-- Don't use the TargetingPack Analyzers for current .NET version unless explicitly enabled -->

@ViktorHoferViktorHoferSep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I read this, it wasn't obvious to me that the target only applies to projects that don't use the local targeting / runtime pack. You want to point that out in the code comment.

For which projects should this target run? Microsoft.NETCore.Platforms doesn't use the local targeting pack but doesn't disable implicit framework references. Are there other such projects under src/libraries?

@ericstj

Copy link
Copy Markdown
MemberAuthor

Here's the reason this was failing:

/analyzer:C:\oss\runtime\artifacts\bin\Microsoft.Interop.SourceGeneration\Release\netstandard2.0\Microsoft.Interop.SourceGeneration.dll
/analyzer:C:\oss\runtime\artifacts\bin\LibraryImportGenerator\Release\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.ComInterfaceGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.JavaScript.JSImportGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

So during the runtime build, we include 8.0 analyzers from the ref pack, but the live built LibraryImportGenerator and Microsoft.Interop.SourceGeneration.dll. Roslyn will force all generators to use that live built Microsoft.Interop.SourceGeneration.dll - even the 8.0 ones. Tarek might be hitting this because he has an RC2 SDK installed, others may hit this as soon as they have RC2. RC2 may be observing this due to a dependency on the removed type - though I haven't found that just yet.

@jkoritzinsky

Copy link
Copy Markdown
Member

The area you're looking for is in generators.targets. However, I wonder if this is the correct fix. The generators that are included in the ref pack are designed for that particular TFM and they generally aren't designed to work downlevel. LibraryImportGenerator is the exception here, though that support is limited. It might be better to change generators.targets to not include the references to LibraryImportGenerator and Microsoft.Interop.SourceGeneration for downlevel netX.0 TFMs (we still need it for netstandard2.0).

@ViktorHofer

Copy link
Copy Markdown
Member

/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

I believe a better fix would be to make sure that when that shared live built source generator is used (Microsoft.Interop.SourceGeneration.dll), the dependent live built source generator are also referenced so that there isn't a mix.

@ericstj

ericstj commented Sep 26, 2023

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky can you take a crack at fixing this in the way that makes the most sense for the interop source generators?

My initial goal here was to minimize the use of those LKG generators when they'd be in torn state. Given we have very few projects targeting LKG for the latest .NET it seemed removing the generators for those would fix it - but I had missed the downlevel cases which seem to be more important.

@ericstjericstj closed this Sep 26, 2023
@jkoritzinsky

Copy link
Copy Markdown
Member

I'll take a look!

@ericstj

Copy link
Copy Markdown
MemberAuthor

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

Agreed for net8.0 - I wasn't proposing we drop source generators from older frameworks. For latest we should really opt for using the code we are building rather than some past snapshot of the SDK. Not doing so might lead us to miss bugs or run in unusual torn state situations.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ericstj@ViktorHofer@jkoritzinsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Disable TargetingPack Analyzers - #92648

Closed
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers
Closed

Disable TargetingPack Analyzers#92648
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers

Conversation

@ericstj

Copy link
Copy Markdown
Member

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

Author:ericstj
Assignees:-
Labels:

area-Infrastructure-libraries

Milestone:-

@ericstj

Copy link
Copy Markdown
MemberAuthor

I'll need to reduce the condition down to just the latest framework.

@ericstj

Copy link
Copy Markdown
MemberAuthor

This might not actually fix the problem. @tarekgh noticed that he still sees a failure with this change. That's due to the failure happening when loading the net8.0 generator.

For some reason the net8.0 generator is binding to the live built version of Microsoft.Interop.SourceGeneration. This could be happening if we're passing the live-built generator bits to downlevel projects. I remember the interop generator does this to add support for interop generation to downlevel frameworks. @jkoritzinsky@elinor-fung

@ViktorHofer

Copy link
Copy Markdown
Member

Doesn't this change partially undo 07ae197 which was observed as general goodness?

Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

Shouldn't this infrastructure prevent that?

</ItemGroup>
</Target>

<!-- Don't use the TargetingPack Analyzers for current .NET version unless explicitly enabled -->

@ViktorHoferViktorHoferSep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I read this, it wasn't obvious to me that the target only applies to projects that don't use the local targeting / runtime pack. You want to point that out in the code comment.

For which projects should this target run? Microsoft.NETCore.Platforms doesn't use the local targeting pack but doesn't disable implicit framework references. Are there other such projects under src/libraries?

@ericstj

Copy link
Copy Markdown
MemberAuthor

Here's the reason this was failing:

/analyzer:C:\oss\runtime\artifacts\bin\Microsoft.Interop.SourceGeneration\Release\netstandard2.0\Microsoft.Interop.SourceGeneration.dll
/analyzer:C:\oss\runtime\artifacts\bin\LibraryImportGenerator\Release\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.ComInterfaceGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.JavaScript.JSImportGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

So during the runtime build, we include 8.0 analyzers from the ref pack, but the live built LibraryImportGenerator and Microsoft.Interop.SourceGeneration.dll. Roslyn will force all generators to use that live built Microsoft.Interop.SourceGeneration.dll - even the 8.0 ones. Tarek might be hitting this because he has an RC2 SDK installed, others may hit this as soon as they have RC2. RC2 may be observing this due to a dependency on the removed type - though I haven't found that just yet.

@jkoritzinsky

Copy link
Copy Markdown
Member

The area you're looking for is in generators.targets. However, I wonder if this is the correct fix. The generators that are included in the ref pack are designed for that particular TFM and they generally aren't designed to work downlevel. LibraryImportGenerator is the exception here, though that support is limited. It might be better to change generators.targets to not include the references to LibraryImportGenerator and Microsoft.Interop.SourceGeneration for downlevel netX.0 TFMs (we still need it for netstandard2.0).

@ViktorHofer

Copy link
Copy Markdown
Member

/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

I believe a better fix would be to make sure that when that shared live built source generator is used (Microsoft.Interop.SourceGeneration.dll), the dependent live built source generator are also referenced so that there isn't a mix.

@ericstj

ericstj commented Sep 26, 2023

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky can you take a crack at fixing this in the way that makes the most sense for the interop source generators?

My initial goal here was to minimize the use of those LKG generators when they'd be in torn state. Given we have very few projects targeting LKG for the latest .NET it seemed removing the generators for those would fix it - but I had missed the downlevel cases which seem to be more important.

@ericstjericstj closed this Sep 26, 2023
@jkoritzinsky

Copy link
Copy Markdown
Member

I'll take a look!

@ericstj

Copy link
Copy Markdown
MemberAuthor

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

Agreed for net8.0 - I wasn't proposing we drop source generators from older frameworks. For latest we should really opt for using the code we are building rather than some past snapshot of the SDK. Not doing so might lead us to miss bugs or run in unusual torn state situations.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ericstj@ViktorHofer@jkoritzinsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Disable TargetingPack Analyzers - #92648

Closed
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers
Closed

Disable TargetingPack Analyzers#92648
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers

Conversation

@ericstj

Copy link
Copy Markdown
Member

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

Author:ericstj
Assignees:-
Labels:

area-Infrastructure-libraries

Milestone:-

@ericstj

Copy link
Copy Markdown
MemberAuthor

I'll need to reduce the condition down to just the latest framework.

@ericstj

Copy link
Copy Markdown
MemberAuthor

This might not actually fix the problem. @tarekgh noticed that he still sees a failure with this change. That's due to the failure happening when loading the net8.0 generator.

For some reason the net8.0 generator is binding to the live built version of Microsoft.Interop.SourceGeneration. This could be happening if we're passing the live-built generator bits to downlevel projects. I remember the interop generator does this to add support for interop generation to downlevel frameworks. @jkoritzinsky@elinor-fung

@ViktorHofer

Copy link
Copy Markdown
Member

Doesn't this change partially undo 07ae197 which was observed as general goodness?

Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

Shouldn't this infrastructure prevent that?

</ItemGroup>
</Target>

<!-- Don't use the TargetingPack Analyzers for current .NET version unless explicitly enabled -->

@ViktorHoferViktorHoferSep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I read this, it wasn't obvious to me that the target only applies to projects that don't use the local targeting / runtime pack. You want to point that out in the code comment.

For which projects should this target run? Microsoft.NETCore.Platforms doesn't use the local targeting pack but doesn't disable implicit framework references. Are there other such projects under src/libraries?

@ericstj

Copy link
Copy Markdown
MemberAuthor

Here's the reason this was failing:

/analyzer:C:\oss\runtime\artifacts\bin\Microsoft.Interop.SourceGeneration\Release\netstandard2.0\Microsoft.Interop.SourceGeneration.dll
/analyzer:C:\oss\runtime\artifacts\bin\LibraryImportGenerator\Release\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.ComInterfaceGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.JavaScript.JSImportGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

So during the runtime build, we include 8.0 analyzers from the ref pack, but the live built LibraryImportGenerator and Microsoft.Interop.SourceGeneration.dll. Roslyn will force all generators to use that live built Microsoft.Interop.SourceGeneration.dll - even the 8.0 ones. Tarek might be hitting this because he has an RC2 SDK installed, others may hit this as soon as they have RC2. RC2 may be observing this due to a dependency on the removed type - though I haven't found that just yet.

@jkoritzinsky

Copy link
Copy Markdown
Member

The area you're looking for is in generators.targets. However, I wonder if this is the correct fix. The generators that are included in the ref pack are designed for that particular TFM and they generally aren't designed to work downlevel. LibraryImportGenerator is the exception here, though that support is limited. It might be better to change generators.targets to not include the references to LibraryImportGenerator and Microsoft.Interop.SourceGeneration for downlevel netX.0 TFMs (we still need it for netstandard2.0).

@ViktorHofer

Copy link
Copy Markdown
Member

/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

I believe a better fix would be to make sure that when that shared live built source generator is used (Microsoft.Interop.SourceGeneration.dll), the dependent live built source generator are also referenced so that there isn't a mix.

@ericstj

ericstj commented Sep 26, 2023

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky can you take a crack at fixing this in the way that makes the most sense for the interop source generators?

My initial goal here was to minimize the use of those LKG generators when they'd be in torn state. Given we have very few projects targeting LKG for the latest .NET it seemed removing the generators for those would fix it - but I had missed the downlevel cases which seem to be more important.

@ericstjericstj closed this Sep 26, 2023
@jkoritzinsky

Copy link
Copy Markdown
Member

I'll take a look!

@ericstj

Copy link
Copy Markdown
MemberAuthor

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

Agreed for net8.0 - I wasn't proposing we drop source generators from older frameworks. For latest we should really opt for using the code we are building rather than some past snapshot of the SDK. Not doing so might lead us to miss bugs or run in unusual torn state situations.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ericstj@ViktorHofer@jkoritzinsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Disable TargetingPack Analyzers - #92648

Closed
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers
Closed

Disable TargetingPack Analyzers#92648
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers

Conversation

@ericstj

Copy link
Copy Markdown
Member

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

Author:ericstj
Assignees:-
Labels:

area-Infrastructure-libraries

Milestone:-

@ericstj

Copy link
Copy Markdown
MemberAuthor

I'll need to reduce the condition down to just the latest framework.

@ericstj

Copy link
Copy Markdown
MemberAuthor

This might not actually fix the problem. @tarekgh noticed that he still sees a failure with this change. That's due to the failure happening when loading the net8.0 generator.

For some reason the net8.0 generator is binding to the live built version of Microsoft.Interop.SourceGeneration. This could be happening if we're passing the live-built generator bits to downlevel projects. I remember the interop generator does this to add support for interop generation to downlevel frameworks. @jkoritzinsky@elinor-fung

@ViktorHofer

Copy link
Copy Markdown
Member

Doesn't this change partially undo 07ae197 which was observed as general goodness?

Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

Shouldn't this infrastructure prevent that?

</ItemGroup>
</Target>

<!-- Don't use the TargetingPack Analyzers for current .NET version unless explicitly enabled -->

@ViktorHoferViktorHoferSep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I read this, it wasn't obvious to me that the target only applies to projects that don't use the local targeting / runtime pack. You want to point that out in the code comment.

For which projects should this target run? Microsoft.NETCore.Platforms doesn't use the local targeting pack but doesn't disable implicit framework references. Are there other such projects under src/libraries?

@ericstj

Copy link
Copy Markdown
MemberAuthor

Here's the reason this was failing:

/analyzer:C:\oss\runtime\artifacts\bin\Microsoft.Interop.SourceGeneration\Release\netstandard2.0\Microsoft.Interop.SourceGeneration.dll
/analyzer:C:\oss\runtime\artifacts\bin\LibraryImportGenerator\Release\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.ComInterfaceGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.JavaScript.JSImportGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

So during the runtime build, we include 8.0 analyzers from the ref pack, but the live built LibraryImportGenerator and Microsoft.Interop.SourceGeneration.dll. Roslyn will force all generators to use that live built Microsoft.Interop.SourceGeneration.dll - even the 8.0 ones. Tarek might be hitting this because he has an RC2 SDK installed, others may hit this as soon as they have RC2. RC2 may be observing this due to a dependency on the removed type - though I haven't found that just yet.

@jkoritzinsky

Copy link
Copy Markdown
Member

The area you're looking for is in generators.targets. However, I wonder if this is the correct fix. The generators that are included in the ref pack are designed for that particular TFM and they generally aren't designed to work downlevel. LibraryImportGenerator is the exception here, though that support is limited. It might be better to change generators.targets to not include the references to LibraryImportGenerator and Microsoft.Interop.SourceGeneration for downlevel netX.0 TFMs (we still need it for netstandard2.0).

@ViktorHofer

Copy link
Copy Markdown
Member

/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

I believe a better fix would be to make sure that when that shared live built source generator is used (Microsoft.Interop.SourceGeneration.dll), the dependent live built source generator are also referenced so that there isn't a mix.

@ericstj

ericstj commented Sep 26, 2023

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky can you take a crack at fixing this in the way that makes the most sense for the interop source generators?

My initial goal here was to minimize the use of those LKG generators when they'd be in torn state. Given we have very few projects targeting LKG for the latest .NET it seemed removing the generators for those would fix it - but I had missed the downlevel cases which seem to be more important.

@ericstjericstj closed this Sep 26, 2023
@jkoritzinsky

Copy link
Copy Markdown
Member

I'll take a look!

@ericstj

Copy link
Copy Markdown
MemberAuthor

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

Agreed for net8.0 - I wasn't proposing we drop source generators from older frameworks. For latest we should really opt for using the code we are building rather than some past snapshot of the SDK. Not doing so might lead us to miss bugs or run in unusual torn state situations.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ericstj@ViktorHofer@jkoritzinsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Disable TargetingPack Analyzers - #92648

Closed
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers
Closed

Disable TargetingPack Analyzers#92648
ericstj wants to merge 2 commits into
dotnet:mainfrom
ericstj:disableTargetingPackAnalyzers

Conversation

@ericstj

Copy link
Copy Markdown
Member

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix issue introduced in #92301

This makes it the default to remove all analyzers that come from the TargetingPack unless specified.

This is important so that we don't mix the LKG binaries in the same process that will later load the live-built binaries. Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

I considered making this opt-in instead of opt-out, but I don't think we want anything in the repo loading the LKG binaries in the compiler process.

Author:ericstj
Assignees:-
Labels:

area-Infrastructure-libraries

Milestone:-

@ericstj

Copy link
Copy Markdown
MemberAuthor

I'll need to reduce the condition down to just the latest framework.

@ericstj

Copy link
Copy Markdown
MemberAuthor

This might not actually fix the problem. @tarekgh noticed that he still sees a failure with this change. That's due to the failure happening when loading the net8.0 generator.

For some reason the net8.0 generator is binding to the live built version of Microsoft.Interop.SourceGeneration. This could be happening if we're passing the live-built generator bits to downlevel projects. I remember the interop generator does this to add support for interop generation to downlevel frameworks. @jkoritzinsky@elinor-fung

@ViktorHofer

Copy link
Copy Markdown
Member

Doesn't this change partially undo 07ae197 which was observed as general goodness?

Doing so can result in the live built binaries being ignored since they have the same assembly name as those already loaded in process.

Shouldn't this infrastructure prevent that?

</ItemGroup>
</Target>

<!-- Don't use the TargetingPack Analyzers for current .NET version unless explicitly enabled -->

@ViktorHoferViktorHoferSep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I read this, it wasn't obvious to me that the target only applies to projects that don't use the local targeting / runtime pack. You want to point that out in the code comment.

For which projects should this target run? Microsoft.NETCore.Platforms doesn't use the local targeting pack but doesn't disable implicit framework references. Are there other such projects under src/libraries?

@ericstj

Copy link
Copy Markdown
MemberAuthor

Here's the reason this was failing:

/analyzer:C:\oss\runtime\artifacts\bin\Microsoft.Interop.SourceGeneration\Release\netstandard2.0\Microsoft.Interop.SourceGeneration.dll
/analyzer:C:\oss\runtime\artifacts\bin\LibraryImportGenerator\Release\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.ComInterfaceGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/Microsoft.Interop.JavaScript.JSImportGenerator.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

So during the runtime build, we include 8.0 analyzers from the ref pack, but the live built LibraryImportGenerator and Microsoft.Interop.SourceGeneration.dll. Roslyn will force all generators to use that live built Microsoft.Interop.SourceGeneration.dll - even the 8.0 ones. Tarek might be hitting this because he has an RC2 SDK installed, others may hit this as soon as they have RC2. RC2 may be observing this due to a dependency on the removed type - though I haven't found that just yet.

@jkoritzinsky

Copy link
Copy Markdown
Member

The area you're looking for is in generators.targets. However, I wonder if this is the correct fix. The generators that are included in the ref pack are designed for that particular TFM and they generally aren't designed to work downlevel. LibraryImportGenerator is the exception here, though that support is limited. It might be better to change generators.targets to not include the references to LibraryImportGenerator and Microsoft.Interop.SourceGeneration for downlevel netX.0 TFMs (we still need it for netstandard2.0).

@ViktorHofer

Copy link
Copy Markdown
Member

/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.Json.SourceGeneration.dll"
/analyzer:"C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.0-rc.2.23469.9\analyzers/dotnet/cs/System.Text.RegularExpressions.Generator.dll"

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

I believe a better fix would be to make sure that when that shared live built source generator is used (Microsoft.Interop.SourceGeneration.dll), the dependent live built source generator are also referenced so that there isn't a mix.

@ericstj

ericstj commented Sep 26, 2023

Copy link
Copy Markdown
MemberAuthor

@jkoritzinsky can you take a crack at fixing this in the way that makes the most sense for the interop source generators?

My initial goal here was to minimize the use of those LKG generators when they'd be in torn state. Given we have very few projects targeting LKG for the latest .NET it seemed removing the generators for those would fix it - but I had missed the downlevel cases which seem to be more important.

@ericstjericstj closed this Sep 26, 2023
@jkoritzinsky

Copy link
Copy Markdown
Member

I'll take a look!

@ericstj

Copy link
Copy Markdown
MemberAuthor

These two don't depend on that shared source generator and should still be consumed from the targeting pack.

Agreed for net8.0 - I wasn't proposing we drop source generators from older frameworks. For latest we should really opt for using the code we are building rather than some past snapshot of the SDK. Not doing so might lead us to miss bugs or run in unusual torn state situations.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ericstj@ViktorHofer@jkoritzinsky