Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[WIP] Initial commit of libcorefx_io with stat shims #1225

Closed
wants to merge 1 commit into from

Conversation

nguerrera
Copy link

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:

  • we have to introduce something unique (Flags).
    • we leave some things out (not worth wasting dev cycles deciding how to surface unused types with stable ABI or CPU cycles copying them).

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.

@nguerrera
Copy link
Author

cc @stephentoub @ellismg @jkotas

@jkotas
Copy link
Member

jkotas commented Jul 10, 2015

I dislike the .Native.so pattern without lib prefix

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.

@davidfowl
Copy link
Member

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?

@akoeplinger
Copy link
Member

@nguerrera
Copy link
Author

@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:

  • We're going to want to share these across several managed libraries, so it's not really as simple as <managed name>.Native.dll. I want to avoid creating too many of these just to line them up with the managed packages.
  • I see them being referenced as runtime dependencies and not included in the same package as the managed caller. We foresee needing to build these for each flavor (including different Linux distributions!), and I don't think it's scalable to use "fat" packages in that world.
  • I'm actually less worried about 3rd party usage than I am about our own versioning issues when they're consumed by managed packages that version independently. We can simply encode a version the file name to address both. I do want to be free to refactor these as necessary.
  • On Windows, there's some precedent with clrcompression.dll that we already ship on nuget (cc @ericstj). It would be nice if we can have a consistent scheme (modulo lib prefix, extension) across operating systems. However, I'm not sure the clr prefix is the right choice so maybe it's more about making Windows compression consistent with what we have here than vice versa.

@stephentoub
Copy link
Member

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.

We're going to want to share these across several managed libraries

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;
Copy link
Member

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.

Copy link
Author

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.

Copy link

Choose a reason for hiding this comment

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

I agree.

Copy link
Member

Choose a reason for hiding this comment

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

Yup.

@nguerrera
Copy link
Author

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?

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.

@nguerrera
Copy link
Author

Closing this as I will replace it with code building in corefx.

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

Successfully merging this pull request may close these issues.

8 participants