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

Add support disclaimer to data-formats.md #107341

Closed
wants to merge 1 commit into from

Conversation

MichalStrehovsky
Copy link
Member

We don't consider e.g. custom attribute annotation supported.

Cc @dotnet/illink

We don't consider e.g. custom attribute annotation supported.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Sep 4, 2024
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Sep 4, 2024
Copy link
Contributor

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

@@ -1,5 +1,9 @@
# Data Formats

> [!WARNING]
> The file formats described in this file may not be supported for use outside the dotnet/runtime repo. Refer to the official documentation at https://learn.microsoft.com/dotnet/core/deploying/trimming/trim-self-contained for supported formats.
Copy link
Contributor

Choose a reason for hiding this comment

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

https://learn.microsoft.com/en-gb/dotnet/core/deploying/trimming/trimming-options#root-descriptors links back to this document. This means that only root descriptors are supported for external use?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://learn.microsoft.com/en-gb/dotnet/core/deploying/trimming/trimming-options#root-descriptors links back to this document. This means that only root descriptors are supported for external use?

We very likely do not want to link to this doc from learn, whatever is supported should be moved to the docs repo. I guess that link explains how a customer dug out the attribute XML format the other day (cc @sbomer @agocke).

Copy link
Member

Choose a reason for hiding this comment

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

Personally I would be fine leaving the doc here - there are several other places where learn links to dotnet/runtime for the next level of detail.

Could we move the warning down to the custom attribute annotation section?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we sure the rest is supported?

I just quickly skimmed the doc - how about the "Override static field value with a constant" part for example?

Copy link
Member

Choose a reason for hiding this comment

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

I think we weren't very deliberate about defining the support level of these features when they were introduced. I would like to consider that one as unsupported as well. Ideally by actually removing the functionality (https://github.com/dotnet/linker/issues/2323) but adding a warning there too seems good for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we weren't very deliberate about defining the support level of these features when they were introduced.

This is a general problem with having official documentation in the dotnet/runtime repo. These files are not thought about as public contracts - neither the author of the doc nor the reviewers look at it that way.

Official doc changes are always looked at as public contracts because there's a clear context switch between "I'm developing a feature" and "I'm documenting a contract". Plus doing that in the docs repo ensures the doc matches the standards of official docs by getting a review from relevant doc subject matter experts.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also ok with moving the descriptor XML docs to ms learn. In general I prefer to get rid of the unsupported features entirely if we can, otherwise there will likely be some customers who find and use those features.

Copy link
Contributor

Choose a reason for hiding this comment

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

In an unreleased library of mine, I am using a substitutions file to trim the resource keys if resource keys are enabled, copying the BCL's mechanism. Also F# is using a substitutions file to remove some big binary resources from FSharp.Core with signature data (which I copied to a released F# library of mine).

Are these not supported for external use?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like that's already used in a few places: https://github.com/search?q=path%3AILLink.Substitutions.xml+resource&type=code&p=1. I see CsWinRT, FSharp.Core, and Farkle. I think we need to keep this one around, but probably don't want to advertise it. It technically can break user code with no warnings, similar to attribute stripping (see #103934).

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a recommendation for what these places should use instead?

@MichalStrehovsky MichalStrehovsky deleted the MichalStrehovsky-patch-1 branch December 9, 2024 07:50
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants