Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add TaskAsyncEnumerableExtensions to Microsoft.Bcl.AsyncInterfaces#37379

Merged
joperezr merged 4 commits intodotnet:masterfrom
stephentoub:addextensionstopackage
May 9, 2019
Merged

Add TaskAsyncEnumerableExtensions to Microsoft.Bcl.AsyncInterfaces#37379
joperezr merged 4 commits intodotnet:masterfrom
stephentoub:addextensionstopackage

Conversation

@stephentoub
Copy link
Copy Markdown
Member

Adds the remaining types needed for the Microsoft.Bcl.AsyncInterfaces package
Blocked on #37367
Contributes to #37354
cc: @terrajobst, @joperezr

@stephentoub stephentoub force-pushed the addextensionstopackage branch from 98b3be6 to 52ca08f Compare May 4, 2019 00:31
Copy link
Copy Markdown
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Changes here LGTM

@stephentoub stephentoub force-pushed the addextensionstopackage branch from 52ca08f to e7a166b Compare May 7, 2019 17:21
@stephentoub
Copy link
Copy Markdown
Member Author

@joperezr, it looks like this is configured to build against netstandard and netstandard2.1. netstandard2.1 doesn't yet contain these APIs, so the ref TypeForwardedTo is failing to compile. What's the right way for me to deal with this now? Should I hold off until @terrajobst adds these for netstandard2.1, and then presumably we'd pull in those updated refs and things would just start working?

@joperezr
Copy link
Copy Markdown
Member

joperezr commented May 7, 2019

I see, in this case for the ref, we can do two different approaches. We can either have that TypeForwardedTo in the Forwards.cs file moved to a different file that gets includded when not building for netstandard2.1, or an easier way would be to simply set the Ignore missing types property on the project when building for netstandard2.1 like:

<PropertyGroup Condition="'$(TargetGroup)' == 'netstandard2.1'">
  <GenFacadesIgnoreMissingTypes>true</GenFacadesIgnoreMissingTypes>
</PropertyGroup>

@stephentoub
Copy link
Copy Markdown
Member Author

GenFacadesIgnoreMissingTypes

If we do that, what's the functioning state of things? If you try to use this package when building against .NET Core 3.0 or .NET Standard 2.1, what will happen?

@joperezr
Copy link
Copy Markdown
Member

joperezr commented May 7, 2019

If you are targetting .NET Standard 2.1 (or a compatible framework like .NET Core3.0 or UWP vNext) there is no problem, since you wouldn't be referencing those types from this contract anyway. The only problem with this, would be in the case where somebody compiled against .NET Standard 2.0 (or a compatible assembly) which would mean that they would look for the type in the M.Bcl.AI.dll but then try to run in a .NET Standard 2.1 compatible framework (other than .NET Core 3.0 since we do have the typeforward in that case) since what would happen is that the app wouldn't be able to properly find the re-route of this type to where it lives in that framework. I think that is an ok risk to take, if and only if we believe that this type will eventually (soon) be added to .NET Standard2.1, as when we eventually do it we would just remove the condition and everything will be fixed. If we are not thinking of having this type inbox in .NET Standard 2.1, then we should provide a typedef for .NET Standard2.1 for this type and a .NET STandard2.1 implementation.

@stephentoub stephentoub force-pushed the addextensionstopackage branch from e7a166b to 9545cf7 Compare May 8, 2019 20:45
@joperezr joperezr force-pushed the addextensionstopackage branch from 27057ff to be6afe8 Compare May 8, 2019 21:23
@joperezr
Copy link
Copy Markdown
Member

joperezr commented May 8, 2019

@stephentoub failures in the PR seem to be due to System.Data.OleDb.Tests tests failing (hanging) on some architectures. From the changes you are making, I can't see how that would be related to your PR, so in that case is it ok to go ahead and merge this or is there a reason why you might think that failures are related?

@danmoseley
Copy link
Copy Markdown
Member

@maryamariyan the OLEDB tests are timing out, after several failures like

  System.Data.OleDb.Tests.OleDbCommandBuilderTests.DeriveParameters_NullCommand_Throws(commandType: Text) [FAIL]
      System.Runtime.InteropServices.SEHException : External component has thrown an exception.
      Stack Trace:
        D:\a\1\s\src\System.Data.OleDb\src\OleDbWrapper.cs(145,0): at System.Data.OleDb.DataSourceWrapper.InitializeAndCreateSession(OleDbConnectionString constr, SessionWrapper& sessionWrapper)
        D:\a\1\s\src\System.Data.OleDb\src\OleDbConnectionInternal.cs(94,0): at System.Data.OleDb.OleDbConnectionInternal..ctor(OleDbConnectionString constr, OleDbConnection connection)

@joperezr I have confident this should not stop you merging.

@stephentoub
Copy link
Copy Markdown
Member Author

Thanks, Jose. Yeah, not related at all.

netstandard;
<!-- netstandard; -->
netcoreapp;
uap;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This kind of defeats the purpose of these tests, right? :)

Should we instead just exclude ConfiguredCancelableAsyncEnumerableTests.netcoreapp.cs from being included until netstandard2.1 is updated?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It does defeat the purpose, but it is a temporary change, so I'm fine with either removing the tests or leaving this as is and fixing once netstandard contains the type. As you prefer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks. I prefer the excluding-these-few-tests approach.

@maryamariyan
Copy link
Copy Markdown

@maryamariyan the OLEDB tests are timing out, after several failures like

Created https://github.com/dotnet/corefx/issues/37538
I am trying to debug the failures. In the meantime I skipped all OleDb tests first to stop disrupting other PRs with this failure.

@joperezr
Copy link
Copy Markdown
Member

joperezr commented May 9, 2019

Networking test failures are not related to this PR either so I'll go ahead and merge this. Thanks @stephentoub!

@joperezr joperezr merged commit 2392cae into dotnet:master May 9, 2019
@karelz karelz added this to the 3.0 milestone May 22, 2019
@stephentoub stephentoub deleted the addextensionstopackage branch June 12, 2019 19:58
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…otnet/corefx#37379)

* Add TaskAsyncEnumerableExtensions to Microsoft.Bcl.AsyncInterfaces

* Adding baseline entry for the missing type on netstandard

* Temporarily changing test configurations of AsyncInterfaces package so that all types are found.

* Comment out AsyncEnumerable tests while the type makes its way to netstandard 2.1


Commit migrated from dotnet/corefx@2392cae
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants