Remove dependencies on non-live illink - #91468

Merged
sbomer merged 34 commits into
dotnet:mainfrom
sbomer:liveILLink
Oct 5, 2023
Merged

Remove dependencies on non-live illink#91468
sbomer merged 34 commits into
dotnet:mainfrom
sbomer:liveILLink

Conversation

@sbomer

@sbomersbomer commented Sep 1, 2023

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 1, 2023
@ghostghost assigned sbomerSep 1, 2023
jkoritzinsky
jkoritzinsky reviewed Sep 1, 2023
View reviewed changes

@jkoritzinskyjkoritzinsky left a comment

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.

Can you take a look in targetingpacks.targets and update the KnownILLink item changes to point to the live ILLink?

This was referenced Sep 1, 2023
Closed
Closed
Closed
Open
@sbomer

sbomer commented Sep 1, 2023

Copy link
Copy Markdown
MemberAuthor

I think we might be able to remove KnownILLinkPack entirely because we shouldn't be using the illink referenced by the SDK anywhere. Going to give this a try.

@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
cb9f7b5
@sbomer
Don't require illink analyzers for source generator projects
a64cc2a
Microsoft.Extensions.Options.SourceGeneration was enabling
the single-file analyzer, but did not import illink.targets
because it is not a source project.
We could fix this by importing liveILLink.targets instead, but
this introduces more problems with circular dependencies
(generator projects depending on illink depending on
generator projects).
ViktorHofer
ViktorHofer reviewed Sep 2, 2023
View reviewed changes

@ViktorHoferViktorHofer left a comment •
edited
Loading

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.

First of all: Amazing change!!

Slightly unrelated to this PR but still important from my point of view: Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?

ViktorHofer
ViktorHofer reviewed Sep 2, 2023
View reviewed changes
Comment threadeng/liveILLink.targets Outdated
ViktorHofer
ViktorHofer reviewed Sep 2, 2023
View reviewed changes
Comment threadeng/Analyzers.targets Outdated
@sbomer
Only enable single-file analyzer for source projects
abc514f
This more closely matches the condition on the import of
illink.targets. When EnableSingleFileAnalyzer is true, we should
also import liveILLink.targets so that we get the live analyzer
bits.
Also address feedback about path separators.
@sbomer
Separate imports
597a012
This will make it easier to use different conditions for the two.
@sbomer
Fix libraries build
e83a2a0
Some libraries test projects set PublishTrimmed, so they require
the liveILLink.targets.
@sbomer

sbomer commented Sep 5, 2023

Copy link
Copy Markdown
MemberAuthor

Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?

I'm looking for a clean way to do this, but I'm not sure of the best way yet. _RequiresILLinkPack is effectively the SDK's version of this:
https://github.com/dotnet/sdk/blob/1c9e30a466d2c1f3b4673e3b0adfb02590dc8b74/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L37-L43

liveILLink.targets sets this to false because we don't want to reference the SDK's version of the package. Maybe we should use a similar condition for the liveILLink.targets.

@build-analysisbuild-analysisBot mentioned this pull request Sep 5, 2023
Closed
@build-analysisbuild-analysisBot mentioned this pull request Sep 5, 2023
Closed
@sbomer
Import liveILLink.targets in libraries projects
d99b2d6
This should let libraries projects (whether they are source or test
projects) use the SDK's trimming properties with a live ILLink, enabling
the ProjectReference only for those projects that need it.
Because illink.targets also imports liveILLink.targets, this import is
guarded by a property to ensure liveILLink.targets is only imported
once on this path.
@sbomer
Fix import path
e0279d2
ViktorHofer
ViktorHofer reviewed Oct 3, 2023
View reviewed changes
Comment threadsrc/coreclr/Directory.Build.targets Outdated
ViktorHofer
ViktorHofer reviewed Oct 3, 2023
View reviewed changes
Comment threadsrc/libraries/Directory.Build.targets
ViktorHofer
ViktorHofer reviewed Oct 3, 2023
View reviewed changes
Comment threadeng/liveILLink.targets Outdated
sbomerand others added 2 commits October 3, 2023 13:44
@sbomer@ViktorHofer
Apply suggestions from code review
b442dbc
- Fix indentation
- Only import liveILLink.targets in libraries when illink.targets is imported
- Respect MSBuildRuntimeType for live ILLinkTasksAssembly override
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
d18a62d
jkoritzinsky
jkoritzinsky reviewed Oct 3, 2023
View reviewed changes
Comment threadeng/targetingpacks.targets
@sbomer
Revert "Fix libs test project that sets PublishTrimmed"
2c16391
This reverts commit be50c35.
@sbomer
Revert condition on liveILLink.targets import
96a663d
Various test configurations (wasm, nativeaot) rely on the live ILLink
when running libraries tests.
@ViktorHofer

ViktorHofer commented Oct 5, 2023

Copy link
Copy Markdown
Member

Looks like there is a remaining failure in the NativeAOT leg .

@sbomersbomer mentioned this pull request Oct 5, 2023
Merged
1 task
@sbomer
Skip nonexistent targets when copying native binaries
9b9ed8d
This target isn't defined in the ILLink.Tasks.csproj ProjectReference.
Try skipping nonexistent targets to fix this.
This was referenced Oct 5, 2023
Open
Closed
Closed
Closed
@sbomer
sbomer merged commit 1ef38a8 into dotnet:mainOct 5, 2023
@sbomer

sbomer commented Oct 5, 2023

Copy link
Copy Markdown
MemberAuthor

Failures are known (it seems browser-wasm job timed out but didn't report status back to github). I couldn't have got this through without your help @ViktorHofer and @jkoritzinsky - thank you!

@build-analysisbuild-analysisBot mentioned this pull request Oct 6, 2023
Closed
3 tasks
This was referenced Oct 26, 2023
Closed
Merged
@sbomer
sbomer deleted the liveILLink branch November 3, 2023 18:38
@ghostghost locked as resolved and limited conversation to collaborators Dec 3, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@ViktorHoferViktorHoferViktorHofer approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
@lewinglewingAwaiting requested review from lewing
@pavelsavarapavelsavaraAwaiting requested review from pavelsavara

Assignees

@sbomersbomer

Labels

needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sbomer@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
Remove dependencies on non-live illink - #91468
Merged
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Oct 5, 2023
Merged
Remove dependencies on non-live illink#91468
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Conversation
@sbomer
@sbomersbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

Member
 No description provided. 
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Sep 1, 2023
@ghostghost
assigned sbomerSep 1, 2023
jkoritzinsky
jkoritzinsky
reviewed
Sep 1, 2023
View reviewed changes
@jkoritzinskyjkoritzinsky
left a comment


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.

Can you take a look in targetingpacks.targets and update the KnownILLink item changes to point to the live ILLink?

This was referenced Sep 1, 2023

Closed


Closed


Closed


Open

@sbomer
sbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

MemberAuthor
I think we might be able to remove KnownILLinkPack entirely because we shouldn't be using the illink referenced by the SDK anywhere. Going to give this a try.
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
cb9f7b5
@sbomer
Don't require illink analyzers for source generator projects
a64cc2a
Microsoft.Extensions.Options.SourceGeneration was enabling
the single-file analyzer, but did not import illink.targets
because it is not a source project.
We could fix this by importing liveILLink.targets instead, but
this introduces more problems with circular dependencies
(generator projects depending on illink depending on
generator projects).
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
@ViktorHoferViktorHofer
left a comment
•

edited

Loading

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.

First of all: Amazing change!!
Slightly unrelated to this PR but still important from my point of view: Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/Analyzers.targets
Outdated

@sbomer
Only enable single-file analyzer for source projects
abc514f
This more closely matches the condition on the import of
illink.targets. When EnableSingleFileAnalyzer is true, we should
also import liveILLink.targets so that we get the live analyzer
bits.
Also address feedback about path separators.
@sbomer
Separate imports
597a012
This will make it easier to use different conditions for the two.
@sbomer
Fix libraries build
e83a2a0
Some libraries test projects set PublishTrimmed, so they require
the liveILLink.targets.
@sbomer
sbomer
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
I'm looking for a clean way to do this, but I'm not sure of the best way yet. _RequiresILLinkPack is effectively the SDK's version of this:
https://github.com/dotnet/sdk/blob/1c9e30a466d2c1f3b4673e3b0adfb02590dc8b74/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L37-L43
liveILLink.targets sets this to false because we don't want to reference the SDK's version of the package. Maybe we should use a similar condition for the liveILLink.targets.
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@sbomer
Import liveILLink.targets in libraries projects
d99b2d6
This should let libraries projects (whether they are source or test
projects) use the SDK's trimming properties with a live ILLink, enabling
the ProjectReference only for those projects that need it.
Because illink.targets also imports liveILLink.targets, this import is
guarded by a property to ensure liveILLink.targets is only imported
once on this path.
@sbomer
Fix import path
e0279d2
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/coreclr/Directory.Build.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/libraries/Directory.Build.targets
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

sbomerand others
added 2 commits
October 3, 2023 13:44
@sbomer@ViktorHofer
Apply suggestions from code review
b442dbc
- Fix indentation
- Only import liveILLink.targets in libraries when illink.targets is imported
- Respect MSBuildRuntimeType for live ILLinkTasksAssembly override
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
d18a62d
jkoritzinsky
jkoritzinsky
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/targetingpacks.targets
@sbomer
Revert "Fix libs test project that sets PublishTrimmed"
2c16391
This reverts commit be50c35.
@sbomer
Revert condition on liveILLink.targets import
96a663d
Various test configurations (wasm, nativeaot) rely on the live ILLink
when running libraries tests.
@ViktorHofer
ViktorHofer
commented
Oct 5, 2023

Copy link


Copy Markdown

Member
Looks like there is a remaining failure in the NativeAOT leg .
@sbomersbomer
mentioned this pull request
Oct 5, 2023
 Merged

1 task
@sbomer
Skip nonexistent targets when copying native binaries
9b9ed8d
This target isn't defined in the ILLink.Tasks.csproj ProjectReference.
Try skipping nonexistent targets to fix this.

This was referenced Oct 5, 2023

Open


Closed


Closed


Closed

@sbomer
sbomer
merged commit 1ef38a8
into
dotnet:mainOct 5, 2023
@sbomer
sbomer
commented
Oct 5, 2023

Copy link


Copy Markdown

MemberAuthor
Failures are known (it seems browser-wasm job timed out but didn't report status back to github). I couldn't have got this through without your help @ViktorHofer and @jkoritzinsky - thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 6, 2023

Closed

3 tasks

This was referenced Oct 26, 2023

Closed

 Merged

@sbomer
sbomer
deleted the

liveILLink

branch
November 3, 2023 18:38
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@ViktorHoferViktorHoferViktorHofer approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
@lewinglewingAwaiting requested review from lewing
@pavelsavarapavelsavaraAwaiting requested review from pavelsavara

Assignees

@sbomersbomer

Labels

needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sbomer@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
Remove dependencies on non-live illink - #91468
Merged
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Oct 5, 2023
Merged
Remove dependencies on non-live illink#91468
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Conversation
@sbomer
@sbomersbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

Member
 No description provided. 
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Sep 1, 2023
@ghostghost
assigned sbomerSep 1, 2023
jkoritzinsky
jkoritzinsky
reviewed
Sep 1, 2023
View reviewed changes
@jkoritzinskyjkoritzinsky
left a comment


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.

Can you take a look in targetingpacks.targets and update the KnownILLink item changes to point to the live ILLink?

This was referenced Sep 1, 2023

Closed


Closed


Closed


Open

@sbomer
sbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

MemberAuthor
I think we might be able to remove KnownILLinkPack entirely because we shouldn't be using the illink referenced by the SDK anywhere. Going to give this a try.
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
cb9f7b5
@sbomer
Don't require illink analyzers for source generator projects
a64cc2a
Microsoft.Extensions.Options.SourceGeneration was enabling
the single-file analyzer, but did not import illink.targets
because it is not a source project.
We could fix this by importing liveILLink.targets instead, but
this introduces more problems with circular dependencies
(generator projects depending on illink depending on
generator projects).
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
@ViktorHoferViktorHofer
left a comment
•

edited

Loading

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.

First of all: Amazing change!!
Slightly unrelated to this PR but still important from my point of view: Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/Analyzers.targets
Outdated

@sbomer
Only enable single-file analyzer for source projects
abc514f
This more closely matches the condition on the import of
illink.targets. When EnableSingleFileAnalyzer is true, we should
also import liveILLink.targets so that we get the live analyzer
bits.
Also address feedback about path separators.
@sbomer
Separate imports
597a012
This will make it easier to use different conditions for the two.
@sbomer
Fix libraries build
e83a2a0
Some libraries test projects set PublishTrimmed, so they require
the liveILLink.targets.
@sbomer
sbomer
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
I'm looking for a clean way to do this, but I'm not sure of the best way yet. _RequiresILLinkPack is effectively the SDK's version of this:
https://github.com/dotnet/sdk/blob/1c9e30a466d2c1f3b4673e3b0adfb02590dc8b74/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L37-L43
liveILLink.targets sets this to false because we don't want to reference the SDK's version of the package. Maybe we should use a similar condition for the liveILLink.targets.
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@sbomer
Import liveILLink.targets in libraries projects
d99b2d6
This should let libraries projects (whether they are source or test
projects) use the SDK's trimming properties with a live ILLink, enabling
the ProjectReference only for those projects that need it.
Because illink.targets also imports liveILLink.targets, this import is
guarded by a property to ensure liveILLink.targets is only imported
once on this path.
@sbomer
Fix import path
e0279d2
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/coreclr/Directory.Build.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/libraries/Directory.Build.targets
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

sbomerand others
added 2 commits
October 3, 2023 13:44
@sbomer@ViktorHofer
Apply suggestions from code review
b442dbc
- Fix indentation
- Only import liveILLink.targets in libraries when illink.targets is imported
- Respect MSBuildRuntimeType for live ILLinkTasksAssembly override
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
d18a62d
jkoritzinsky
jkoritzinsky
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/targetingpacks.targets
@sbomer
Revert "Fix libs test project that sets PublishTrimmed"
2c16391
This reverts commit be50c35.
@sbomer
Revert condition on liveILLink.targets import
96a663d
Various test configurations (wasm, nativeaot) rely on the live ILLink
when running libraries tests.
@ViktorHofer
ViktorHofer
commented
Oct 5, 2023

Copy link


Copy Markdown

Member
Looks like there is a remaining failure in the NativeAOT leg .
@sbomersbomer
mentioned this pull request
Oct 5, 2023
 Merged

1 task
@sbomer
Skip nonexistent targets when copying native binaries
9b9ed8d
This target isn't defined in the ILLink.Tasks.csproj ProjectReference.
Try skipping nonexistent targets to fix this.

This was referenced Oct 5, 2023

Open


Closed


Closed


Closed

@sbomer
sbomer
merged commit 1ef38a8
into
dotnet:mainOct 5, 2023
@sbomer
sbomer
commented
Oct 5, 2023

Copy link


Copy Markdown

MemberAuthor
Failures are known (it seems browser-wasm job timed out but didn't report status back to github). I couldn't have got this through without your help @ViktorHofer and @jkoritzinsky - thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 6, 2023

Closed

3 tasks

This was referenced Oct 26, 2023

Closed

 Merged

@sbomer
sbomer
deleted the

liveILLink

branch
November 3, 2023 18:38
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@ViktorHoferViktorHoferViktorHofer approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
@lewinglewingAwaiting requested review from lewing
@pavelsavarapavelsavaraAwaiting requested review from pavelsavara

Assignees

@sbomersbomer

Labels

needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sbomer@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
Remove dependencies on non-live illink - #91468
Merged
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Oct 5, 2023
Merged
Remove dependencies on non-live illink#91468
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Conversation
@sbomer
@sbomersbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

Member
 No description provided. 
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Sep 1, 2023
@ghostghost
assigned sbomerSep 1, 2023
jkoritzinsky
jkoritzinsky
reviewed
Sep 1, 2023
View reviewed changes
@jkoritzinskyjkoritzinsky
left a comment


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.

Can you take a look in targetingpacks.targets and update the KnownILLink item changes to point to the live ILLink?

This was referenced Sep 1, 2023

Closed


Closed


Closed


Open

@sbomer
sbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

MemberAuthor
I think we might be able to remove KnownILLinkPack entirely because we shouldn't be using the illink referenced by the SDK anywhere. Going to give this a try.
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
cb9f7b5
@sbomer
Don't require illink analyzers for source generator projects
a64cc2a
Microsoft.Extensions.Options.SourceGeneration was enabling
the single-file analyzer, but did not import illink.targets
because it is not a source project.
We could fix this by importing liveILLink.targets instead, but
this introduces more problems with circular dependencies
(generator projects depending on illink depending on
generator projects).
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
@ViktorHoferViktorHofer
left a comment
•

edited

Loading

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.

First of all: Amazing change!!
Slightly unrelated to this PR but still important from my point of view: Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/Analyzers.targets
Outdated

@sbomer
Only enable single-file analyzer for source projects
abc514f
This more closely matches the condition on the import of
illink.targets. When EnableSingleFileAnalyzer is true, we should
also import liveILLink.targets so that we get the live analyzer
bits.
Also address feedback about path separators.
@sbomer
Separate imports
597a012
This will make it easier to use different conditions for the two.
@sbomer
Fix libraries build
e83a2a0
Some libraries test projects set PublishTrimmed, so they require
the liveILLink.targets.
@sbomer
sbomer
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
I'm looking for a clean way to do this, but I'm not sure of the best way yet. _RequiresILLinkPack is effectively the SDK's version of this:
https://github.com/dotnet/sdk/blob/1c9e30a466d2c1f3b4673e3b0adfb02590dc8b74/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L37-L43
liveILLink.targets sets this to false because we don't want to reference the SDK's version of the package. Maybe we should use a similar condition for the liveILLink.targets.
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@sbomer
Import liveILLink.targets in libraries projects
d99b2d6
This should let libraries projects (whether they are source or test
projects) use the SDK's trimming properties with a live ILLink, enabling
the ProjectReference only for those projects that need it.
Because illink.targets also imports liveILLink.targets, this import is
guarded by a property to ensure liveILLink.targets is only imported
once on this path.
@sbomer
Fix import path
e0279d2
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/coreclr/Directory.Build.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/libraries/Directory.Build.targets
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

sbomerand others
added 2 commits
October 3, 2023 13:44
@sbomer@ViktorHofer
Apply suggestions from code review
b442dbc
- Fix indentation
- Only import liveILLink.targets in libraries when illink.targets is imported
- Respect MSBuildRuntimeType for live ILLinkTasksAssembly override
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
d18a62d
jkoritzinsky
jkoritzinsky
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/targetingpacks.targets
@sbomer
Revert "Fix libs test project that sets PublishTrimmed"
2c16391
This reverts commit be50c35.
@sbomer
Revert condition on liveILLink.targets import
96a663d
Various test configurations (wasm, nativeaot) rely on the live ILLink
when running libraries tests.
@ViktorHofer
ViktorHofer
commented
Oct 5, 2023

Copy link


Copy Markdown

Member
Looks like there is a remaining failure in the NativeAOT leg .
@sbomersbomer
mentioned this pull request
Oct 5, 2023
 Merged

1 task
@sbomer
Skip nonexistent targets when copying native binaries
9b9ed8d
This target isn't defined in the ILLink.Tasks.csproj ProjectReference.
Try skipping nonexistent targets to fix this.

This was referenced Oct 5, 2023

Open


Closed


Closed


Closed

@sbomer
sbomer
merged commit 1ef38a8
into
dotnet:mainOct 5, 2023
@sbomer
sbomer
commented
Oct 5, 2023

Copy link


Copy Markdown

MemberAuthor
Failures are known (it seems browser-wasm job timed out but didn't report status back to github). I couldn't have got this through without your help @ViktorHofer and @jkoritzinsky - thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 6, 2023

Closed

3 tasks

This was referenced Oct 26, 2023

Closed

 Merged

@sbomer
sbomer
deleted the

liveILLink

branch
November 3, 2023 18:38
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@ViktorHoferViktorHoferViktorHofer approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
@lewinglewingAwaiting requested review from lewing
@pavelsavarapavelsavaraAwaiting requested review from pavelsavara

Assignees

@sbomersbomer

Labels

needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sbomer@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
Remove dependencies on non-live illink - #91468
Merged
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Oct 5, 2023
Merged
Remove dependencies on non-live illink#91468
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Conversation
@sbomer
@sbomersbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

Member
 No description provided. 
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Sep 1, 2023
@ghostghost
assigned sbomerSep 1, 2023
jkoritzinsky
jkoritzinsky
reviewed
Sep 1, 2023
View reviewed changes
@jkoritzinskyjkoritzinsky
left a comment


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.

Can you take a look in targetingpacks.targets and update the KnownILLink item changes to point to the live ILLink?

This was referenced Sep 1, 2023

Closed


Closed


Closed


Open

@sbomer
sbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

MemberAuthor
I think we might be able to remove KnownILLinkPack entirely because we shouldn't be using the illink referenced by the SDK anywhere. Going to give this a try.
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
cb9f7b5
@sbomer
Don't require illink analyzers for source generator projects
a64cc2a
Microsoft.Extensions.Options.SourceGeneration was enabling
the single-file analyzer, but did not import illink.targets
because it is not a source project.
We could fix this by importing liveILLink.targets instead, but
this introduces more problems with circular dependencies
(generator projects depending on illink depending on
generator projects).
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
@ViktorHoferViktorHofer
left a comment
•

edited

Loading

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.

First of all: Amazing change!!
Slightly unrelated to this PR but still important from my point of view: Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/Analyzers.targets
Outdated

@sbomer
Only enable single-file analyzer for source projects
abc514f
This more closely matches the condition on the import of
illink.targets. When EnableSingleFileAnalyzer is true, we should
also import liveILLink.targets so that we get the live analyzer
bits.
Also address feedback about path separators.
@sbomer
Separate imports
597a012
This will make it easier to use different conditions for the two.
@sbomer
Fix libraries build
e83a2a0
Some libraries test projects set PublishTrimmed, so they require
the liveILLink.targets.
@sbomer
sbomer
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
I'm looking for a clean way to do this, but I'm not sure of the best way yet. _RequiresILLinkPack is effectively the SDK's version of this:
https://github.com/dotnet/sdk/blob/1c9e30a466d2c1f3b4673e3b0adfb02590dc8b74/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L37-L43
liveILLink.targets sets this to false because we don't want to reference the SDK's version of the package. Maybe we should use a similar condition for the liveILLink.targets.
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@sbomer
Import liveILLink.targets in libraries projects
d99b2d6
This should let libraries projects (whether they are source or test
projects) use the SDK's trimming properties with a live ILLink, enabling
the ProjectReference only for those projects that need it.
Because illink.targets also imports liveILLink.targets, this import is
guarded by a property to ensure liveILLink.targets is only imported
once on this path.
@sbomer
Fix import path
e0279d2
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/coreclr/Directory.Build.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/libraries/Directory.Build.targets
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

sbomerand others
added 2 commits
October 3, 2023 13:44
@sbomer@ViktorHofer
Apply suggestions from code review
b442dbc
- Fix indentation
- Only import liveILLink.targets in libraries when illink.targets is imported
- Respect MSBuildRuntimeType for live ILLinkTasksAssembly override
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
d18a62d
jkoritzinsky
jkoritzinsky
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/targetingpacks.targets
@sbomer
Revert "Fix libs test project that sets PublishTrimmed"
2c16391
This reverts commit be50c35.
@sbomer
Revert condition on liveILLink.targets import
96a663d
Various test configurations (wasm, nativeaot) rely on the live ILLink
when running libraries tests.
@ViktorHofer
ViktorHofer
commented
Oct 5, 2023

Copy link


Copy Markdown

Member
Looks like there is a remaining failure in the NativeAOT leg .
@sbomersbomer
mentioned this pull request
Oct 5, 2023
 Merged

1 task
@sbomer
Skip nonexistent targets when copying native binaries
9b9ed8d
This target isn't defined in the ILLink.Tasks.csproj ProjectReference.
Try skipping nonexistent targets to fix this.

This was referenced Oct 5, 2023

Open


Closed


Closed


Closed

@sbomer
sbomer
merged commit 1ef38a8
into
dotnet:mainOct 5, 2023
@sbomer
sbomer
commented
Oct 5, 2023

Copy link


Copy Markdown

MemberAuthor
Failures are known (it seems browser-wasm job timed out but didn't report status back to github). I couldn't have got this through without your help @ViktorHofer and @jkoritzinsky - thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 6, 2023

Closed

3 tasks

This was referenced Oct 26, 2023

Closed

 Merged

@sbomer
sbomer
deleted the

liveILLink

branch
November 3, 2023 18:38
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@ViktorHoferViktorHoferViktorHofer approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
@lewinglewingAwaiting requested review from lewing
@pavelsavarapavelsavaraAwaiting requested review from pavelsavara

Assignees

@sbomersbomer

Labels

needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sbomer@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
Remove dependencies on non-live illink - #91468
Merged
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Oct 5, 2023
Merged
Remove dependencies on non-live illink#91468
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Conversation
@sbomer
@sbomersbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

Member
 No description provided. 
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Sep 1, 2023
@ghostghost
assigned sbomerSep 1, 2023
jkoritzinsky
jkoritzinsky
reviewed
Sep 1, 2023
View reviewed changes
@jkoritzinskyjkoritzinsky
left a comment


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.

Can you take a look in targetingpacks.targets and update the KnownILLink item changes to point to the live ILLink?

This was referenced Sep 1, 2023

Closed


Closed


Closed


Open

@sbomer
sbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

MemberAuthor
I think we might be able to remove KnownILLinkPack entirely because we shouldn't be using the illink referenced by the SDK anywhere. Going to give this a try.
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
cb9f7b5
@sbomer
Don't require illink analyzers for source generator projects
a64cc2a
Microsoft.Extensions.Options.SourceGeneration was enabling
the single-file analyzer, but did not import illink.targets
because it is not a source project.
We could fix this by importing liveILLink.targets instead, but
this introduces more problems with circular dependencies
(generator projects depending on illink depending on
generator projects).
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
@ViktorHoferViktorHofer
left a comment
•

edited

Loading

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.

First of all: Amazing change!!
Slightly unrelated to this PR but still important from my point of view: Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/Analyzers.targets
Outdated

@sbomer
Only enable single-file analyzer for source projects
abc514f
This more closely matches the condition on the import of
illink.targets. When EnableSingleFileAnalyzer is true, we should
also import liveILLink.targets so that we get the live analyzer
bits.
Also address feedback about path separators.
@sbomer
Separate imports
597a012
This will make it easier to use different conditions for the two.
@sbomer
Fix libraries build
e83a2a0
Some libraries test projects set PublishTrimmed, so they require
the liveILLink.targets.
@sbomer
sbomer
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
I'm looking for a clean way to do this, but I'm not sure of the best way yet. _RequiresILLinkPack is effectively the SDK's version of this:
https://github.com/dotnet/sdk/blob/1c9e30a466d2c1f3b4673e3b0adfb02590dc8b74/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L37-L43
liveILLink.targets sets this to false because we don't want to reference the SDK's version of the package. Maybe we should use a similar condition for the liveILLink.targets.
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@sbomer
Import liveILLink.targets in libraries projects
d99b2d6
This should let libraries projects (whether they are source or test
projects) use the SDK's trimming properties with a live ILLink, enabling
the ProjectReference only for those projects that need it.
Because illink.targets also imports liveILLink.targets, this import is
guarded by a property to ensure liveILLink.targets is only imported
once on this path.
@sbomer
Fix import path
e0279d2
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/coreclr/Directory.Build.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/libraries/Directory.Build.targets
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

sbomerand others
added 2 commits
October 3, 2023 13:44
@sbomer@ViktorHofer
Apply suggestions from code review
b442dbc
- Fix indentation
- Only import liveILLink.targets in libraries when illink.targets is imported
- Respect MSBuildRuntimeType for live ILLinkTasksAssembly override
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
d18a62d
jkoritzinsky
jkoritzinsky
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/targetingpacks.targets
@sbomer
Revert "Fix libs test project that sets PublishTrimmed"
2c16391
This reverts commit be50c35.
@sbomer
Revert condition on liveILLink.targets import
96a663d
Various test configurations (wasm, nativeaot) rely on the live ILLink
when running libraries tests.
@ViktorHofer
ViktorHofer
commented
Oct 5, 2023

Copy link


Copy Markdown

Member
Looks like there is a remaining failure in the NativeAOT leg .
@sbomersbomer
mentioned this pull request
Oct 5, 2023
 Merged

1 task
@sbomer
Skip nonexistent targets when copying native binaries
9b9ed8d
This target isn't defined in the ILLink.Tasks.csproj ProjectReference.
Try skipping nonexistent targets to fix this.

This was referenced Oct 5, 2023

Open


Closed


Closed


Closed

@sbomer
sbomer
merged commit 1ef38a8
into
dotnet:mainOct 5, 2023
@sbomer
sbomer
commented
Oct 5, 2023

Copy link


Copy Markdown

MemberAuthor
Failures are known (it seems browser-wasm job timed out but didn't report status back to github). I couldn't have got this through without your help @ViktorHofer and @jkoritzinsky - thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 6, 2023

Closed

3 tasks

This was referenced Oct 26, 2023

Closed

 Merged

@sbomer
sbomer
deleted the

liveILLink

branch
November 3, 2023 18:38
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@ViktorHoferViktorHoferViktorHofer approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
@lewinglewingAwaiting requested review from lewing
@pavelsavarapavelsavaraAwaiting requested review from pavelsavara

Assignees

@sbomersbomer

Labels

needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sbomer@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
Remove dependencies on non-live illink - #91468
Merged
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Oct 5, 2023
Merged
Remove dependencies on non-live illink#91468
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Conversation
@sbomer
@sbomersbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

Member
 No description provided. 
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Sep 1, 2023
@ghostghost
assigned sbomerSep 1, 2023
jkoritzinsky
jkoritzinsky
reviewed
Sep 1, 2023
View reviewed changes
@jkoritzinskyjkoritzinsky
left a comment


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.

Can you take a look in targetingpacks.targets and update the KnownILLink item changes to point to the live ILLink?

This was referenced Sep 1, 2023

Closed


Closed


Closed


Open

@sbomer
sbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

MemberAuthor
I think we might be able to remove KnownILLinkPack entirely because we shouldn't be using the illink referenced by the SDK anywhere. Going to give this a try.
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
cb9f7b5
@sbomer
Don't require illink analyzers for source generator projects
a64cc2a
Microsoft.Extensions.Options.SourceGeneration was enabling
the single-file analyzer, but did not import illink.targets
because it is not a source project.
We could fix this by importing liveILLink.targets instead, but
this introduces more problems with circular dependencies
(generator projects depending on illink depending on
generator projects).
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
@ViktorHoferViktorHofer
left a comment
•

edited

Loading

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.

First of all: Amazing change!!
Slightly unrelated to this PR but still important from my point of view: Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/Analyzers.targets
Outdated

@sbomer
Only enable single-file analyzer for source projects
abc514f
This more closely matches the condition on the import of
illink.targets. When EnableSingleFileAnalyzer is true, we should
also import liveILLink.targets so that we get the live analyzer
bits.
Also address feedback about path separators.
@sbomer
Separate imports
597a012
This will make it easier to use different conditions for the two.
@sbomer
Fix libraries build
e83a2a0
Some libraries test projects set PublishTrimmed, so they require
the liveILLink.targets.
@sbomer
sbomer
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
I'm looking for a clean way to do this, but I'm not sure of the best way yet. _RequiresILLinkPack is effectively the SDK's version of this:
https://github.com/dotnet/sdk/blob/1c9e30a466d2c1f3b4673e3b0adfb02590dc8b74/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L37-L43
liveILLink.targets sets this to false because we don't want to reference the SDK's version of the package. Maybe we should use a similar condition for the liveILLink.targets.
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@sbomer
Import liveILLink.targets in libraries projects
d99b2d6
This should let libraries projects (whether they are source or test
projects) use the SDK's trimming properties with a live ILLink, enabling
the ProjectReference only for those projects that need it.
Because illink.targets also imports liveILLink.targets, this import is
guarded by a property to ensure liveILLink.targets is only imported
once on this path.
@sbomer
Fix import path
e0279d2
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/coreclr/Directory.Build.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/libraries/Directory.Build.targets
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

sbomerand others
added 2 commits
October 3, 2023 13:44
@sbomer@ViktorHofer
Apply suggestions from code review
b442dbc
- Fix indentation
- Only import liveILLink.targets in libraries when illink.targets is imported
- Respect MSBuildRuntimeType for live ILLinkTasksAssembly override
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
d18a62d
jkoritzinsky
jkoritzinsky
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/targetingpacks.targets
@sbomer
Revert "Fix libs test project that sets PublishTrimmed"
2c16391
This reverts commit be50c35.
@sbomer
Revert condition on liveILLink.targets import
96a663d
Various test configurations (wasm, nativeaot) rely on the live ILLink
when running libraries tests.
@ViktorHofer
ViktorHofer
commented
Oct 5, 2023

Copy link


Copy Markdown

Member
Looks like there is a remaining failure in the NativeAOT leg .
@sbomersbomer
mentioned this pull request
Oct 5, 2023
 Merged

1 task
@sbomer
Skip nonexistent targets when copying native binaries
9b9ed8d
This target isn't defined in the ILLink.Tasks.csproj ProjectReference.
Try skipping nonexistent targets to fix this.

This was referenced Oct 5, 2023

Open


Closed


Closed


Closed

@sbomer
sbomer
merged commit 1ef38a8
into
dotnet:mainOct 5, 2023
@sbomer
sbomer
commented
Oct 5, 2023

Copy link


Copy Markdown

MemberAuthor
Failures are known (it seems browser-wasm job timed out but didn't report status back to github). I couldn't have got this through without your help @ViktorHofer and @jkoritzinsky - thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 6, 2023

Closed

3 tasks

This was referenced Oct 26, 2023

Closed

 Merged

@sbomer
sbomer
deleted the

liveILLink

branch
November 3, 2023 18:38
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@ViktorHoferViktorHoferViktorHofer approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
@lewinglewingAwaiting requested review from lewing
@pavelsavarapavelsavaraAwaiting requested review from pavelsavara

Assignees

@sbomersbomer

Labels

needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sbomer@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
Remove dependencies on non-live illink - #91468
Merged
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Oct 5, 2023
Merged
Remove dependencies on non-live illink#91468
sbomer merged 34 commits into
dotnet:mainfrom 
sbomer:liveILLink
Conversation
@sbomer
@sbomersbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

Member
 No description provided. 
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Sep 1, 2023
@ghostghost
assigned sbomerSep 1, 2023
jkoritzinsky
jkoritzinsky
reviewed
Sep 1, 2023
View reviewed changes
@jkoritzinskyjkoritzinsky
left a comment


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.

Can you take a look in targetingpacks.targets and update the KnownILLink item changes to point to the live ILLink?

This was referenced Sep 1, 2023

Closed


Closed


Closed


Open

@sbomer
sbomer
commented
Sep 1, 2023

Copy link


Copy Markdown

MemberAuthor
I think we might be able to remove KnownILLinkPack entirely because we shouldn't be using the illink referenced by the SDK anywhere. Going to give this a try.
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
cb9f7b5
@sbomer
Don't require illink analyzers for source generator projects
a64cc2a
Microsoft.Extensions.Options.SourceGeneration was enabling
the single-file analyzer, but did not import illink.targets
because it is not a source project.
We could fix this by importing liveILLink.targets instead, but
this introduces more problems with circular dependencies
(generator projects depending on illink depending on
generator projects).
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
@ViktorHoferViktorHofer
left a comment
•

edited

Loading

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.

First of all: Amazing change!!
Slightly unrelated to this PR but still important from my point of view: Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Sep 2, 2023
View reviewed changes
Comment threadeng/Analyzers.targets
Outdated

@sbomer
Only enable single-file analyzer for source projects
abc514f
This more closely matches the condition on the import of
illink.targets. When EnableSingleFileAnalyzer is true, we should
also import liveILLink.targets so that we get the live analyzer
bits.
Also address feedback about path separators.
@sbomer
Separate imports
597a012
This will make it easier to use different conditions for the two.
@sbomer
Fix libraries build
e83a2a0
Some libraries test projects set PublishTrimmed, so they require
the liveILLink.targets.
@sbomer
sbomer
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?
I'm looking for a clean way to do this, but I'm not sure of the best way yet. _RequiresILLinkPack is effectively the SDK's version of this:
https://github.com/dotnet/sdk/blob/1c9e30a466d2c1f3b4673e3b0adfb02590dc8b74/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L37-L43
liveILLink.targets sets this to false because we don't want to reference the SDK's version of the package. Maybe we should use a similar condition for the liveILLink.targets.
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@build-analysisbuild-analysisBot
mentioned this pull request
Sep 5, 2023

Closed

@sbomer
Import liveILLink.targets in libraries projects
d99b2d6
This should let libraries projects (whether they are source or test
projects) use the SDK's trimming properties with a live ILLink, enabling
the ProjectReference only for those projects that need it.
Because illink.targets also imports liveILLink.targets, this import is
guarded by a property to ensure liveILLink.targets is only imported
once on this path.
@sbomer
Fix import path
e0279d2
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/coreclr/Directory.Build.targets
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadsrc/libraries/Directory.Build.targets
ViktorHofer
ViktorHofer
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/liveILLink.targets
Outdated

sbomerand others
added 2 commits
October 3, 2023 13:44
@sbomer@ViktorHofer
Apply suggestions from code review
b442dbc
- Fix indentation
- Only import liveILLink.targets in libraries when illink.targets is imported
- Respect MSBuildRuntimeType for live ILLinkTasksAssembly override
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@sbomer
Merge remote-tracking branch 'origin/main' into liveILLink
d18a62d
jkoritzinsky
jkoritzinsky
reviewed
Oct 3, 2023
View reviewed changes
Comment threadeng/targetingpacks.targets
@sbomer
Revert "Fix libs test project that sets PublishTrimmed"
2c16391
This reverts commit be50c35.
@sbomer
Revert condition on liveILLink.targets import
96a663d
Various test configurations (wasm, nativeaot) rely on the live ILLink
when running libraries tests.
@ViktorHofer
ViktorHofer
commented
Oct 5, 2023

Copy link


Copy Markdown

Member
Looks like there is a remaining failure in the NativeAOT leg .
@sbomersbomer
mentioned this pull request
Oct 5, 2023
 Merged

1 task
@sbomer
Skip nonexistent targets when copying native binaries
9b9ed8d
This target isn't defined in the ILLink.Tasks.csproj ProjectReference.
Try skipping nonexistent targets to fix this.

This was referenced Oct 5, 2023

Open


Closed


Closed


Closed

@sbomer
sbomer
merged commit 1ef38a8
into
dotnet:mainOct 5, 2023
@sbomer
sbomer
commented
Oct 5, 2023

Copy link


Copy Markdown

MemberAuthor
Failures are known (it seems browser-wasm job timed out but didn't report status back to github). I couldn't have got this through without your help @ViktorHofer and @jkoritzinsky - thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 6, 2023

Closed

3 tasks

This was referenced Oct 26, 2023

Closed

 Merged

@sbomer
sbomer
deleted the

liveILLink

branch
November 3, 2023 18:38
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyjkoritzinsky approved these changes
@ViktorHoferViktorHoferViktorHofer approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
@lewinglewingAwaiting requested review from lewing
@pavelsavarapavelsavaraAwaiting requested review from pavelsavara

Assignees

@sbomersbomer

Labels

needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@sbomer@ViktorHofer@jkoritzinsky