Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[AOT] Fix RuntimeContext warnings #4460
[AOT] Fix RuntimeContext warnings #4460
Changes from 34 commits
87cfa2a
bf8a530
b133982
236a5f1
d4c38af
bd2d709
db04bd1
260327b
6be9d0c
5dae942
ebee07a
299fc2a
e93903f
cc00b30
99c8baa
a773917
fc32b25
6ef81cd
23b77b5
6faf599
e6b016c
44bb210
8d76270
05bb4b6
8586200
452f6a6
649e39d
f0c7f44
f8714f9
7956ab1
d2024a8
b9c0b33
4120ffb
633da1b
7c1fde4
48d0b7f
19fce6a
35173c8
21495bc
586511d
9ba6626
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need a
net7.0
target for this to work?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute is in the runtime of .net7.0 and .net8.0: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.requiresdynamiccodeattribute?view=net-7.0
Therefore, we will only need to compile this file if it is not net7.0 or greater.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's basically future proofing the code. That way if/when we add a net7.0+ target, this code doesn't get compiled for it. Yes it isn't strictly necessary now, but it is saving some effort/confusion for a future dev who adds a new TFM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of forces us to always add the new target to the API project as well. If in the future, the SDK project also ends up using this attribute, we wouldn't be able to add a net7.0+ target only to the SDK project as it would complain about the attribute existing in both
OpenTelemetry.Api
(because of InternalsVisible) andSystem.Runtime
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a recommendation to resolve this? The only other option I could imagine is to
#if NET7_OR_GREATER
where we need to use this attribute AND add anet7.0
target now in order to use the attribute.Using InternalsVisibleTo across separate NuGet packages isn't recommended, since the package versions can get out of sync and users will see errors.
In general, matching TFMs across these packages is preferrable. It makes maintenance much easier. It is how we manage our packages we ship out of dotnet/runtime. They all have a consistent set of targets. For the 8.0-* packages it is:
net6.0;net7.0;net8.0;netstandard2.0;net462
. For example see https://www.nuget.org/packages/System.Collections.Immutable/8.0.0-preview.4.23259.5#dependencies-body-tab:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of this: https://github.com/open-telemetry/opentelemetry-dotnet/actions/runs/5028125135/jobs/9018449888?pr=4460.
I plan to address these in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for Fixing:
https://github.com/open-telemetry/opentelemetry-dotnet/actions/runs/5009473252/jobs/8978448877