Fix compile error when Azure SDK is targeting .NET 6. #2245

AraHaan · 2022-05-22T03:49:33Z

Description

When I tried to do Azure/azure-sdk-for-net#28911 which basically adds .NET 6 to the .NET Azure SDK, it resulted in compile errors due to the code in this repository. As such this change is a prerequisite of that pr.

Checklist

To ensure a quick review and merge, please ensure:

The PR has a understandable title and description explaining the why and what.
The PR is opened in draft if not ready for review yet.
- If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

Build is completely green
- Submissions with test failures require tracking issue and approval of a CODEOWNER
At least one +1 review by a CODEOWNER
All -1 reviews are confirmed resolved by the reviewer
- Override/Marking reviews stale must be discussed with CODEOWNERS first

ghost · 2022-05-22T03:49:37Z

Thank you for your contribution @AraHaan! We will review the pull request and get back to you soon.

AraHaan · 2022-05-22T03:57:25Z

cc: @annelo-msft for reviews.

src/assets/Generator.Shared/StringRequestContent.cs

AraHaan · 2022-05-22T15:30:02Z

I have made changes above to use the ReadOnlyMemory version of WriteAsync where the framework provides it for Stream when it implements .NET Standard 2.1. Since Core > 2.1 implements it and that the Azure SDK targets .NET Framework, Standard 2.0, Core 3.1, and now .NET 6 I think it should be fine after this.

annelo-msft

These are Azure.Core types, one of @christothes, @JoshLove-msft, or myself will need to approve them. @AlexanderSher, @jsquire FYI.

AraHaan · 2022-05-30T16:40:13Z

This should be ready for review, just waiting on when you all have time to review.

ghost · 2022-08-05T10:01:44Z

Hi @AraHaan. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

AraHaan · 2022-08-05T13:43:42Z

Still working on this.

AraHaan · 2022-08-16T10:54:57Z

cc: @annelo-msft any thoughts on anything else that may cause compile errors when targeting .NET 6 on the Azure SDK (before this gets merged and we find that there was some that were missed)?

ArthurMa1978 · 2022-09-30T06:34:58Z

Thank you @AraHaan for your contribution, let me ping @christothes, @AlexanderSher, @annelo-msft, @JoshLove-msft , @m-nash @jsquire .

src/AutoRest.CSharp/AutoRest.CSharp.csproj

jsquire

Thanks for your contribution, @AraHaan!

The changes look good to me. I'll defer to @m-nash for final approval.

src/AutoRest.CSharp/AutoRest.CSharp.csproj

AraHaan · 2022-09-30T13:45:44Z

I have made the requested changes.

archerzz · 2022-12-07T03:45:19Z

@AraHaan Sorry for the late response. This PR is different after a series of changes. I have a question: why await stream.WriteAsync(_bytes, 0, _bytes.Length, cancellation).ConfigureAwait(false); doesn't compile under .Net 6? I checked the API doc and it should still work under .Net 6.

Is there any benefit to use await stream.WriteAsync(_bytes.AsMemory(), cancellation).ConfigureAwait(false);? The former one seems more performant. Thanks.

AraHaan · 2022-12-07T08:43:44Z

The former still compiles for programs not set to warn as error, but it generates Visual Studio warnings about using the span versions instead of them (and if warnings as error is enabled it ends up emitting errors instead). Also, I think the span ones are a smidge faster as I think the ones without span end up calling into the span ones which could waste at least a few cpu clock cycles when using .NET 6+ (or even the .NET versions that have the span versions of them).

src/AutoRest.CSharp/AutoRest.CSharp.csproj

src/assets/Generator.Shared/FormUrlEncodedContent.cs

src/AutoRest.CSharp/AutoRest.CSharp.csproj

src/assets/Generator.Shared/FormUrlEncodedContent.cs

src/assets/Generator.Shared/StringRequestContent.cs

AraHaan · 2023-01-10T15:56:49Z

github automatically removed the other pending reviewers ^.

m-nash · 2023-01-10T21:32:02Z

@AraHaan branch is out of date with feature/v3 can you please sync to the latest so the CI can run and validate?

archerzz · 2023-01-11T08:28:30Z

/azp run autorest.csharp

azure-pipelines · 2023-01-11T08:28:40Z

Azure Pipelines successfully started running 1 pipeline(s).

AraHaan · 2023-01-11T15:03:00Z

I wish I could see why that smoketest is failing.

archerzz · 2023-01-12T01:19:19Z

I wish I could see why that smoketest is failing.

@AraHaan Normally it's due to timeout:

ECONNRESET

I've restarted the failed task.

archerzz · 2023-01-12T06:02:51Z

/azp run autorest.csharp

azure-pipelines · 2023-01-12T06:03:01Z

Azure Pipelines successfully started running 1 pipeline(s).

m-nash

Changes are fine but branch still says out of date with feature/v3 which will block merge

archerzz · 2023-01-13T01:35:09Z

/azp run autorest.csharp

azure-pipelines · 2023-01-13T01:35:19Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label May 22, 2022

ghost added the Community Contribution Community members are working on the issue label May 22, 2022

AraHaan mentioned this pull request May 22, 2022

Added explicit .NET 6 TFMs. Azure/azure-sdk-for-net#28911

Open

AraHaan commented May 22, 2022

View reviewed changes

src/assets/Generator.Shared/StringRequestContent.cs Outdated Show resolved Hide resolved

annelo-msft reviewed May 23, 2022

View reviewed changes

ghost added the no-recent-activity There has been no recent activity on this issue. label Aug 5, 2022

ghost removed the no-recent-activity There has been no recent activity on this issue. label Aug 5, 2022

ArthurMa1978 requested review from AlexanderSher, christothes and JoshLove-msft September 30, 2022 06:32

ArthurMa1978 added the Azure.Core label Sep 30, 2022

ArthurMa1978 requested review from m-nash and jsquire September 30, 2022 06:34

archerzz reviewed Sep 30, 2022

View reviewed changes

src/AutoRest.CSharp/AutoRest.CSharp.csproj Outdated Show resolved Hide resolved

jsquire reviewed Sep 30, 2022

View reviewed changes

src/AutoRest.CSharp/AutoRest.CSharp.csproj Outdated Show resolved Hide resolved

AraHaan requested review from lirenhe and chunyu3 as code owners September 30, 2022 13:42

m-nash requested changes Jan 4, 2023

View reviewed changes

src/AutoRest.CSharp/AutoRest.CSharp.csproj Outdated Show resolved Hide resolved

src/assets/Generator.Shared/FormUrlEncodedContent.cs Outdated Show resolved Hide resolved

AraHaan commented Jan 7, 2023

View reviewed changes

AraHaan added 3 commits January 10, 2023 10:54

Update src/AutoRest.CSharp/AutoRest.CSharp.csproj

0a1a5e4

Apply suggestions from code review

70f6087

Apply suggestions from code review

74d0cc2

AraHaan force-pushed the patch-1 branch from 1704806 to 74d0cc2 Compare January 10, 2023 15:54

AraHaan requested review from m-nash and removed request for christothes, AlexanderSher, lirenhe, chunyu3 and JoshLove-msft January 10, 2023 15:55

Merge branch 'feature/v3' into patch-1

4588b4b

archerzz requested a review from AlexanderSher as a code owner January 11, 2023 07:20

archerzz added 3 commits January 12, 2023 09:19

Merge branch 'feature/v3' into patch-1

547eb85

Merge branch 'feature/v3' into patch-1

6208d0c

Merge branch 'feature/v3' into patch-1

3b2f312

m-nash approved these changes Jan 12, 2023

View reviewed changes

Merge branch 'feature/v3' into patch-1

c1fdb25

archerzz enabled auto-merge (squash) January 13, 2023 01:37

archerzz merged commit 3e79c4f into Azure:feature/v3 Jan 13, 2023

AraHaan deleted the patch-1 branch January 13, 2023 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compile error when Azure SDK is targeting .NET 6. #2245

Fix compile error when Azure SDK is targeting .NET 6. #2245

AraHaan commented May 22, 2022 •

edited

Loading

ghost commented May 22, 2022

AraHaan commented May 22, 2022

AraHaan commented May 22, 2022 •

edited

Loading

annelo-msft left a comment •

edited

Loading

AraHaan commented May 30, 2022

ghost commented Aug 5, 2022

AraHaan commented Aug 5, 2022

AraHaan commented Aug 16, 2022 •

edited

Loading

ArthurMa1978 commented Sep 30, 2022

jsquire left a comment

AraHaan commented Sep 30, 2022

archerzz commented Dec 7, 2022 •

edited

Loading

AraHaan commented Dec 7, 2022 •

edited

Loading

AraHaan commented Jan 10, 2023

m-nash commented Jan 10, 2023

archerzz commented Jan 11, 2023

azure-pipelines bot commented Jan 11, 2023

AraHaan commented Jan 11, 2023

archerzz commented Jan 12, 2023 •

edited

Loading

archerzz commented Jan 12, 2023

azure-pipelines bot commented Jan 12, 2023

m-nash left a comment

archerzz commented Jan 13, 2023

azure-pipelines bot commented Jan 13, 2023

Fix compile error when Azure SDK is targeting .NET 6. #2245

Fix compile error when Azure SDK is targeting .NET 6. #2245

Conversation

AraHaan commented May 22, 2022 • edited Loading

Description

Checklist

Ready to Land?

ghost commented May 22, 2022

AraHaan commented May 22, 2022

AraHaan commented May 22, 2022 • edited Loading

annelo-msft left a comment • edited Loading

Choose a reason for hiding this comment

AraHaan commented May 30, 2022

ghost commented Aug 5, 2022

AraHaan commented Aug 5, 2022

AraHaan commented Aug 16, 2022 • edited Loading

ArthurMa1978 commented Sep 30, 2022

jsquire left a comment

Choose a reason for hiding this comment

AraHaan commented Sep 30, 2022

archerzz commented Dec 7, 2022 • edited Loading

AraHaan commented Dec 7, 2022 • edited Loading

AraHaan commented Jan 10, 2023

m-nash commented Jan 10, 2023

archerzz commented Jan 11, 2023

azure-pipelines bot commented Jan 11, 2023

AraHaan commented Jan 11, 2023

archerzz commented Jan 12, 2023 • edited Loading

archerzz commented Jan 12, 2023

azure-pipelines bot commented Jan 12, 2023

m-nash left a comment

Choose a reason for hiding this comment

archerzz commented Jan 13, 2023

azure-pipelines bot commented Jan 13, 2023

AraHaan commented May 22, 2022 •

edited

Loading

AraHaan commented May 22, 2022 •

edited

Loading

annelo-msft left a comment •

edited

Loading

AraHaan commented Aug 16, 2022 •

edited

Loading

archerzz commented Dec 7, 2022 •

edited

Loading

AraHaan commented Dec 7, 2022 •

edited

Loading

archerzz commented Jan 12, 2023 •

edited

Loading