Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the metadata update related APIs to the MetadataUpdater class #54590

Merged
merged 6 commits into from
Jun 25, 2021

Conversation

mikem8361
Copy link
Member

The old APIs AssemblyExtensions.ApplyUpdate() and AssemblyExtensions.GetApplyUpdateCapabilities() will be removed
after all the references to them have been changed to the new APIs.

Add the new IsSupported API described in issue #51159.

Change the tests to use the MetadataUpdater APIs.

/cc: @pranavkm, @eerhardt

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@mikem8361
Copy link
Member Author

@lambdageek, I'm sure the mono version of MetadataUpdater.IsSupported isn't complete, but there is a place holder.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

Mono bits LGTM. CoreCLR looks ok, too, but I'm not super-familiar with the code.

One thing to watch for: the ApplyUpdateTest testsuite only runs in environments where ApplyUpdateUtil.IsSupported is true - for CoreCLR that's basically everywhere, but for Mono in environments where we can't use the remote executor (ie mobile and webassembly) the whole class will only run when DOTNET_MODIFIABLE_ASSEMBLIES is already set appropriately by the launcher.

So MetadataUpdater.IsSupported will always return true if the AssemblyUpdateTest class is not skipped under Mono.

@mikem8361
Copy link
Member Author

All I'm waiting for now is for @eerhardt to approve the feature switch changes and for the API review for making MetadataUpdater.GetCapabilities() public.

The old APIs AssemblyExtensions.ApplyUpdate() and AssemblyExtensions.GetApplyUpdateCapabilities() will be removed
after all the references to them have been changed to the new APIs.

Add the new IsSupported API described in issue dotnet#51159.

Change the tests to use the MetadataUpdater APIs.
Fix the ApplyUpdate qcalls and icalls

Add the ILLink substitutions for MetadataUpdater.IsSupported property

Change the old APIs to call the new ones
…) internal.

Fixed the argument checking in coreclr's MetadataUpdater.ApplyUpdate().
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@stephentoub
Copy link
Member

We should also update

<type fullname="System.Reflection.Metadata.MetadataUpdateHandlerAttribute">
to be based on the new switch rather than on Debugger.IsSupported.

@mikem8361
Copy link
Member Author

How would one make the attribute do that? The xml file just seems to be the attribute type.

@stephentoub
Copy link
Member

It's currently in the section for the debugger switch:

<assembly fullname="System.Private.CoreLib" feature="System.Diagnostics.Debugger.IsSupported" featurevalue="false">

A new section should be added for the new switch and this attribute moved to that section.

@mikem8361 mikem8361 merged commit cef40a1 into dotnet:main Jun 25, 2021
@mikem8361 mikem8361 deleted the metadataupdater branch June 25, 2021 05:54
@ghost ghost locked as resolved and limited conversation to collaborators Jul 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants