Add new Queue and Stack APIs by hughbe · Pull Request #12047 · dotnet/corefx

hughbe · 2016-09-25T13:15:08Z

Fixes #4316

Also, adding APIs to .NET core was something of a soul destroying experience for me - very confusing as there are so many things to change and define and add. #11689 was helpful once I discovered it, but I ended up changing things without really understanding what the impact was.

/cc @benadams @page-not-found @weshaggard @stephentoub @ianhays

benaadams · 2016-09-25T13:28:06Z

@hughbe Thank you 👍

hughbe · 2016-09-25T14:27:46Z

I'm getting test failures of System.IO.FileNotFoundException : Could not load file or assembly 'System.Runtime.Serialization.Formatters, Version=4.0.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.
Not sure if I've broken this or the CI is acting up - tests pass locally

ianhays · 2016-09-26T17:17:09Z

@joperezr

ianhays · 2016-09-26T17:21:23Z

I'm getting test failures of System.IO.FileNotFoundException : Could not load file or assembly 'System.Runtime.Serialization.Formatters, Version=4.0.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.
Not sure if I've broken this or the CI is acting up - tests pass locally

~~I ran into this issue as well in my pr for FSW API additions: https://github.com//pull/11785/files#diff-fd8e915288e5326816b99f27c89120deR31~~

~~I just added the library to the project.json for netstandard1.7.~~

nevermind, i see now that you already have that. I'll try to repro locally.

hughbe · 2016-09-26T17:24:25Z

@ianhays It seems that this is already added to the project.. https://github.com/hughbe/corefx/blob/c815b7677764864a57a4cae3939fd6cf56ac5e04/src/System.Collections/tests/project.json#L28

stephentoub · 2016-09-28T04:44:09Z

The implementations of the new members LGTM.

hughbe · 2016-09-28T12:54:12Z

Thanks. @ianhays i ran the tests locally (msbuild /T:BuildAndTest /P:TargetGroup=netstandard1.7) and I got these errors), so I can reproduce the CI errors when specifying the target group.
Unfortunately I don't understand the error message as the library is already referenced!

ianhays · 2016-09-28T16:40:03Z

Also, adding APIs to .NET core was something of a soul destroying experience for me - very confusing as there are so many things to change and define and add.

Sorry about your trouble with the API addition Hugh. It's a pretty complicated process right now with the update to netstandard1.7, particularly since some libraries have been updated while others are still building netstandard1.3. It's still very much in flux, so I don't think we have any docs for it (at least I'm not aware of any).

System.Collections is one that has already been updated to netstandard1.7, so you shouldn't need to do any of the structural changes around versioning from this PR.

#11689 was helpful once I discovered it, but I ended up changing things without really understanding what the impact was.

Unfortunately #11689 is a special case where Jeremy wanted some components to be exposed only for .NET Core (netcoreapp1.1) and not UAP (uap10.1) or net463. For most cases when adding API, we want to add those for all platforms that are adhere to netstandard1.7 (the previously mentioned), not just a particular one. For System.Collections specifically, there aren't any components that are runtime specific so the src build should be built for netstandard1.7. If you have any more in-depth questions then @joperezr can probably explain the TargetGroup/TestTFM structure better than I can since he's the one who explained it to me yesterday.

nevermind, i see now that you already have that. I'll try to repro locally.

I pulled your changes, removed the netcoreapp1.1 structural stuff, and pushed a working copy to my repo that you can use as an example of what to do when a project has already been updated to build for netstandard1.7. I'd recommend you build the test builds file (msbuild System.Collections.Tests.builds) without args to test your changes, as that is how the CI will be testing them and it ensures you cover all of the configs.

weshaggard · 2016-09-28T17:07:26Z

-    }
+    },
+    "netcoreapp1.1": {},
+    "netcoreapp1.0": {}


Why did you need to add netcoreapp1.0?

I saw it added in #11689 so replicated it

You can remove the netcoreapp1.0 framework as you aren't building this project for that target framework.

Once you add the build configuration I suggested you should be able to test the build and run the tests for netcoreapp1.1. I suspect you will need to add:

"System.Runtime.Serialization.Formatters": "4.3.0-beta-24522-03"

Under your netcoreapp1.1 section here as well, in order to get that dependency passed for the netcoreapp1.1 build.

weshaggard · 2016-09-28T17:08:08Z

            Assert.Equal(itemToAdd, q.Dequeue());
        }

+#if netcoreapp11


My preference would be to have these netcoreapp11 specific tests in their own file instead of using #ifdefs.

weshaggard · 2016-09-28T17:09:32Z

cc @ericstj

weshaggard · 2016-09-28T17:10:57Z

@@ -10,6 +10,7 @@
    <RootNamespace>System.Collections.Tests</RootNamespace>
    <DefineConstants Condition="'$(TargetGroup)'=='netstandard1.7'">$(DefineConstants);netstandard17</DefineConstants>


You are going to need to add a build configuration in the System.Collections.Tests.builds file in order to get this netcoreapp1.1 build setup.

Something along the lines of:

<Project Include="System.Collections.Tests.csproj"> <TargetGroup>netcoreapp1.1</TargetGroup> <TestTFMs>netcoreapp1.1</TestTFMs> </Project>

Then to build and run the tests you either needs to msbuild System.Collections.Tests.csproj /p:TargetGroup=netcoreapp1.1

hughbe · 2016-09-28T17:11:51Z

@ianhays Thanks!
Unfortunately, I think I need to add these new APIs to netcoreapp, not netstandard1.7, as this API is currently .NET Core only?

@weshaggard could you clarify if we can just expose these in netstandard1.7? thanks!

weshaggard · 2016-09-28T17:15:39Z

Unfortunately, I think I need to add these new APIs to netcoreapp, not netstandard1.7, as this API is currently .NET Core only?

That is correct these should be netcoreapp1.1, specific APIs.

hughbe · 2016-09-28T21:01:31Z

Okay! It all works guys, thanks for your help!

hughbe · 2016-09-28T21:01:48Z

Now the CentOs build must work too:
Test Innerloop CentOS7.1 Debug Build and Test
Test Innerloop CentOS7.1 Release Build and Test

weshaggard · 2016-09-28T23:18:16Z

    <RootNamespace>System.Collections.Tests</RootNamespace>
    <DefineConstants Condition="'$(TargetGroup)'=='netstandard1.7'">$(DefineConstants);netstandard17</DefineConstants>
    <NugetTargetMoniker Condition="'$(NugetTargetMoniker)'==''">.NETStandard,Version=v1.3</NugetTargetMoniker>
+    <DefineConstants Condition="'$(TargetGroup)' == 'netcoreapp1.1'">$(DefineConstants);netcoreapp11</DefineConstants>


This define is no longer needed.

Good spot, thanks

weshaggard

one last little clean-up otherwise this change LGTM. Thanks for the contribution and working through the work of adding new APIs.

weshaggard · 2016-09-30T05:11:42Z

Thanks for the contribution @hughbe.

Does not exist in older .NET Standard versions See: dotnet/runtime#15619 => dotnet/corefx#12047

Implement TryPeek and TryPop on Stack Implement TryPeek and TryDequeue on Queue Commit migrated from dotnet/corefx@de966f9

dnfclas added the cla-already-signed label Sep 25, 2016

karelz added area-System.Collections enhancement Product code improvement that does NOT require public API changes/additions labels Sep 26, 2016

weshaggard reviewed Sep 28, 2016

View reviewed changes

hughbe added 2 commits September 28, 2016 18:26

Implement new Queue and Stack methods

0d0ece8

Fix depencies

8cfe159

weshaggard reviewed Sep 28, 2016

View reviewed changes

weshaggard approved these changes Sep 28, 2016

View reviewed changes

Address PR feedback

c4fb7ec

weshaggard merged commit de966f9 into dotnet:master Sep 30, 2016

hughbe deleted the queue-stack-try branch October 3, 2016 16:58

karelz modified the milestone: 1.2.0 Dec 3, 2016

hihain added a commit to hihain/UnigramMobile that referenced this pull request Dec 6, 2021

Remove Queue<T>.TryDequeue(T)

60d699c

Does not exist in older .NET Standard versions See: dotnet/runtime#15619 => dotnet/corefx#12047

hihain added a commit to hihain/UnigramMobile that referenced this pull request Dec 7, 2021

Remove Queue<T>.TryDequeue(T)

fbc60f4

Does not exist in older .NET Standard versions See: dotnet/runtime#15619 => dotnet/corefx#12047

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Add new Queue and Stack APIs (dotnet/corefx#12047)

3b11f63

Implement TryPeek and TryPop on Stack Implement TryPeek and TryDequeue on Queue Commit migrated from dotnet/corefx@de966f9

TheRexYo mentioned this pull request Aug 31, 2023

[Help] Build for the main branch fails due to usage of .NET SDK 5.0+ methods HashSet<T>.AddIfNotPresent and Queue<T>.TryDequeue. ccgould/FCStudios_SubnauticaMods#28

Open

		@@ -10,6 +10,7 @@
		<RootNamespace>System.Collections.Tests</RootNamespace>
		<DefineConstants Condition="'$(TargetGroup)'=='netstandard1.7'">$(DefineConstants);netstandard17</DefineConstants>

Conversation

hughbe commented Sep 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Sep 25, 2016

Uh oh!

hughbe commented Sep 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianhays commented Sep 26, 2016

Uh oh!

ianhays commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hughbe commented Sep 26, 2016

Uh oh!

stephentoub commented Sep 28, 2016

Uh oh!

hughbe commented Sep 28, 2016

Uh oh!

ianhays commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hughbe commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weshaggard commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hughbe commented Sep 28, 2016

Uh oh!

hughbe commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard left a comment

Choose a reason for hiding this comment

Uh oh!

weshaggard commented Sep 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hughbe commented Sep 25, 2016 •

edited

Loading

hughbe commented Sep 25, 2016 •

edited

Loading

ianhays commented Sep 26, 2016 •

edited

Loading

hughbe commented Sep 28, 2016 •

edited

Loading

weshaggard commented Sep 28, 2016 •

edited

Loading