Skip to content

Conversation

@carlossanlop
Copy link
Contributor

@carlossanlop carlossanlop commented Feb 11, 2025

I noticed the ref cs files from these two assemblies are not correctly updated with the latest automatically generated public surface. This might be an issue since before the GenAPI tool was created and this readme file was written: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/updating-ref-source.md

I saw we have a discussion around hiding attributes: #66090 but I couldn't find a discussion around making sure all our ref files are correctly updated and generated using GenAPI (not manually).

I am also removing a couple of test files that were added by mistake many years ago probably to test some experimental features that were never merged. These test files are not among the Compile items in the csproj:

@ghost
Copy link

ghost commented Feb 11, 2025

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
Copy link

ghost commented Feb 11, 2025

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.

@dotnet-policy-service
Copy link
Contributor

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

public override void Write(System.ReadOnlySpan<byte> buffer) { }
public override System.Threading.Tasks.Task WriteAsync(byte[] buffer, int offset, int count, System.Threading.CancellationToken cancellationToken) { throw null; }
public override System.Threading.Tasks.ValueTask WriteAsync(System.ReadOnlyMemory<byte> buffer, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public override System.Threading.Tasks.ValueTask WriteAsync(System.ReadOnlyMemory<byte> buffer, System.Threading.CancellationToken cancellationToken) { throw null; }
Copy link
Member

Choose a reason for hiding this comment

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

This will be a source-breaking change. Can we double check which is intended here?

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure this is a bug in the src rather than the ref. I see the base WriteAsync has the default parameter https://github.com/dotnet/runtime/blob/4da708a7060d238354d79823e58839c1d92f7fd1/src/libraries/System.Runtime/ref/System.Runtime.cs#L10773

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just about to paste this, you're fast. Yes, the src is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the ref.

Fixed = 4
Fixed = 4,
}
public partial class ZLibException : System.IO.IOException, System.Runtime.Serialization.ISerializable
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this was intentionally omitted

<!-- Exposed publicly only in implementation for serialization compat. -->
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:System.IO.Compression.ZLibException</Target>
</Suppression>

Perhaps you can configure GenAPI so that it will exclude it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me understand how it works first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll open an issue to keep investigating later #112451

I'll jump to work on the m-p issue.

Copy link
Member

Choose a reason for hiding this comment

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

I added a comment to #112451 (comment) with how to do this. Your call if you want to do it in this PR or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks. I think I should do it in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed the changes. Nice to see that after creating the txt file and adding that property to the csproj, the ZLibException is no longer getting auto-added to the ref cs when running the dotnet msbuild /t:GenerateReferenceAssemblySource command.

@@ -1,115 +0,0 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

Agree this looks accidental in dotnet/corefx#30364

@carlossanlop
Copy link
Contributor Author

/ba-g Timeouts/deadletters in Windows Server 2025 (known/pre-existing).

@carlossanlop carlossanlop merged commit 8c3506a into dotnet:main Feb 12, 2025
80 of 87 checks passed
@carlossanlop carlossanlop deleted the CompressionRefs branch February 12, 2025 23:53
@github-actions github-actions bot locked and limited conversation to collaborators Mar 15, 2025
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.

2 participants