Add public APIs for persisted AssemblyBulder by buyaa-n · Pull Request #97177 · dotnet/runtime

buyaa-n · 2024-01-18T21:18:32Z

Add public APIs for persisted AssemblyBulder, approve API shape:

namespaceSystem.Reflection.Emit;publicpartialclassAssemblyBuilder{// New API - note that it does not have RequiresDynamicCode annotationpublicstaticAssemblyBuilderDefinePersistedAssembly(AssemblyNamename,AssemblycoreAssembly,IEnumerable<CustomAttributeBuilder>?assemblyAttributes=null);publicvoidSave(Streamstream);publicvoidSave(stringassemblyFileName);protectedabstractvoidSaveCore(Streamstream);}

Use the public APIs for testing
Refactor tests to make sure temp files disposed after test

Contributes to #92975

… deleted

ghost · 2024-01-18T21:18:40Z

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, 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.

ghost · 2024-01-18T21:19:09Z

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

Issue Details

Add public APIs for persisted AssemblyBulder, approve API shape:

namespaceSystem.Reflection.Emit;publicpartialclassAssemblyBuilder{// New API - note that it does not have RequiresDynamicCode annotationpublicstaticAssemblyBuilderDefinePersistedAssembly(AssemblyNamename,AssemblycoreAssembly,IEnumerable<CustomAttributeBuilder>?assemblyAttributes=null);publicvoidSave(Streamstream);publicvoidSave(stringassemblyFileName);protectedabstractvoidSaveCore(Streamstream);}

Use the public APIs for testing
Refactor tests to make sure temp files disposed after test

Author:	buyaa-n
Assignees:	-
Labels:	`area-System.Reflection.Emit`, `new-api-needs-documentation`
Milestone:	-

stephentoub · 2024-01-18T21:38:43Z

+ /// When implemented in derived type serializer the assembly to stream.
+ /// </summary>
+ /// <param name="stream">The stream to which the assembly serialized.</param>
+ protected abstract void SaveCore(Stream stream);


Was it not possible for someone externally to derive from AssemblyBuilder in .NET 8 and earlier? Adding a new abstract method will break anyone who did.

We made the AssemblyBuilder abstract in .NET 8, before it was sealed. So only .NET 8 can be considered, and I think it's unlikely somebody would have derived from AssemblyBuilder and build their own implementation.

If it was possible to derive from AssemblyBuilder in .NET 8, then this is a binary breaking change. I think we need to make it virtual with the base implementation throwing.
cc: @terrajobst

…iton thrown

Co-authored-by: Stephen Toub <stoub@microsoft.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com>

steveharter

LGTM; some misc comments. Thanks for this important feature!

steveharter · 2024-01-19T18:38:29Z

 <!-- https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/diagnostic-ids -->
 <Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
-</Suppressions>
+</Suppressions>


nit: revert this file

it is reverted, could not revert the line ending

buyaa-n · 2024-01-21T02:22:43Z

Failures unrelated and known

* Add public APIs for persisted AssemblyBuilder * Update tests to use public API, refactor tests so that the temp files deleted * Make AB.SaveCore(Stream) virtual and add meaningful message for excepiton thrown Co-authored-by: Stephen Toub <stoub@microsoft.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com> * Remove unneeded message * Update newer tests to use new public APIs --------- Co-authored-by: Stephen Toub <stoub@microsoft.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com>

buyaa-n added 2 commits January 18, 2024 10:34

Add public APIs for persisted AssemblyBuilder
cd86a5b

Update tests to use public API, refactor tests so that the temp files…
0dc5f12
… deleted

buyaa-n requested review from AaronRobinsonMSFT, jkotas and steveharter January 18, 2024 21:18

ghost added area-System.Reflection.Emit new-api-needs-documentation labels Jan 18, 2024

ghost assigned buyaa-n Jan 18, 2024

buyaa-n mentioned this pull request Jan 18, 2024
Tracking issue for remaining AssemblyBuilder.Save work in .NET 9 #92975
Closed
20 tasks

stephentoub reviewed Jan 18, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs Outdated

stephentoub reviewed Jan 18, 2024
View reviewed changes

Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeAssemblyBuilder.cs Outdated

stephentoub reviewed Jan 18, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs

stephentoub reviewed Jan 18, 2024
View reviewed changes

This was referenced Jan 19, 2024
Error while retrieving client Settings for PipelineArtifact. HttpRequestException: nodename nor servname provided, or not known (vsblobprodcus3.vsblob.visualstudio.com:443) #96798
Open
BasicEventSourceTests.TestsManifestGeneration test failing in CI #97103
Closed

Make AB.SaveCore(Stream) virtual and add meaningful message for excep…
7b79bea
…iton thrown

build-analysisBot mentioned this pull request Jan 19, 2024
Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009
Open
2 tasks

jkotas reviewed Jan 19, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs Outdated

stephentoub reviewed Jan 19, 2024
View reviewed changes

Comment threadsrc/libraries/System.Reflection.Emit/ref/System.Reflection.Emit.cs Outdated

stephentoub reviewed Jan 19, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

stephentoub reviewed Jan 19, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

stephentoub reviewed Jan 19, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs Outdated

stephentoub reviewed Jan 19, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

jkotas reviewed Jan 19, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs Outdated

Apply suggestions from code review
23304b6
Co-authored-by: Stephen Toub <stoub@microsoft.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com>

buyaa-n commented Jan 19, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated

Remove unneeded message
f854c54

buyaa-n commented Jan 19, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs Outdated

buyaa-n commented Jan 19, 2024
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.cs Outdated

Apply suggestions from code review
444225b

build-analysisBot mentioned this pull request Jan 19, 2024
System.Security.Cryptography.X509Certificates.Tests.ChainTests.BuildInvalidSignatureTwice failure #82837
Open

steveharter approved these changes Jan 19, 2024
View reviewed changes

Apply feedbacks
eaa7434

build-analysisBot mentioned this pull request Jan 19, 2024
Tests crashing in CI with no dump: exit code 137 means SIGKILL Killed #97049
Closed

buyaa-n added 2 commits January 19, 2024 14:39

Merge conflicts
509a4c0

Update newer tests to use new public APIs
d1e8d20

This was referenced Jan 20, 2024
[browser][MT] various WebWorkerTest CI failures #96628
Closed
[browser][MT] GC deadlock #96190
Closed
Test failing: System.Globalization.Tests.CompareInfoHashCodeTests.CheckHashingOfSkippedChars #97241
Closed

stephentoub approved these changes Jan 20, 2024
View reviewed changes

buyaa-n mentioned this pull request Jan 20, 2024
System.Diagnostics.Tracing.Tests BasicEventSourceTests.TestsManifestGeneration test fails on windows x86 leg #97255
Closed

Merge conflicts
a4c735a

This was referenced Jan 21, 2024
ReadAsStreamAsync_HandlerProducesWellBehavedResponseStream test failure on release coreclr windows x64 Debug CI leg #97259
Closed
[browser][MT] WebSocket is already in CLOSING or CLOSED state #97260
Closed

buyaa-n merged commit bd84a96 into dotnet:mainJan 21, 2024

buyaa-n deleted the ab_save_public branch January 21, 2024 02:23

Spacefish mentioned this pull request Jan 26, 2024
AOT Support for .NET 9.0 curiosity-ai/rocksdb-sharp#52
Open

buyaa-n mentioned this pull request Feb 6, 2024
[API Proposal] Add persist able AssemblyBuilder implementation #83988
Closed

github-actionsBot locked and limited conversation to collaborators Feb 20, 2024

Uh oh!

Conversation

buyaa-n commented Jan 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 18, 2024

Uh oh!

ghost commented Jan 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentoubJan 18, 2024

Choose a reason for hiding this comment

Uh oh!

buyaa-nJan 18, 2024

Choose a reason for hiding this comment

Uh oh!

stephentoubJan 18, 2024

Choose a reason for hiding this comment

Uh oh!

buyaa-nJan 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveharter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveharterJan 19, 2024

Choose a reason for hiding this comment

Uh oh!

buyaa-nJan 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

buyaa-n commented Jan 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

buyaa-n commented Jan 18, 2024•
edited
Loading