Add string overloads to DbDataReader.Get*()#35211
Add string overloads to DbDataReader.Get*()#35211ajcvickers merged 1 commit intodotnet:masterfrom TheBlueSky:dbdatareader-string-overloads
Conversation
src/System.Data.Common/tests/System/Data/Common/DbDataReaderTest.cs
Outdated
Show resolved
Hide resolved
|
@roji, any hint regarding the failing checks? :| |
|
I think you can ignore the non-ci builds, those are being replaced by the CI versions. try doing |
|
Yeah, so those error in the logs, but couldn't figure out why. It looks like the rest class couldn't see the changes in Would like a hint on where to start looking. |
|
Does it repro locally for the failing target? So if you specify the netfx target do you get a clean build locally? |
|
And then the errors that are shown in the CI builds; i.e., However, when I try to build without specifying the framework or build individual frameworks, the build succeeds; e.g., |
|
Have you done |
|
I ran I followed this document (https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md) to get started. I'm not sure I missed something. Also, if the build fails in CI pipeline, I'm not sure it's related to dependencies. For some reason, certain configurations cannot see the changes I did to |
|
It's not just you, I've replicated the problem and I know my setup works. Started from clean ( What I can't see is how the tests are supposed to be resolving the main library, there's no reference in the project. If it's using nuget then it won't work until the package is rebuilt and I've no good way to do that (when required I edited the nuget package but obviously that won't work in the ci). So, bat signal time @safern to the rescue! hopefully... |
|
Lets see if the area owners can help, /cc @divega, @ajcvickers, @afsanehr, @David-Engel, @tarikulsabbir |
|
@karelz Any idea of can help with this? Are checks working normally at the moment? When I click on the Details for the failed checks I get 404s. |
|
The thing is that some of the builds doesn’t exist anymore, they where deleted. So we need to close and reopen the PR. |
|
Also it would be great if this PR could be rebased on top of master and push a new commit so that the CI system reboots and the latest changes take effect. Once I can see the failures I can help to reproduce and find the issue. |
|
@roji, can you have a look? |
|
Does i.e. do they need to be netcoreapp only? |
|
It looks like @benaadams might have found the answer. All corefx assemblies have an inbox attribute applied through metadata The fix is to make the ref class partial and use a second file called I could be wrong but it's worth a try I'd say, the only other option is to keep waiting. |
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Sorry for not reviewing earlier, I'll be looking at this more closely now.
@TheBlueSky adding to @bgrainger, see some comments from me. Please try @benaadams's and @Wraith2's suggestion to make the build pass, if not we'll involve someone from the team to unblock this.
src/System.Data.Common/tests/System/Data/Common/DbDataReaderMock.cs
Outdated
Show resolved
Hide resolved
@Wraith2, What about |
|
There's a similar msbuild condition for UAP, if you look in the SqlClient project I know it'll be in there somewhere. It'll just be a case of adding the second condition to prevent the new methods being compiled in. |
|
@Wraith2, excuse my ignorance here. If we want to compile these for |
|
yes, same structure for src and ref, ref just contains the null return/throwing versions as you've already done, src contains functioning implementations. |
Implement #31595
|
@TheBlueSky thanks for the quick fixes - to me this looks ready. The remaining test fix seems related to a Helix infrastructure issue (unrelated to your PR). @terrajobst @divega what's the next step for this? |
|
@roji, thanks for reviewing this PR; I learnt couple of things along the way. The next step should be someone to tell me why the macOS build times out, and whether this is a big deal or not :) |
|
The mac build failed on my PR yesterday as well, i'm assuming it's a build server problem and someone in engineering will poke it when they get into the office on US time. Given that you're not doing anything platform specific and the linux builds are working i'd probably ignore it. |
|
As @Wraith2 wrote, I wouldn't worry about the MacOS build. There's some flakiness in the build infrastructure sometimes. |
|
@dotnet-bot test OSX x64 Debug Build please |
This comment no longer works. I was going to retry the failing leg manually, but accidentally clicked the wrong button and ran all the legs, sorry about that 😢 |
Well, it worked for me, though not directly |
|
Linux failed this time. How do you work out what went wrong? all I see is the send to helix step failed but that doesn't mean anything or provide any point to debug from. |
|
In the logs you can see the link to test results and a summary of what failed. It seems like infrastructure stuff, trying to download the .zip folder with the test binaries: |
|
Thank you @TheBlueSky for the contribution and @Wraith2, @safern for all the help getting it in. @roji is this the first of all the ADO.NET improvements you filed to get fixed? 😄 |
|
Yay! :) Thanks for everyone who helped in this, especially @Wraith2. |
Yeah, I think it is, after all these years :) A sign of more PRs to come :) |
9 participants
Implement #31595