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

Code duplication between different Unixes #14715

Closed
janhenke opened this issue Jun 14, 2015 · 6 comments
Closed

Code duplication between different Unixes #14715

janhenke opened this issue Jun 14, 2015 · 6 comments
Milestone

Comments

@janhenke
Copy link
Member

While working on FreeBSD support in CoreFX, we came across an issue that needs wider discussion, as the scope is not specific to FreeBSD.

We are facing a situation, where FreeBSD often behaves identical to Linux. E.g. they share the same crypto library (OpenSSL), they have the same file path restrictions (case sensitive, all Unicode characters allowed except NULL). At the same time they are not fully identical. E.g. FreeBSD's libc is derived from the "BSD libc", while Linux mostly uses the glibc. So we cannot alias one for the other, but still have a lot of duplicated code.

Right now the common Unix folder implements only code that is really identical across all 3 currently unix-like oses (FreeBSD, Linux and OS X).
One way for the future would maybe to move more code to the common UNIX part, if it is the same across more than one platform and then overwrite that behaviour on those platforms which differ.

I would like to use this issue to discuss this topic with all people involved, since any change would have a significant impact on the library structure and the way possible future platforms would be implemented (there is some interest in getting OpenBSD and NetBSD at some point in the future).

/cc @josteink @stephentoub @akoeplinger

@josteink
Copy link
Member

I can see why things have been solved the way they have been solved so far: With Linux and OSX as the only Unixes, they have diverged significantly enough to warrant a full split code-wise.

But with the introduction of the FreeBSD port we now have enough scenarios where the same implementation will be used for at least 2 platforms in a significant amount of corefx assemblies.

When trying to implement FreeBSD platform support I'm more often than not feeling that I'm committing copy-paste coding-violations. In my experience when you violate the DRY principle, you are effectively creating future bugs.

I agree it would be nice to have a better solution for code-reuse and I think it would be nice to not create too much technical debt before we start cleaning up :)

If the solution is use of more UnixCommon type classes or partial classes or whatever is not that a big issue for me as long as it can be applied cleanly and consistently in a way which scales better towards more platforms.

@stephentoub
Copy link
Member

In cases where most Unix-implementations implement the exact same thing but one platform is the odd-man out, I think it's fine for us to just put that file under Interop\Unix for most implementations to use and then have the other platform have its own version in its Interop\PlatformHere directory. An example here could be posix_fadvise, which is part of a POSIX standard but which OSX happens to not support.

In cases where we have different implementations of a particular piece of functionality and where platforms may pick and choose which to use, I think it's reasonable to put those files under Interop\Unix as well, naming them based on their characteristics rather than platform. For example, today we have System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Linux.cs which implements a shared backing object using shm_open and we have System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.OSX.cs which implements a shared backing object using a file on disk. We could instead name these MemoryMappedFile.SharedMemory.cs and MemoryMappedFile.Disk.cs, and then have the .csproj include them as appropriate into each platform based on the needs of that platform.

Rather than trying to do some massive refactoring in advance, I think it makes sense to just tackle individual cases as they arise, and if there are cases that don't map to one of the above cases, we can figure it out as it comes up.

cc: @ellismg

@ellismg
Copy link
Contributor

ellismg commented Jun 15, 2015

I agree with @stephentoub's comments here. One other general goal, is that I would like us to attempt to not use abstract/virtual methods in our interop code, even if it means some code duplication. It would be ideal to not have to pay the cost of virtual dispatch (or hope that the JIT can devirtualize calls) at runtime for our interop stuff.

@bartonjs
Copy link
Member

I agree with @stephentoub and @ellismg.

Aside from the perf implications (which are a sufficient argument to me), I'd be a bit worried about how complex the interop areas were getting if they required that much class hierarchy. The interop types are expected to be largely extern methods, with the occasional helper method thrown in to reduce multiple-call-site complexity (like OpenSslD2I).

That's a bit different than PAL functionality, where sometimes the universe has enough complexity that an interface helps keep things in line (like in X509Certificates' ICertificatePal). But that, as with many things, requires a judgment call.

@stephentoub
Copy link
Member

@janhenke, can we close this now? I expect things will also look differently after dotnet/corefx#2137 (cc: @nguerrera).

@janhenke
Copy link
Member Author

Well I think this is a long term problem with not single fix. dotnet/corefx#2137 looks like a good step into the right direction. I am sure though that the issue will continue to pop up.

We can close this one for now as I see no immediate action required. I think it served it's purpose in raising awareness about this topic and starting a long term transition to a more future-proof architecture.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 1.0.0-rtm milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants