Crossplatform ResourceUpdater - #89303

Merged
elinor-fung merged 79 commits into
dotnet:mainfrom
anatawa12:crossplatform-resource-updater
Aug 8, 2023
Merged

Crossplatform ResourceUpdater#89303
elinor-fung merged 79 commits into
dotnet:mainfrom
anatawa12:crossplatform-resource-updater

Conversation

@anatawa12

@anatawa12anatawa12 commented Jul 21, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

I tried to implement resource updater based on Mono.Cecil.Binary from mono repositorySystem.Reflection.Metadata and Compiler/Win32Resources

Fixes#3828
Fixes#88465

@ghostghost added area-HostModel community-contribution Indicates that the PR has been added by a community member labels Jul 21, 2023
@ghost

ghost commented Jul 21, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @vitek-karas, @agocke
See info in area-owners.md if you want to be subscribed.

Issue Details

I tried to implement resource updater based on Mono.Cecil.Binary from mono repository

Fixes #3828

Author:anatawa12
Assignees:-
Labels:

area-HostModel

Milestone:-

@vitek-karas

vitek-karas commented Jul 21, 2023

Copy link
Copy Markdown
Member

/cc @elinor-fung

@anatawa12anatawa12 changed the title Crossplatform resource updaterCrossplatform ResourceUpdaterJul 21, 2023
@build-analysisbuild-analysisBot mentioned this pull request Jul 21, 2023
Closed
@anatawa12

anatawa12 commented Jul 22, 2023

Copy link
Copy Markdown
ContributorAuthor

The test failure AppHost.Bundle.Tests_net8.0_arm64.html and HostActivation.Tests_net8.0_arm64.html are failed also on main branch in my environment (08a6e06) so I think those failure is not produced by my changes.

@vitek-karas

vitek-karas commented Jul 24, 2023

Copy link
Copy Markdown
Member

The test failures are suspect - these tests suites are exactly those which target the functionality around the ResourceUpdater... so we need to figure out what's wrong.

@anatawa12

anatawa12 commented Jul 25, 2023

Copy link
Copy Markdown
ContributorAuthor

Sorry I found the failure on main branch is my fault.
I fixed ResourceUpdater and ResourceWriter.

@elinor-fung

elinor-fung commented Jul 25, 2023

Copy link
Copy Markdown
Member

Thanks a lot for tackling this @anatawa12.

Some high level comments/questions about the implementation:

  1. Microsoft.NET.HostModel already depends on System.Reflection.Metadata. There seems to be quite some overlap between the functionality being pulled into Mono.Cecil.Binary and what is already provided in System.Reflection.PortableExecutable - specifically PEReader.
    • Could this use the reading functionality available in System.Reflection.Metadata instead?
  2. The runtime repo has some code used by other tools to read resource data into a model and update it: https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/Common/Compiler/Win32Resources
    • Could this re-use that resource data model instead?

In general, I'm looking to re-use existing code/functionality and pull in as little new logic as possible that we would have to ship in the SDK and maintain separately from other implementations. I think if the two things above are doable, it would just leave the actual writing/patching as new logic specific to HostModel.

Testing is probably my biggest concern here. This basically affects every build targeting Windows. Unfortunately, even running on Windows, we have a significant test hole around resource updating.

With the exception of those two tests, that were failing and you addressed, the tests in the runtime repo aren't actually using ResourceUpdater (if they were, I expect more tests would have failed). There is one test in dotnet/sdk that explicitly targets resources being copied from the managed dll, but that would only catch failures when we try to integrate runtime into sdk, rather than when the changes are made in runtime.

Ideally, we would have:

  • Unit tests for ResourceUpdater
    • Something basic like updating and reading it back to check that a resource was added
  • Integration tests using HostModel to update an apphost
    • Most tests in this repo don't actually go through HostWriter/ResourceUpdater and the ones that do actually don't pass in a binary from which to copy resources. I'm going to look into this - this was a surprise to me and we need to fix it.
  • E2E tests for cross-building an app on Unix targeting Windows
    • I don't think we really have a good mechanism for doing this in runtime. I believe the sdk repo has some tests that cross-build, so that may be where we add tests like this.

@anatawa12

anatawa12 commented Jul 26, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Could this use the reading functionality available in System.Reflection.Metadata instead?

I think yes. I didn't know about System.Reflection.Metadata. I'm going to rewriting with System.Reflection.Metadata.

Could this re-use that resource data model instead?

I think data model can be reused but I think we need new writing resource logic because it looks current logic is for object file, not suitable for image files. writing logic may also reusable.

@anatawa12
anatawa12force-pushed the crossplatform-resource-updater branch from 9b19911 to ed3f32aCompareAugust 5, 2023 15:35
@rebinfrebinf mentioned this pull request Aug 5, 2023
Closed
elinor-fung
elinor-fung reviewed Aug 7, 2023
View reviewed changes
Comment threadsrc/installer/managed/Microsoft.NET.HostModel/PEOffsets.cs Outdated
elinor-fung
elinor-fung approved these changes Aug 7, 2023
View reviewed changes

@elinor-fungelinor-fung 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.

Final minor comment. Excited to see this come together - thank you!

Comment thread...r/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/AppWithUnknownLanguageResource.cs Outdated
@anatawa12

anatawa12 commented Aug 8, 2023

Copy link
Copy Markdown
ContributorAuthor

#90136 fixed the CI failure. Thank you.

anatawa12 added a commit to anatawa12/dotnet-sdk that referenced this pull request Aug 8, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
90f83e8
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@anatawa12anatawa12 mentioned this pull request Aug 8, 2023
Closed
@elinor-fung
elinor-fung merged commit a3e38ff into dotnet:mainAug 8, 2023
@elinor-fung

elinor-fung commented Aug 8, 2023

Copy link
Copy Markdown
Member

Thank you, @anatawa12!

@elinor-fungelinor-fung mentioned this pull request Aug 8, 2023
Closed
@matt-richardson

matt-richardson commented Aug 8, 2023

Copy link
Copy Markdown

@anatawa12 - you are a legend. Thanks!

@akirayamamoto

akirayamamoto commented Aug 8, 2023

Copy link
Copy Markdown

Thank you @anatawa12, that is awesome!

@anatawa12anatawa12 mentioned this pull request Aug 9, 2023
Merged
anatawa12 added a commit to anatawa12/dotnet-sdk that referenced this pull request Aug 10, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
a47bf02
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@agocke

agocke commented Aug 14, 2023

Copy link
Copy Markdown
Member

@anatawa12 Thanks for your contribution, this is great! Really excellent work.

@vitek-karas

vitek-karas commented Aug 15, 2023

Copy link
Copy Markdown
Member

Thanks a lot @anatawa12 - this is great.

anatawa12 added a commit to anatawa12/dotnet-sdk that referenced this pull request Aug 15, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
7d63b26
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@elinor-fungelinor-fung mentioned this pull request Aug 22, 2023
Closed
3 tasks
@alexrpalexrp mentioned this pull request Aug 25, 2023
Closed
@ghostghost locked as resolved and limited conversation to collaborators Sep 14, 2023
@anatawa12
anatawa12 deleted the crossplatform-resource-updater branch February 24, 2024 19:07
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@elinor-fungelinor-fungelinor-fung approved these changes
@vitek-karasvitek-karasAwaiting requested review from vitek-karas
+1 more reviewer
@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

community-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CreateAppHost fails processing Win32 resources Use cross-platform code to update apphost win32 resources, open apphost only once for writing

8 participants

@anatawa12@vitek-karas@elinor-fung@akirayamamoto@MichalStrehovsky@matt-richardson@agocke@am11
, '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
Crossplatform ResourceUpdater - #89303
Merged
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Aug 8, 2023
Merged
Crossplatform ResourceUpdater#89303
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Conversation
@anatawa12
@anatawa12anatawa12
commented
Jul 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I tried to implement resource updater based on Mono.Cecil.Binary from mono repositorySystem.Reflection.Metadata and Compiler/Win32Resources
Fixes#3828
Fixes#88465
@ghostghost
added

area-HostModel

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 21, 2023
@ghost
ghost
commented
Jul 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @vitek-karas, @agocke

See info in area-owners.md if you want to be subscribed.
Issue Details
I tried to implement resource updater based on Mono.Cecil.Binary from mono repository
Fixes #3828
Author:anatawa12
Assignees:-
Labels:
area-HostModel
Milestone:-
@vitek-karas
vitek-karas
commented
Jul 21, 2023

Copy link


Copy Markdown

Member
/cc @elinor-fung
@anatawa12anatawa12
changed the title
Crossplatform resource updaterCrossplatform ResourceUpdaterJul 21, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 21, 2023

Closed

@anatawa12
anatawa12
commented
Jul 22, 2023

Copy link


Copy Markdown

ContributorAuthor
The test failure AppHost.Bundle.Tests_net8.0_arm64.html and HostActivation.Tests_net8.0_arm64.html are failed also on main branch in my environment (08a6e06) so I think those failure is not produced by my changes.
@vitek-karas
vitek-karas
commented
Jul 24, 2023

Copy link


Copy Markdown

Member
The test failures are suspect - these tests suites are exactly those which target the functionality around the ResourceUpdater... so we need to figure out what's wrong.
@anatawa12
anatawa12
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Sorry I found the failure on main branch is my fault.

I fixed ResourceUpdater and ResourceWriter.
@elinor-fung
elinor-fung
commented
Jul 25, 2023

Copy link


Copy Markdown

Member
Thanks a lot for tackling this @anatawa12.
Some high level comments/questions about the implementation:
  1. Microsoft.NET.HostModel already depends on System.Reflection.Metadata. There seems to be quite some overlap between the functionality being pulled into Mono.Cecil.Binary and what is already provided in System.Reflection.PortableExecutable - specifically PEReader.

    • Could this use the reading functionality available in System.Reflection.Metadata instead?
  2. The runtime repo has some code used by other tools to read resource data into a model and update it: https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/Common/Compiler/Win32Resources
    • Could this re-use that resource data model instead?
In general, I'm looking to re-use existing code/functionality and pull in as little new logic as possible that we would have to ship in the SDK and maintain separately from other implementations. I think if the two things above are doable, it would just leave the actual writing/patching as new logic specific to HostModel.
Testing is probably my biggest concern here. This basically affects every build targeting Windows. Unfortunately, even running on Windows, we have a significant test hole around resource updating.
With the exception of those two tests, that were failing and you addressed, the tests in the runtime repo aren't actually using ResourceUpdater (if they were, I expect more tests would have failed). There is one test in dotnet/sdk that explicitly targets resources being copied from the managed dll, but that would only catch failures when we try to integrate runtime into sdk, rather than when the changes are made in runtime.
Ideally, we would have:
  • Unit tests for ResourceUpdater
    • Something basic like updating and reading it back to check that a resource was added
  • Integration tests using HostModel to update an apphost

    • Most tests in this repo don't actually go through HostWriter/ResourceUpdater and the ones that do actually don't pass in a binary from which to copy resources. I'm going to look into this - this was a surprise to me and we need to fix it.
  • E2E tests for cross-building an app on Unix targeting Windows

    • I don't think we really have a good mechanism for doing this in runtime. I believe the sdk repo has some tests that cross-build, so that may be where we add tests like this.
@anatawa12
anatawa12
commented
Jul 26, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Could this use the reading functionality available in System.Reflection.Metadata instead?
I think yes. I didn't know about System.Reflection.Metadata. I'm going to rewriting with System.Reflection.Metadata.
Could this re-use that resource data model instead?
I think data model can be reused but I think we need new writing resource logic because it looks current logic is for object file, not suitable for image files. writing logic may also reusable.
@anatawa12
anatawa12force-pushed
the
crossplatform-resource-updater
branch
from
9b19911 to
ed3f32aCompareAugust 5, 2023 15:35
@rebinfrebinf
mentioned this pull request
Aug 5, 2023

Closed

elinor-fung
elinor-fung
reviewed
Aug 7, 2023
View reviewed changes
Comment threadsrc/installer/managed/Microsoft.NET.HostModel/PEOffsets.cs
Outdated

elinor-fung
elinor-fung
approved these changes
Aug 7, 2023
View reviewed changes
@elinor-fungelinor-fung
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.

Final minor comment. Excited to see this come together - thank you!
Comment thread...r/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/AppWithUnknownLanguageResource.cs
Outdated

@anatawa12
anatawa12
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
#90136 fixed the CI failure. Thank you.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 8, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
90f83e8
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@anatawa12anatawa12
mentioned this pull request
Aug 8, 2023

Closed

@elinor-fung
elinor-fung
merged commit a3e38ff
into
dotnet:mainAug 8, 2023
@elinor-fung
elinor-fung
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
Thank you, @anatawa12!
@elinor-fungelinor-fung
mentioned this pull request
Aug 8, 2023

Closed

@matt-richardson
matt-richardson
commented
Aug 8, 2023

Copy link


Copy Markdown

@anatawa12 - you are a legend. Thanks!
@akirayamamoto
akirayamamoto
commented
Aug 8, 2023

Copy link


Copy Markdown

Thank you @anatawa12, that is awesome!
@anatawa12anatawa12
mentioned this pull request
Aug 9, 2023
 Merged

anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 10, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
a47bf02
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@agocke
agocke
commented
Aug 14, 2023

Copy link


Copy Markdown

Member
@anatawa12 Thanks for your contribution, this is great! Really excellent work.
@vitek-karas
vitek-karas
commented
Aug 15, 2023

Copy link


Copy Markdown

Member
Thanks a lot @anatawa12 - this is great.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 15, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
7d63b26
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@elinor-fungelinor-fung
mentioned this pull request
Aug 22, 2023

Closed

3 tasks
@alexrpalexrp
mentioned this pull request
Aug 25, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 14, 2023
@anatawa12
anatawa12
deleted the

crossplatform-resource-updater

branch
February 24, 2024 19:07
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@elinor-fungelinor-fungelinor-fung approved these changes
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

community-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

CreateAppHost fails processing Win32 resources

Use cross-platform code to update apphost win32 resources, open apphost only once for writing


8 participants

@anatawa12@vitek-karas@elinor-fung@akirayamamoto@MichalStrehovsky@matt-richardson@agocke@am11

, '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
Crossplatform ResourceUpdater - #89303
Merged
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Aug 8, 2023
Merged
Crossplatform ResourceUpdater#89303
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Conversation
@anatawa12
@anatawa12anatawa12
commented
Jul 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I tried to implement resource updater based on Mono.Cecil.Binary from mono repositorySystem.Reflection.Metadata and Compiler/Win32Resources
Fixes#3828
Fixes#88465
@ghostghost
added

area-HostModel

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 21, 2023
@ghost
ghost
commented
Jul 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @vitek-karas, @agocke

See info in area-owners.md if you want to be subscribed.
Issue Details
I tried to implement resource updater based on Mono.Cecil.Binary from mono repository
Fixes #3828
Author:anatawa12
Assignees:-
Labels:
area-HostModel
Milestone:-
@vitek-karas
vitek-karas
commented
Jul 21, 2023

Copy link


Copy Markdown

Member
/cc @elinor-fung
@anatawa12anatawa12
changed the title
Crossplatform resource updaterCrossplatform ResourceUpdaterJul 21, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 21, 2023

Closed

@anatawa12
anatawa12
commented
Jul 22, 2023

Copy link


Copy Markdown

ContributorAuthor
The test failure AppHost.Bundle.Tests_net8.0_arm64.html and HostActivation.Tests_net8.0_arm64.html are failed also on main branch in my environment (08a6e06) so I think those failure is not produced by my changes.
@vitek-karas
vitek-karas
commented
Jul 24, 2023

Copy link


Copy Markdown

Member
The test failures are suspect - these tests suites are exactly those which target the functionality around the ResourceUpdater... so we need to figure out what's wrong.
@anatawa12
anatawa12
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Sorry I found the failure on main branch is my fault.

I fixed ResourceUpdater and ResourceWriter.
@elinor-fung
elinor-fung
commented
Jul 25, 2023

Copy link


Copy Markdown

Member
Thanks a lot for tackling this @anatawa12.
Some high level comments/questions about the implementation:
  1. Microsoft.NET.HostModel already depends on System.Reflection.Metadata. There seems to be quite some overlap between the functionality being pulled into Mono.Cecil.Binary and what is already provided in System.Reflection.PortableExecutable - specifically PEReader.

    • Could this use the reading functionality available in System.Reflection.Metadata instead?
  2. The runtime repo has some code used by other tools to read resource data into a model and update it: https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/Common/Compiler/Win32Resources
    • Could this re-use that resource data model instead?
In general, I'm looking to re-use existing code/functionality and pull in as little new logic as possible that we would have to ship in the SDK and maintain separately from other implementations. I think if the two things above are doable, it would just leave the actual writing/patching as new logic specific to HostModel.
Testing is probably my biggest concern here. This basically affects every build targeting Windows. Unfortunately, even running on Windows, we have a significant test hole around resource updating.
With the exception of those two tests, that were failing and you addressed, the tests in the runtime repo aren't actually using ResourceUpdater (if they were, I expect more tests would have failed). There is one test in dotnet/sdk that explicitly targets resources being copied from the managed dll, but that would only catch failures when we try to integrate runtime into sdk, rather than when the changes are made in runtime.
Ideally, we would have:
  • Unit tests for ResourceUpdater
    • Something basic like updating and reading it back to check that a resource was added
  • Integration tests using HostModel to update an apphost

    • Most tests in this repo don't actually go through HostWriter/ResourceUpdater and the ones that do actually don't pass in a binary from which to copy resources. I'm going to look into this - this was a surprise to me and we need to fix it.
  • E2E tests for cross-building an app on Unix targeting Windows

    • I don't think we really have a good mechanism for doing this in runtime. I believe the sdk repo has some tests that cross-build, so that may be where we add tests like this.
@anatawa12
anatawa12
commented
Jul 26, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Could this use the reading functionality available in System.Reflection.Metadata instead?
I think yes. I didn't know about System.Reflection.Metadata. I'm going to rewriting with System.Reflection.Metadata.
Could this re-use that resource data model instead?
I think data model can be reused but I think we need new writing resource logic because it looks current logic is for object file, not suitable for image files. writing logic may also reusable.
@anatawa12
anatawa12force-pushed
the
crossplatform-resource-updater
branch
from
9b19911 to
ed3f32aCompareAugust 5, 2023 15:35
@rebinfrebinf
mentioned this pull request
Aug 5, 2023

Closed

elinor-fung
elinor-fung
reviewed
Aug 7, 2023
View reviewed changes
Comment threadsrc/installer/managed/Microsoft.NET.HostModel/PEOffsets.cs
Outdated

elinor-fung
elinor-fung
approved these changes
Aug 7, 2023
View reviewed changes
@elinor-fungelinor-fung
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.

Final minor comment. Excited to see this come together - thank you!
Comment thread...r/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/AppWithUnknownLanguageResource.cs
Outdated

@anatawa12
anatawa12
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
#90136 fixed the CI failure. Thank you.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 8, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
90f83e8
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@anatawa12anatawa12
mentioned this pull request
Aug 8, 2023

Closed

@elinor-fung
elinor-fung
merged commit a3e38ff
into
dotnet:mainAug 8, 2023
@elinor-fung
elinor-fung
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
Thank you, @anatawa12!
@elinor-fungelinor-fung
mentioned this pull request
Aug 8, 2023

Closed

@matt-richardson
matt-richardson
commented
Aug 8, 2023

Copy link


Copy Markdown

@anatawa12 - you are a legend. Thanks!
@akirayamamoto
akirayamamoto
commented
Aug 8, 2023

Copy link


Copy Markdown

Thank you @anatawa12, that is awesome!
@anatawa12anatawa12
mentioned this pull request
Aug 9, 2023
 Merged

anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 10, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
a47bf02
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@agocke
agocke
commented
Aug 14, 2023

Copy link


Copy Markdown

Member
@anatawa12 Thanks for your contribution, this is great! Really excellent work.
@vitek-karas
vitek-karas
commented
Aug 15, 2023

Copy link


Copy Markdown

Member
Thanks a lot @anatawa12 - this is great.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 15, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
7d63b26
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@elinor-fungelinor-fung
mentioned this pull request
Aug 22, 2023

Closed

3 tasks
@alexrpalexrp
mentioned this pull request
Aug 25, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 14, 2023
@anatawa12
anatawa12
deleted the

crossplatform-resource-updater

branch
February 24, 2024 19:07
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@elinor-fungelinor-fungelinor-fung approved these changes
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

community-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

CreateAppHost fails processing Win32 resources

Use cross-platform code to update apphost win32 resources, open apphost only once for writing


8 participants

@anatawa12@vitek-karas@elinor-fung@akirayamamoto@MichalStrehovsky@matt-richardson@agocke@am11

, '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
Crossplatform ResourceUpdater - #89303
Merged
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Aug 8, 2023
Merged
Crossplatform ResourceUpdater#89303
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Conversation
@anatawa12
@anatawa12anatawa12
commented
Jul 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I tried to implement resource updater based on Mono.Cecil.Binary from mono repositorySystem.Reflection.Metadata and Compiler/Win32Resources
Fixes#3828
Fixes#88465
@ghostghost
added

area-HostModel

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 21, 2023
@ghost
ghost
commented
Jul 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @vitek-karas, @agocke

See info in area-owners.md if you want to be subscribed.
Issue Details
I tried to implement resource updater based on Mono.Cecil.Binary from mono repository
Fixes #3828
Author:anatawa12
Assignees:-
Labels:
area-HostModel
Milestone:-
@vitek-karas
vitek-karas
commented
Jul 21, 2023

Copy link


Copy Markdown

Member
/cc @elinor-fung
@anatawa12anatawa12
changed the title
Crossplatform resource updaterCrossplatform ResourceUpdaterJul 21, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 21, 2023

Closed

@anatawa12
anatawa12
commented
Jul 22, 2023

Copy link


Copy Markdown

ContributorAuthor
The test failure AppHost.Bundle.Tests_net8.0_arm64.html and HostActivation.Tests_net8.0_arm64.html are failed also on main branch in my environment (08a6e06) so I think those failure is not produced by my changes.
@vitek-karas
vitek-karas
commented
Jul 24, 2023

Copy link


Copy Markdown

Member
The test failures are suspect - these tests suites are exactly those which target the functionality around the ResourceUpdater... so we need to figure out what's wrong.
@anatawa12
anatawa12
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Sorry I found the failure on main branch is my fault.

I fixed ResourceUpdater and ResourceWriter.
@elinor-fung
elinor-fung
commented
Jul 25, 2023

Copy link


Copy Markdown

Member
Thanks a lot for tackling this @anatawa12.
Some high level comments/questions about the implementation:
  1. Microsoft.NET.HostModel already depends on System.Reflection.Metadata. There seems to be quite some overlap between the functionality being pulled into Mono.Cecil.Binary and what is already provided in System.Reflection.PortableExecutable - specifically PEReader.

    • Could this use the reading functionality available in System.Reflection.Metadata instead?
  2. The runtime repo has some code used by other tools to read resource data into a model and update it: https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/Common/Compiler/Win32Resources
    • Could this re-use that resource data model instead?
In general, I'm looking to re-use existing code/functionality and pull in as little new logic as possible that we would have to ship in the SDK and maintain separately from other implementations. I think if the two things above are doable, it would just leave the actual writing/patching as new logic specific to HostModel.
Testing is probably my biggest concern here. This basically affects every build targeting Windows. Unfortunately, even running on Windows, we have a significant test hole around resource updating.
With the exception of those two tests, that were failing and you addressed, the tests in the runtime repo aren't actually using ResourceUpdater (if they were, I expect more tests would have failed). There is one test in dotnet/sdk that explicitly targets resources being copied from the managed dll, but that would only catch failures when we try to integrate runtime into sdk, rather than when the changes are made in runtime.
Ideally, we would have:
  • Unit tests for ResourceUpdater
    • Something basic like updating and reading it back to check that a resource was added
  • Integration tests using HostModel to update an apphost

    • Most tests in this repo don't actually go through HostWriter/ResourceUpdater and the ones that do actually don't pass in a binary from which to copy resources. I'm going to look into this - this was a surprise to me and we need to fix it.
  • E2E tests for cross-building an app on Unix targeting Windows

    • I don't think we really have a good mechanism for doing this in runtime. I believe the sdk repo has some tests that cross-build, so that may be where we add tests like this.
@anatawa12
anatawa12
commented
Jul 26, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Could this use the reading functionality available in System.Reflection.Metadata instead?
I think yes. I didn't know about System.Reflection.Metadata. I'm going to rewriting with System.Reflection.Metadata.
Could this re-use that resource data model instead?
I think data model can be reused but I think we need new writing resource logic because it looks current logic is for object file, not suitable for image files. writing logic may also reusable.
@anatawa12
anatawa12force-pushed
the
crossplatform-resource-updater
branch
from
9b19911 to
ed3f32aCompareAugust 5, 2023 15:35
@rebinfrebinf
mentioned this pull request
Aug 5, 2023

Closed

elinor-fung
elinor-fung
reviewed
Aug 7, 2023
View reviewed changes
Comment threadsrc/installer/managed/Microsoft.NET.HostModel/PEOffsets.cs
Outdated

elinor-fung
elinor-fung
approved these changes
Aug 7, 2023
View reviewed changes
@elinor-fungelinor-fung
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.

Final minor comment. Excited to see this come together - thank you!
Comment thread...r/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/AppWithUnknownLanguageResource.cs
Outdated

@anatawa12
anatawa12
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
#90136 fixed the CI failure. Thank you.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 8, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
90f83e8
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@anatawa12anatawa12
mentioned this pull request
Aug 8, 2023

Closed

@elinor-fung
elinor-fung
merged commit a3e38ff
into
dotnet:mainAug 8, 2023
@elinor-fung
elinor-fung
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
Thank you, @anatawa12!
@elinor-fungelinor-fung
mentioned this pull request
Aug 8, 2023

Closed

@matt-richardson
matt-richardson
commented
Aug 8, 2023

Copy link


Copy Markdown

@anatawa12 - you are a legend. Thanks!
@akirayamamoto
akirayamamoto
commented
Aug 8, 2023

Copy link


Copy Markdown

Thank you @anatawa12, that is awesome!
@anatawa12anatawa12
mentioned this pull request
Aug 9, 2023
 Merged

anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 10, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
a47bf02
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@agocke
agocke
commented
Aug 14, 2023

Copy link


Copy Markdown

Member
@anatawa12 Thanks for your contribution, this is great! Really excellent work.
@vitek-karas
vitek-karas
commented
Aug 15, 2023

Copy link


Copy Markdown

Member
Thanks a lot @anatawa12 - this is great.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 15, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
7d63b26
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@elinor-fungelinor-fung
mentioned this pull request
Aug 22, 2023

Closed

3 tasks
@alexrpalexrp
mentioned this pull request
Aug 25, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 14, 2023
@anatawa12
anatawa12
deleted the

crossplatform-resource-updater

branch
February 24, 2024 19:07
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@elinor-fungelinor-fungelinor-fung approved these changes
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

community-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

CreateAppHost fails processing Win32 resources

Use cross-platform code to update apphost win32 resources, open apphost only once for writing


8 participants

@anatawa12@vitek-karas@elinor-fung@akirayamamoto@MichalStrehovsky@matt-richardson@agocke@am11

, '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
Crossplatform ResourceUpdater - #89303
Merged
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Aug 8, 2023
Merged
Crossplatform ResourceUpdater#89303
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Conversation
@anatawa12
@anatawa12anatawa12
commented
Jul 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I tried to implement resource updater based on Mono.Cecil.Binary from mono repositorySystem.Reflection.Metadata and Compiler/Win32Resources
Fixes#3828
Fixes#88465
@ghostghost
added

area-HostModel

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 21, 2023
@ghost
ghost
commented
Jul 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @vitek-karas, @agocke

See info in area-owners.md if you want to be subscribed.
Issue Details
I tried to implement resource updater based on Mono.Cecil.Binary from mono repository
Fixes #3828
Author:anatawa12
Assignees:-
Labels:
area-HostModel
Milestone:-
@vitek-karas
vitek-karas
commented
Jul 21, 2023

Copy link


Copy Markdown

Member
/cc @elinor-fung
@anatawa12anatawa12
changed the title
Crossplatform resource updaterCrossplatform ResourceUpdaterJul 21, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 21, 2023

Closed

@anatawa12
anatawa12
commented
Jul 22, 2023

Copy link


Copy Markdown

ContributorAuthor
The test failure AppHost.Bundle.Tests_net8.0_arm64.html and HostActivation.Tests_net8.0_arm64.html are failed also on main branch in my environment (08a6e06) so I think those failure is not produced by my changes.
@vitek-karas
vitek-karas
commented
Jul 24, 2023

Copy link


Copy Markdown

Member
The test failures are suspect - these tests suites are exactly those which target the functionality around the ResourceUpdater... so we need to figure out what's wrong.
@anatawa12
anatawa12
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Sorry I found the failure on main branch is my fault.

I fixed ResourceUpdater and ResourceWriter.
@elinor-fung
elinor-fung
commented
Jul 25, 2023

Copy link


Copy Markdown

Member
Thanks a lot for tackling this @anatawa12.
Some high level comments/questions about the implementation:
  1. Microsoft.NET.HostModel already depends on System.Reflection.Metadata. There seems to be quite some overlap between the functionality being pulled into Mono.Cecil.Binary and what is already provided in System.Reflection.PortableExecutable - specifically PEReader.

    • Could this use the reading functionality available in System.Reflection.Metadata instead?
  2. The runtime repo has some code used by other tools to read resource data into a model and update it: https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/Common/Compiler/Win32Resources
    • Could this re-use that resource data model instead?
In general, I'm looking to re-use existing code/functionality and pull in as little new logic as possible that we would have to ship in the SDK and maintain separately from other implementations. I think if the two things above are doable, it would just leave the actual writing/patching as new logic specific to HostModel.
Testing is probably my biggest concern here. This basically affects every build targeting Windows. Unfortunately, even running on Windows, we have a significant test hole around resource updating.
With the exception of those two tests, that were failing and you addressed, the tests in the runtime repo aren't actually using ResourceUpdater (if they were, I expect more tests would have failed). There is one test in dotnet/sdk that explicitly targets resources being copied from the managed dll, but that would only catch failures when we try to integrate runtime into sdk, rather than when the changes are made in runtime.
Ideally, we would have:
  • Unit tests for ResourceUpdater
    • Something basic like updating and reading it back to check that a resource was added
  • Integration tests using HostModel to update an apphost

    • Most tests in this repo don't actually go through HostWriter/ResourceUpdater and the ones that do actually don't pass in a binary from which to copy resources. I'm going to look into this - this was a surprise to me and we need to fix it.
  • E2E tests for cross-building an app on Unix targeting Windows

    • I don't think we really have a good mechanism for doing this in runtime. I believe the sdk repo has some tests that cross-build, so that may be where we add tests like this.
@anatawa12
anatawa12
commented
Jul 26, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Could this use the reading functionality available in System.Reflection.Metadata instead?
I think yes. I didn't know about System.Reflection.Metadata. I'm going to rewriting with System.Reflection.Metadata.
Could this re-use that resource data model instead?
I think data model can be reused but I think we need new writing resource logic because it looks current logic is for object file, not suitable for image files. writing logic may also reusable.
@anatawa12
anatawa12force-pushed
the
crossplatform-resource-updater
branch
from
9b19911 to
ed3f32aCompareAugust 5, 2023 15:35
@rebinfrebinf
mentioned this pull request
Aug 5, 2023

Closed

elinor-fung
elinor-fung
reviewed
Aug 7, 2023
View reviewed changes
Comment threadsrc/installer/managed/Microsoft.NET.HostModel/PEOffsets.cs
Outdated

elinor-fung
elinor-fung
approved these changes
Aug 7, 2023
View reviewed changes
@elinor-fungelinor-fung
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.

Final minor comment. Excited to see this come together - thank you!
Comment thread...r/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/AppWithUnknownLanguageResource.cs
Outdated

@anatawa12
anatawa12
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
#90136 fixed the CI failure. Thank you.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 8, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
90f83e8
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@anatawa12anatawa12
mentioned this pull request
Aug 8, 2023

Closed

@elinor-fung
elinor-fung
merged commit a3e38ff
into
dotnet:mainAug 8, 2023
@elinor-fung
elinor-fung
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
Thank you, @anatawa12!
@elinor-fungelinor-fung
mentioned this pull request
Aug 8, 2023

Closed

@matt-richardson
matt-richardson
commented
Aug 8, 2023

Copy link


Copy Markdown

@anatawa12 - you are a legend. Thanks!
@akirayamamoto
akirayamamoto
commented
Aug 8, 2023

Copy link


Copy Markdown

Thank you @anatawa12, that is awesome!
@anatawa12anatawa12
mentioned this pull request
Aug 9, 2023
 Merged

anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 10, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
a47bf02
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@agocke
agocke
commented
Aug 14, 2023

Copy link


Copy Markdown

Member
@anatawa12 Thanks for your contribution, this is great! Really excellent work.
@vitek-karas
vitek-karas
commented
Aug 15, 2023

Copy link


Copy Markdown

Member
Thanks a lot @anatawa12 - this is great.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 15, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
7d63b26
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@elinor-fungelinor-fung
mentioned this pull request
Aug 22, 2023

Closed

3 tasks
@alexrpalexrp
mentioned this pull request
Aug 25, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 14, 2023
@anatawa12
anatawa12
deleted the

crossplatform-resource-updater

branch
February 24, 2024 19:07
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@elinor-fungelinor-fungelinor-fung approved these changes
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

community-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

CreateAppHost fails processing Win32 resources

Use cross-platform code to update apphost win32 resources, open apphost only once for writing


8 participants

@anatawa12@vitek-karas@elinor-fung@akirayamamoto@MichalStrehovsky@matt-richardson@agocke@am11

, '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
Crossplatform ResourceUpdater - #89303
Merged
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Aug 8, 2023
Merged
Crossplatform ResourceUpdater#89303
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Conversation
@anatawa12
@anatawa12anatawa12
commented
Jul 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I tried to implement resource updater based on Mono.Cecil.Binary from mono repositorySystem.Reflection.Metadata and Compiler/Win32Resources
Fixes#3828
Fixes#88465
@ghostghost
added

area-HostModel

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 21, 2023
@ghost
ghost
commented
Jul 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @vitek-karas, @agocke

See info in area-owners.md if you want to be subscribed.
Issue Details
I tried to implement resource updater based on Mono.Cecil.Binary from mono repository
Fixes #3828
Author:anatawa12
Assignees:-
Labels:
area-HostModel
Milestone:-
@vitek-karas
vitek-karas
commented
Jul 21, 2023

Copy link


Copy Markdown

Member
/cc @elinor-fung
@anatawa12anatawa12
changed the title
Crossplatform resource updaterCrossplatform ResourceUpdaterJul 21, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 21, 2023

Closed

@anatawa12
anatawa12
commented
Jul 22, 2023

Copy link


Copy Markdown

ContributorAuthor
The test failure AppHost.Bundle.Tests_net8.0_arm64.html and HostActivation.Tests_net8.0_arm64.html are failed also on main branch in my environment (08a6e06) so I think those failure is not produced by my changes.
@vitek-karas
vitek-karas
commented
Jul 24, 2023

Copy link


Copy Markdown

Member
The test failures are suspect - these tests suites are exactly those which target the functionality around the ResourceUpdater... so we need to figure out what's wrong.
@anatawa12
anatawa12
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Sorry I found the failure on main branch is my fault.

I fixed ResourceUpdater and ResourceWriter.
@elinor-fung
elinor-fung
commented
Jul 25, 2023

Copy link


Copy Markdown

Member
Thanks a lot for tackling this @anatawa12.
Some high level comments/questions about the implementation:
  1. Microsoft.NET.HostModel already depends on System.Reflection.Metadata. There seems to be quite some overlap between the functionality being pulled into Mono.Cecil.Binary and what is already provided in System.Reflection.PortableExecutable - specifically PEReader.

    • Could this use the reading functionality available in System.Reflection.Metadata instead?
  2. The runtime repo has some code used by other tools to read resource data into a model and update it: https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/Common/Compiler/Win32Resources
    • Could this re-use that resource data model instead?
In general, I'm looking to re-use existing code/functionality and pull in as little new logic as possible that we would have to ship in the SDK and maintain separately from other implementations. I think if the two things above are doable, it would just leave the actual writing/patching as new logic specific to HostModel.
Testing is probably my biggest concern here. This basically affects every build targeting Windows. Unfortunately, even running on Windows, we have a significant test hole around resource updating.
With the exception of those two tests, that were failing and you addressed, the tests in the runtime repo aren't actually using ResourceUpdater (if they were, I expect more tests would have failed). There is one test in dotnet/sdk that explicitly targets resources being copied from the managed dll, but that would only catch failures when we try to integrate runtime into sdk, rather than when the changes are made in runtime.
Ideally, we would have:
  • Unit tests for ResourceUpdater
    • Something basic like updating and reading it back to check that a resource was added
  • Integration tests using HostModel to update an apphost

    • Most tests in this repo don't actually go through HostWriter/ResourceUpdater and the ones that do actually don't pass in a binary from which to copy resources. I'm going to look into this - this was a surprise to me and we need to fix it.
  • E2E tests for cross-building an app on Unix targeting Windows

    • I don't think we really have a good mechanism for doing this in runtime. I believe the sdk repo has some tests that cross-build, so that may be where we add tests like this.
@anatawa12
anatawa12
commented
Jul 26, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Could this use the reading functionality available in System.Reflection.Metadata instead?
I think yes. I didn't know about System.Reflection.Metadata. I'm going to rewriting with System.Reflection.Metadata.
Could this re-use that resource data model instead?
I think data model can be reused but I think we need new writing resource logic because it looks current logic is for object file, not suitable for image files. writing logic may also reusable.
@anatawa12
anatawa12force-pushed
the
crossplatform-resource-updater
branch
from
9b19911 to
ed3f32aCompareAugust 5, 2023 15:35
@rebinfrebinf
mentioned this pull request
Aug 5, 2023

Closed

elinor-fung
elinor-fung
reviewed
Aug 7, 2023
View reviewed changes
Comment threadsrc/installer/managed/Microsoft.NET.HostModel/PEOffsets.cs
Outdated

elinor-fung
elinor-fung
approved these changes
Aug 7, 2023
View reviewed changes
@elinor-fungelinor-fung
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.

Final minor comment. Excited to see this come together - thank you!
Comment thread...r/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/AppWithUnknownLanguageResource.cs
Outdated

@anatawa12
anatawa12
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
#90136 fixed the CI failure. Thank you.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 8, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
90f83e8
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@anatawa12anatawa12
mentioned this pull request
Aug 8, 2023

Closed

@elinor-fung
elinor-fung
merged commit a3e38ff
into
dotnet:mainAug 8, 2023
@elinor-fung
elinor-fung
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
Thank you, @anatawa12!
@elinor-fungelinor-fung
mentioned this pull request
Aug 8, 2023

Closed

@matt-richardson
matt-richardson
commented
Aug 8, 2023

Copy link


Copy Markdown

@anatawa12 - you are a legend. Thanks!
@akirayamamoto
akirayamamoto
commented
Aug 8, 2023

Copy link


Copy Markdown

Thank you @anatawa12, that is awesome!
@anatawa12anatawa12
mentioned this pull request
Aug 9, 2023
 Merged

anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 10, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
a47bf02
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@agocke
agocke
commented
Aug 14, 2023

Copy link


Copy Markdown

Member
@anatawa12 Thanks for your contribution, this is great! Really excellent work.
@vitek-karas
vitek-karas
commented
Aug 15, 2023

Copy link


Copy Markdown

Member
Thanks a lot @anatawa12 - this is great.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 15, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
7d63b26
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@elinor-fungelinor-fung
mentioned this pull request
Aug 22, 2023

Closed

3 tasks
@alexrpalexrp
mentioned this pull request
Aug 25, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 14, 2023
@anatawa12
anatawa12
deleted the

crossplatform-resource-updater

branch
February 24, 2024 19:07
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@elinor-fungelinor-fungelinor-fung approved these changes
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

community-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

CreateAppHost fails processing Win32 resources

Use cross-platform code to update apphost win32 resources, open apphost only once for writing


8 participants

@anatawa12@vitek-karas@elinor-fung@akirayamamoto@MichalStrehovsky@matt-richardson@agocke@am11

, '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
Crossplatform ResourceUpdater - #89303
Merged
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Aug 8, 2023
Merged
Crossplatform ResourceUpdater#89303
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Conversation
@anatawa12
@anatawa12anatawa12
commented
Jul 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I tried to implement resource updater based on Mono.Cecil.Binary from mono repositorySystem.Reflection.Metadata and Compiler/Win32Resources
Fixes#3828
Fixes#88465
@ghostghost
added

area-HostModel

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 21, 2023
@ghost
ghost
commented
Jul 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @vitek-karas, @agocke

See info in area-owners.md if you want to be subscribed.
Issue Details
I tried to implement resource updater based on Mono.Cecil.Binary from mono repository
Fixes #3828
Author:anatawa12
Assignees:-
Labels:
area-HostModel
Milestone:-
@vitek-karas
vitek-karas
commented
Jul 21, 2023

Copy link


Copy Markdown

Member
/cc @elinor-fung
@anatawa12anatawa12
changed the title
Crossplatform resource updaterCrossplatform ResourceUpdaterJul 21, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 21, 2023

Closed

@anatawa12
anatawa12
commented
Jul 22, 2023

Copy link


Copy Markdown

ContributorAuthor
The test failure AppHost.Bundle.Tests_net8.0_arm64.html and HostActivation.Tests_net8.0_arm64.html are failed also on main branch in my environment (08a6e06) so I think those failure is not produced by my changes.
@vitek-karas
vitek-karas
commented
Jul 24, 2023

Copy link


Copy Markdown

Member
The test failures are suspect - these tests suites are exactly those which target the functionality around the ResourceUpdater... so we need to figure out what's wrong.
@anatawa12
anatawa12
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Sorry I found the failure on main branch is my fault.

I fixed ResourceUpdater and ResourceWriter.
@elinor-fung
elinor-fung
commented
Jul 25, 2023

Copy link


Copy Markdown

Member
Thanks a lot for tackling this @anatawa12.
Some high level comments/questions about the implementation:
  1. Microsoft.NET.HostModel already depends on System.Reflection.Metadata. There seems to be quite some overlap between the functionality being pulled into Mono.Cecil.Binary and what is already provided in System.Reflection.PortableExecutable - specifically PEReader.

    • Could this use the reading functionality available in System.Reflection.Metadata instead?
  2. The runtime repo has some code used by other tools to read resource data into a model and update it: https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/Common/Compiler/Win32Resources
    • Could this re-use that resource data model instead?
In general, I'm looking to re-use existing code/functionality and pull in as little new logic as possible that we would have to ship in the SDK and maintain separately from other implementations. I think if the two things above are doable, it would just leave the actual writing/patching as new logic specific to HostModel.
Testing is probably my biggest concern here. This basically affects every build targeting Windows. Unfortunately, even running on Windows, we have a significant test hole around resource updating.
With the exception of those two tests, that were failing and you addressed, the tests in the runtime repo aren't actually using ResourceUpdater (if they were, I expect more tests would have failed). There is one test in dotnet/sdk that explicitly targets resources being copied from the managed dll, but that would only catch failures when we try to integrate runtime into sdk, rather than when the changes are made in runtime.
Ideally, we would have:
  • Unit tests for ResourceUpdater
    • Something basic like updating and reading it back to check that a resource was added
  • Integration tests using HostModel to update an apphost

    • Most tests in this repo don't actually go through HostWriter/ResourceUpdater and the ones that do actually don't pass in a binary from which to copy resources. I'm going to look into this - this was a surprise to me and we need to fix it.
  • E2E tests for cross-building an app on Unix targeting Windows

    • I don't think we really have a good mechanism for doing this in runtime. I believe the sdk repo has some tests that cross-build, so that may be where we add tests like this.
@anatawa12
anatawa12
commented
Jul 26, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Could this use the reading functionality available in System.Reflection.Metadata instead?
I think yes. I didn't know about System.Reflection.Metadata. I'm going to rewriting with System.Reflection.Metadata.
Could this re-use that resource data model instead?
I think data model can be reused but I think we need new writing resource logic because it looks current logic is for object file, not suitable for image files. writing logic may also reusable.
@anatawa12
anatawa12force-pushed
the
crossplatform-resource-updater
branch
from
9b19911 to
ed3f32aCompareAugust 5, 2023 15:35
@rebinfrebinf
mentioned this pull request
Aug 5, 2023

Closed

elinor-fung
elinor-fung
reviewed
Aug 7, 2023
View reviewed changes
Comment threadsrc/installer/managed/Microsoft.NET.HostModel/PEOffsets.cs
Outdated

elinor-fung
elinor-fung
approved these changes
Aug 7, 2023
View reviewed changes
@elinor-fungelinor-fung
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.

Final minor comment. Excited to see this come together - thank you!
Comment thread...r/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/AppWithUnknownLanguageResource.cs
Outdated

@anatawa12
anatawa12
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
#90136 fixed the CI failure. Thank you.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 8, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
90f83e8
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@anatawa12anatawa12
mentioned this pull request
Aug 8, 2023

Closed

@elinor-fung
elinor-fung
merged commit a3e38ff
into
dotnet:mainAug 8, 2023
@elinor-fung
elinor-fung
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
Thank you, @anatawa12!
@elinor-fungelinor-fung
mentioned this pull request
Aug 8, 2023

Closed

@matt-richardson
matt-richardson
commented
Aug 8, 2023

Copy link


Copy Markdown

@anatawa12 - you are a legend. Thanks!
@akirayamamoto
akirayamamoto
commented
Aug 8, 2023

Copy link


Copy Markdown

Thank you @anatawa12, that is awesome!
@anatawa12anatawa12
mentioned this pull request
Aug 9, 2023
 Merged

anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 10, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
a47bf02
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@agocke
agocke
commented
Aug 14, 2023

Copy link


Copy Markdown

Member
@anatawa12 Thanks for your contribution, this is great! Really excellent work.
@vitek-karas
vitek-karas
commented
Aug 15, 2023

Copy link


Copy Markdown

Member
Thanks a lot @anatawa12 - this is great.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 15, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
7d63b26
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@elinor-fungelinor-fung
mentioned this pull request
Aug 22, 2023

Closed

3 tasks
@alexrpalexrp
mentioned this pull request
Aug 25, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 14, 2023
@anatawa12
anatawa12
deleted the

crossplatform-resource-updater

branch
February 24, 2024 19:07
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@elinor-fungelinor-fungelinor-fung approved these changes
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

community-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

CreateAppHost fails processing Win32 resources

Use cross-platform code to update apphost win32 resources, open apphost only once for writing


8 participants

@anatawa12@vitek-karas@elinor-fung@akirayamamoto@MichalStrehovsky@matt-richardson@agocke@am11

, '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
Crossplatform ResourceUpdater - #89303
Merged
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Aug 8, 2023
Merged
Crossplatform ResourceUpdater#89303
elinor-fung merged 79 commits into
dotnet:mainfrom 
anatawa12:crossplatform-resource-updater
Conversation
@anatawa12
@anatawa12anatawa12
commented
Jul 21, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
I tried to implement resource updater based on Mono.Cecil.Binary from mono repositorySystem.Reflection.Metadata and Compiler/Win32Resources
Fixes#3828
Fixes#88465
@ghostghost
added

