-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[WIP] Initial commit of libcorefx_io with stat shims #1225
Conversation
The PR for System.Security.Native #745 has a lot of discussion on this topic. It would be really nice to be consistent at least - e.g. if you want to change the naming convention, change the existing libraries to follow it too. |
Is there any reason we're putting all of the native code for corefx into the coreclr itself? Isn't the point of corefx to have self contained libraries? Is it because coreclr is already setup with cmake to build native code? |
@jkotas Whatever we decide, I will certainly make it consistent. My goal in sending this out early was to gather this sort of feedback before changing too much code. To join the earlier naming discussion:
|
I've been in favor of the System.Whatever.Native naming, but I don't feel particularly strong about it, so if there are good reasons to go another direction, fine, as long as we're consistent.
Have we thought through how that's going to work when a) everything is deployed app-local, b) we need to version the native surface along with the managed, and c) we support mixing and matching versions of different libraries? |
static void ConvertFileStats(const struct stat_& src, FileStats* dst) | ||
{ | ||
dst->Flags = FILESTATS_FLAGS_NONE; | ||
dst->Mode = src.st_mode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any systems that use different values for the named flags that make up the st_mode? I seem to remember @ellismg verifying "no", at least for the systems we were currently targeting, but I don't know that that's always going to be true, in which case we'd need to do conversions for fields like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should declare these constants here even if they are fixed in practice. It's good discipline to follow the pattern we definitely need elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
I have started to think about it. As I hinted above, I'd like to see these packaged separately from their managed callers. And the managed nuget packages can manifest their runtime dependencies on these packages. I would further say that just as with the managed packages, we will avoid breaking changes such that standard nuget resolution picking one that satisfies all dependencies will allow a given app to share one copy. However, we have a nice out vs. the managed case: we can simply encode a new version in the package and .so file name if any breaking is needed and they can live side-by-side in the same app-local deployment when necessary. Unlike in the managed ecosystem case, there's no fallout to the names of these implementation details changing. :) The alternative (unless I'm missing something) is every managed lib carries its own native shim, but that will create lots of duplication of shim code that further needs to be built for every unix flavor. I don't yet have the numbers on (# of libs that need shims x # of shim flavors) but my intuition is that it will get quite messy if we go that way. Also, I opened dotnet/corefx#2302 yesterday to track our packaging decisions in a hope to decouple the work to get to a safe place from the requirements of distributing this native code. I think that's OK because it should be straight forward to split or merge the source code, rename things, etc. We can evolve our thinking on granularity, while making parallel progress on writing and using shim source code. |
Closing this as I will replace it with code building in corefx. |
WIP
Using this as a sample of my current thinking on dotnet/corefx#2137 and to get early feedback.
Some notes on departures of from earlier offline discussion:
Library names
I dislike the .Native.so pattern without lib prefix. It just feels out of place in native code. I propose libcorefx_io, libcorefx_crypto, etc. instead. Also, depending on where we land for how these are distributed/versioned, etc. we might opt to merge them all together or split along different, fewer lines...
API names
I propose using our own PascalCased and spelled out names, and not names that are 1:1 with the native API they wrap. My reasoning is that we can't always be 1:1...
Even in this simple stat example:
And looking at crypto:
- being 1:1 with libcrypto would be completely impractical with all of that pointer indirection. We can't copy whole graphs to a stable layout and then only use a small part of it!
So, in the end, I think giving these libraries their own style in which to express themselves will make them more self-consistent and easier to evolve. The guideline is still to keep them as "thin" as possible/practical.