Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[WIP] Add NativeLibrary reference assemblies and tests #27258

Closed

Conversation

GrabYourPitchforks
Copy link
Member

This is the corefx equivalent of dotnet/coreclr#16409.

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Feb 19, 2018

Yes, I know submitting binary files is horrible. The reason it's proposed this way is that it allows the build to succeed in all environments (including from the IDE). I've tweaked the .vcxproj files to ensure that they're producing the smallest possible outputs.

GitHub also seems to think the .c / .h files are binary so won't show them properly in the diff window. Hitting 'View' on each file will show them correctly. I'll look into this and try to address it in the near future.

@GrabYourPitchforks
Copy link
Member Author

/cc @ahsonkhan @bartonjs

Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

  • Consider adding an update script, a la the FIleVersionInfo tests
  • nativelibrarywin.c (and the .h files) is/are appearing here as binary, please try to rectify that.
  • Please add argument validation tests
    • name != null
    • times when caller is required
    • Values (mapped or unmapped) for search paths which are not allowed
    • etc
  • Please add tests for GetSymbolAddress returning something other than a function pointer
  • Anything we can assert about the Handle value (!= IntPtr.Zero, that using it you can use GetProcAddress to get the same answer as TryGetSymbolAddress, etc)

public static TDelegate GetDelegate<TDelegate>(this NativeLibrary nativeLibrary, string name, bool exactSpelling = false) where TDelegate : class
{
nativeLibrary.TryGetDelegate<TDelegate>(name, exactSpelling, out var retVal);
return retVal;
Copy link
Member

Choose a reason for hiding this comment

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

Consider having this test-only extension assert that the nullarity of retVal and the return value are consistent.

@jkotas
Copy link
Member

jkotas commented Feb 23, 2018

I do not think we should be checking in binaries. If CoreFX does not make it easy to add native binaries for testing, we should have tests for this in CoreCLR instead. CoreCLR has infrastucture for native test binaries.

@jkotas
Copy link
Member

jkotas commented Feb 23, 2018

All interesting interop tests are in CoreCLR today for this and other reasons.

@karelz
Copy link
Member

karelz commented Mar 12, 2018

@GrabYourPitchforks what is status of this PR? Do you plan to finish it for 2.1?
If not, we should close it, until the branch reopens for post-2.1 work.

@GrabYourPitchforks
Copy link
Member Author

Couldn't get consensus on feature before deadline, so punted to Future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants