-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add FreeBSD support to System.IO.Compression.ZipFile #2041
Add FreeBSD support to System.IO.Compression.ZipFile #2041
Conversation
I'm a bit weary about the code-duplication going on and I think we should have a future plan for getting some of the code which is common across Unixes merged into common files. However I see the way things are done here adheres to the conventions and practices already in place in corefx for cross-platform support. As such this LGTM. |
I agree it does not seem a good path to add more platforms in the future. For now it seems the way to go, as it is the same structure across the different platforms. The problem is that OS X is different from the other platforms. I still think this should be merged as it is. We need to discuss that issue in a wider range and implement a consistent solution later. |
I think the PathInternal files should be refactored slightly... Today we have:
I suggest we change it to this:
Then we just include the right files in the right parts of the projects:
I think that makes things cleaner and avoids unnecessary code duplication. (This whole PathInternal.GetComparison thing will potentially change in the future if/when we're able to detect the right casing: #1086). |
If folks agree with that suggestion, I've submitted a PR (#2045) to refactor it accordingly. This PR could then be updated to use it once it's merged and avoid duplicating the files. |
Originally I did not want to touch other code, but the path looks good. I am absolutely in favour of going ahead with it like you outlined. |
I'm perfectly fine with it. Go ahead :) |
69d63a2
to
0d78900
Compare
With the re-factored code base the patch became much simpler. |
LGTM |
This is much better. LGTM. |
Add FreeBSD support to System.IO.Compression.ZipFile
…sion.ZipFile Add FreeBSD support to System.IO.Compression.ZipFile Commit migrated from dotnet/corefx@8d2f914
Adding support for FreeBSD to System.IO.Compression.ZipFile. PathInternal is identical to Linux, as they share the same file system requirements (paths are case sensitive and can contain all Unicode characters except NULL).
/cc @josteink @stephentoub