area-HostModel

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 21, 2023
@ghost
ghost
commented
Jul 21, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @vitek-karas, @agocke

See info in area-owners.md if you want to be subscribed.
Issue Details
I tried to implement resource updater based on Mono.Cecil.Binary from mono repository
Fixes #3828
Author:anatawa12
Assignees:-
Labels:
area-HostModel
Milestone:-
@vitek-karas
vitek-karas
commented
Jul 21, 2023

Copy link


Copy Markdown

Member
/cc @elinor-fung
@anatawa12anatawa12
changed the title
Crossplatform resource updaterCrossplatform ResourceUpdaterJul 21, 2023
@build-analysisbuild-analysisBot
mentioned this pull request
Jul 21, 2023

Closed

@anatawa12
anatawa12
commented
Jul 22, 2023

Copy link


Copy Markdown

ContributorAuthor
The test failure AppHost.Bundle.Tests_net8.0_arm64.html and HostActivation.Tests_net8.0_arm64.html are failed also on main branch in my environment (08a6e06) so I think those failure is not produced by my changes.
@vitek-karas
vitek-karas
commented
Jul 24, 2023

Copy link


Copy Markdown

Member
The test failures are suspect - these tests suites are exactly those which target the functionality around the ResourceUpdater... so we need to figure out what's wrong.
@anatawa12
anatawa12
commented
Jul 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Sorry I found the failure on main branch is my fault.

I fixed ResourceUpdater and ResourceWriter.
@elinor-fung
elinor-fung
commented
Jul 25, 2023

Copy link


Copy Markdown

Member
Thanks a lot for tackling this @anatawa12.
Some high level comments/questions about the implementation:
  1. Microsoft.NET.HostModel already depends on System.Reflection.Metadata. There seems to be quite some overlap between the functionality being pulled into Mono.Cecil.Binary and what is already provided in System.Reflection.PortableExecutable - specifically PEReader.

    • Could this use the reading functionality available in System.Reflection.Metadata instead?
  2. The runtime repo has some code used by other tools to read resource data into a model and update it: https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/Common/Compiler/Win32Resources
    • Could this re-use that resource data model instead?
In general, I'm looking to re-use existing code/functionality and pull in as little new logic as possible that we would have to ship in the SDK and maintain separately from other implementations. I think if the two things above are doable, it would just leave the actual writing/patching as new logic specific to HostModel.
Testing is probably my biggest concern here. This basically affects every build targeting Windows. Unfortunately, even running on Windows, we have a significant test hole around resource updating.
With the exception of those two tests, that were failing and you addressed, the tests in the runtime repo aren't actually using ResourceUpdater (if they were, I expect more tests would have failed). There is one test in dotnet/sdk that explicitly targets resources being copied from the managed dll, but that would only catch failures when we try to integrate runtime into sdk, rather than when the changes are made in runtime.
Ideally, we would have:
  • Unit tests for ResourceUpdater
    • Something basic like updating and reading it back to check that a resource was added
  • Integration tests using HostModel to update an apphost

    • Most tests in this repo don't actually go through HostWriter/ResourceUpdater and the ones that do actually don't pass in a binary from which to copy resources. I'm going to look into this - this was a surprise to me and we need to fix it.
  • E2E tests for cross-building an app on Unix targeting Windows

    • I don't think we really have a good mechanism for doing this in runtime. I believe the sdk repo has some tests that cross-build, so that may be where we add tests like this.
@anatawa12
anatawa12
commented
Jul 26, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Could this use the reading functionality available in System.Reflection.Metadata instead?
I think yes. I didn't know about System.Reflection.Metadata. I'm going to rewriting with System.Reflection.Metadata.
Could this re-use that resource data model instead?
I think data model can be reused but I think we need new writing resource logic because it looks current logic is for object file, not suitable for image files. writing logic may also reusable.
@anatawa12
anatawa12force-pushed
the
crossplatform-resource-updater
branch
from
9b19911 to
ed3f32aCompareAugust 5, 2023 15:35
@rebinfrebinf
mentioned this pull request
Aug 5, 2023

Closed

elinor-fung
elinor-fung
reviewed
Aug 7, 2023
View reviewed changes
Comment threadsrc/installer/managed/Microsoft.NET.HostModel/PEOffsets.cs
Outdated

elinor-fung
elinor-fung
approved these changes
Aug 7, 2023
View reviewed changes
@elinor-fungelinor-fung
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.

Final minor comment. Excited to see this come together - thank you!
Comment thread...r/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/AppWithUnknownLanguageResource.cs
Outdated

@anatawa12
anatawa12
commented
Aug 8, 2023

Copy link


Copy Markdown

ContributorAuthor
#90136 fixed the CI failure. Thank you.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 8, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
90f83e8
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@anatawa12anatawa12
mentioned this pull request
Aug 8, 2023

Closed

@elinor-fung
elinor-fung
merged commit a3e38ff
into
dotnet:mainAug 8, 2023
@elinor-fung
elinor-fung
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
Thank you, @anatawa12!
@elinor-fungelinor-fung
mentioned this pull request
Aug 8, 2023

Closed

@matt-richardson
matt-richardson
commented
Aug 8, 2023

Copy link


Copy Markdown

@anatawa12 - you are a legend. Thanks!
@akirayamamoto
akirayamamoto
commented
Aug 8, 2023

Copy link


Copy Markdown

Thank you @anatawa12, that is awesome!
@anatawa12anatawa12
mentioned this pull request
Aug 9, 2023
 Merged

anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 10, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
a47bf02
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@agocke
agocke
commented
Aug 14, 2023

Copy link


Copy Markdown

Member
@anatawa12 Thanks for your contribution, this is great! Really excellent work.
@vitek-karas
vitek-karas
commented
Aug 15, 2023

Copy link


Copy Markdown

Member
Thanks a lot @anatawa12 - this is great.
anatawa12 added a commit
to anatawa12/dotnet-sdk
that referenced
this pull request
Aug 15, 2023
@anatawa12
remove all guard with ResourceUpdater.IsSupportedOS()
7d63b26
ResourceUpdater.IsSupportedOS() will always return true since dotnet/runtime#89303
@elinor-fungelinor-fung
mentioned this pull request
Aug 22, 2023

Closed

3 tasks
@alexrpalexrp
mentioned this pull request
Aug 25, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 14, 2023
@anatawa12
anatawa12
deleted the

crossplatform-resource-updater

branch
February 24, 2024 19:07
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@elinor-fungelinor-fungelinor-fung approved these changes
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

community-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

CreateAppHost fails processing Win32 resources

Use cross-platform code to update apphost win32 resources, open apphost only once for writing


8 participants

@anatawa12@vitek-karas@elinor-fung@akirayamamoto@MichalStrehovsky@matt-richardson@agocke@am11