-
Notifications
You must be signed in to change notification settings - Fork 786
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
Consolidate NullableAttributes across OpenTelemetry and OpenTelemetry.Api #4293
Consolidate NullableAttributes across OpenTelemetry and OpenTelemetry.Api #4293
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4293 +/- ##
==========================================
- Coverage 84.77% 84.72% -0.06%
==========================================
Files 298 298
Lines 11984 11984
==========================================
- Hits 10159 10153 -6
- Misses 1825 1831 +6
|
@@ -0,0 +1,37 @@ | |||
// <copyright file="NullableAttributes.cs" company="OpenTelemetry Authors"> |
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.
I think that content from this file should be merged with existing one.
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.
These should absolutely be merged!
I didn't know this file was here! This is probably what was causing conflicts in my nullable branch. Thanks for pointing this out! :)
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.
I've run into an issue merging these two files and I'm still exploring options before I make a recommendation.
Both OpenTelemetry
and OpenTelemetry.Api
projects need access to nullables.
OTel has a build dependency on OTel.Api.
OTel.Api makes internals visible to OTel.
The blocker is that OTel targets net6.0;netstandard2.1;netstandard2.0;net462
where as OTel.Api targets netstandard2.0;net462
.
OTel (net6 and netstardard2.1) will take a dependency on OTel.Api (netstandard2.0).
And then OTel (net6 and netstardard2.1) has a conflict with runtime.
Error CS0433 The type 'AllowNullAttribute' exists in both 'OpenTelemetry.Api, Version=1.0.0.0, Culture=neutral, PublicKeyToken=7bd6737fe5b67e3c' and 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' OpenTelemetry (net6.0)
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.
The fix is to add netstandard2.1 to the OTel.Api project. I'll bring this up in the SIG meeting tomorrow.
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.
Discussed this in today's SIG and there are no blockers with this approach. :)
It would be preferred if we can move shared files to a separate project, but this only works if we can also remove the internals visible to between OTel and OTel.Api. I'm going to take a day to explore if this would work.
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
I'm closing this PR in favor of making a new Shared project. |
Towards #4338
NullableAttributes
are copied from dotnet runtime (link) to support older frameworks.This repo currently has nullable attributes spread across both
OpenTelemetry
project (link) andOpenTelemetry.Api
(link).This PR merges these into a single file.
Another issue that needed to be addressed is target versions between
OpenTelemetry
andOpenTelemetry.Api
projects.OpenTelemetry
project targetsnet6.0;netstandard2.1;netstandard2.0;net462
and has a dependency onOpenTelemetry.Api
which targetsnetstandard2.0;net462
.Both projects need access to
NullableAttributes
andOpenTelemetry.Api
makes its internals visible toOpenTelemetry
.Consider that the highest version of OpenTelemetry.Api is netstandard2.0, which means the NullableAttributes are always present.
The OpenTelemetry project will have access to these NullableAttributes in the Api project and will conflict with the dotnet runtime.
To address this, OpenTelemetry.Api needs to target netstandard2.1 so it produces a dll without those internals. See also this comment: SDK: Nullable annotations for base classes & batch + shims to enable compiler features #3374 (comment)
Changes
Please provide a brief description of the changes here.
netstandard2.1
toOpenTelemetry.Api
project.NullableAttributes
from OpenTelemetry to OpenTelemetry.Api.OpenTelemetry.Api
's Guard.cs and add to NullableAttributes.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes