-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix build for System.Text.Encodings.Web #39448
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
Fix build for System.Text.Encodings.Web #39448
Conversation
|
Tagging subscribers to this area: @tarekgh |
pgovind
left a comment
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.
LGTM. I think System.Utf8String.Experimental.csproj also suffers from the same issue, but nobody has raised it yet, so maybe I'm wrong. I'll wait for Jan to confirm
|
I'm going to merge this for now to unblock STEW builds. We can revisit if Utf8String has issues later. |
| <Nullable>enable</Nullable> | ||
| </PropertyGroup> | ||
| <PropertyGroup> | ||
| <PropertyGroup Condition="'$(TargetFramework)' == 'netcoreapp3.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.
I should've caught this sooner. I'm wondering if this should be NETCOREAPPCURRENT instead of netcoreapp3.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.
In this case, no. If you take a look at the particular proj file, it is hardcoding netcoreapp3.0 in most places.
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.
Oh I see, you just refactored this. Still, the USE_INTERNAL_ACCESSIBILITY flag itself should be turned on only for NETCOREAPPCURRENT maybe?
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 you can test this by trying both -f netcoreapp30 and -f netcoreapp50 locally
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.
Nope, it's not needed. If you look at the item group at line 36, you'll see that the shims are only included for netcoreapp3.0.
Cherry picks #39447 for the preview8 branch. Addresses #39444