Skip to content
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

Use Marshal.GetLastPInvokeError instead of Marshal.GetLastWin32Error in NETCoreApp #51648

Closed
elinor-fung opened this issue Apr 21, 2021 · 12 comments · Fixed by #52003 or #52212
Closed
Assignees
Labels
area-System.Runtime.InteropServices help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@elinor-fung
Copy link
Member

.NET 6 introduces Marshal.GetLastPInvokeError that has the same functionality as Marshal.GetLastWin32Error, but is named to be representative of what it does: #46843

Usage in NETCoreApp should be updated to use GetLastPInvokeError instead of GetLastWin32Error. Note that not all usage in libraries can be updated, since some libraries compile for down-level targets.

@elinor-fung elinor-fung added area-System.Runtime.InteropServices help wanted [up-for-grabs] Good issue for external contributors labels Apr 21, 2021
@elinor-fung elinor-fung added this to the 6.0.0 milestone Apr 21, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 21, 2021
@elinor-fung elinor-fung removed the untriaged New issue has not been triaged by the area owner label Apr 21, 2021
@KZedan
Copy link
Contributor

KZedan commented Apr 24, 2021

Hey, interested in contributing. How do I get this assigned to me? :D

@elinor-fung
Copy link
Member Author

Hi @KZedan - I have assigned the issue to you. Thank you!
Please let us know if you have questions by commenting in this issue.

cc @AaronRobinsonMSFT @jkoritzinsky

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 28, 2021
@KZedan
Copy link
Contributor

KZedan commented Apr 29, 2021

@elinor-fung I have linked the PR to this issue however there seem to be some failing builds in the CI but I have noticed the same issue happening in different PRs where the same build is failing. I am assuming this is a known issue with the builds? I have run the tests and build locally and they all succeeded.

As you mentioned there were invocations of the method that I had to revert back to Marshal.GetLastWin32Error as they were causing build errors.

Let me know if this covers it.

Thanks!

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 29, 2021
@jkotas jkotas reopened this Apr 29, 2021
@jkotas
Copy link
Member

jkotas commented Apr 29, 2021

#52003 converted some of the occurences. I believe that there are more than can be converted.

@KZedan
Copy link
Contributor

KZedan commented Apr 30, 2021

@jkotas I will take another look.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 3, 2021
@KZedan
Copy link
Contributor

KZedan commented May 4, 2021

@jkotas I've managed to get the remaining done that were referenced in system.private.corelib. There were some conflicts due to different framework versions of the package being used simultaneously. I followed the readme and added the ref for System.Runtime.Interop directly into the relative slns.

One thing that I am confused about currently is after making this change, the Interop.Crypt32.CryptProtectData call is now complaining about a compilation error as the string szDataDescr param is not nullable when it's being passed as null in ProtectedData.cs. This is in the System.Security.Cryptography.ProtectedData library. It seems that before it was being passed as such but this compilation error must have been suppressed somehow.

Any ideas what might be causing this?

I've attached the PR as a reference with the parameter being tweaked to become nullable but I am under the impression that it is not supposed to so.

image
image

@jkotas
Copy link
Member

jkotas commented May 4, 2021

Any ideas what might be causing this?

I think it may be because of you have added $(NetCoreAppCurrent) target.

I am not sure that it is worth it to be adding ifdefs to files that are used in multiple places. I think it would be better to use the existing API without an ifdef.

Have you looked at the places outside corelib?

@KZedan
Copy link
Contributor

KZedan commented May 4, 2021

Have you looked at the places outside corelib?

Nope, I was under the impression that corelib had everything contained in its sln library-wise (that's what the readme said), sounds like that's not the case though, where else should I be looking?

@KZedan
Copy link
Contributor

KZedan commented May 4, 2021

Ah nevermind, I just found a bunch in externals.csproj

@jkotas
Copy link
Member

jkotas commented May 4, 2021

Any Marshal.GetLastWin32Error under src/libraries in this repo is a candidate, as long as it does not require ifdefs to update.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 9, 2021
@elinor-fung
Copy link
Member Author

Thank you, @KZedan!

@KZedan
Copy link
Contributor

KZedan commented May 9, 2021

No problem!

@ghost ghost locked as resolved and limited conversation to collaborators Jun 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants