Conversation
|
@michael-yuji Has that issue been reported on Foundation? Not having the tests build is definitely going to be hiding bugs. |
|
In the meantime, as Foundation is unlikely to fix the issue in 6.2, can I suggest we skip the test until 6.3 on FreeBSD? |
| } | ||
|
|
||
| #if canImport(Glibc) || canImport(Musl) || canImport(Bionic) | ||
| #if !os(FreeBSD) && (canImport(Glibc) || canImport(Musl) || canImport(Bionic)) |
There was a problem hiding this comment.
Under what circumstance would this check have previously compiled? Does FreeBSD report its libc as Glibc?
I see from the rest of the code that it does. Is there any way we can change that before formal support? It's going to make life very difficult, as well as being factually untrue.
There was a problem hiding this comment.
Unfortunately FreeBSD does report its libc as Glibc, I do have a PR open (swiftlang/swift#79261) but we have decided that that's no bandwidth to make the overlay change into 6.2.x
cc @etcwilde
| // Okay, return empty value. | ||
| return .success([]) | ||
| #endif | ||
| #if !os(FreeBSD) |
There was a problem hiding this comment.
Is this here because noData is impossible or because it's unreachable?
There was a problem hiding this comment.
it's because .nodata is not available on this platform
| #endif | ||
| default: | ||
| #if os(FreeBSD) | ||
| let error = FileSystemError.extattr_get_fd( |
There was a problem hiding this comment.
Does this behave meaningfully differently than fgetxattr? If not, can this #if os be moved into that function so it doesn't end up here?
There was a problem hiding this comment.
fgetxattr is not available, the extended attribute apis are called differently
There was a problem hiding this comment.
If it only has a different name, but otherwise behaves the same, please merge them into the same function.
| } | ||
|
|
||
| #elseif canImport(Glibc) || canImport(Musl) || canImport(Bionic) | ||
| #elseif os(FreeBSD) || canImport(Glibc) || canImport(Musl) || canImport(Bionic) |
There was a problem hiding this comment.
canImport(Glibc) covers os(FreeBSD) currently.
| #if os(FreeBSD) | ||
| self.fflags = 0 | ||
| self.data = 0 | ||
| #else |
There was a problem hiding this comment.
It's not clear to me that this is right: as noted above, EVFILT_EXCEPT does appear to be available on FreeBSD, and I think we actually do want it.
| @inline(never) | ||
| public static func opendir(pathname: String) throws -> OpaquePointer { | ||
| #if os(FreeBSD) | ||
| sysOpendir(pathname)! |
There was a problem hiding this comment.
Please wrap this in the syscall helper.
| var nonThrowingErrnos: [Errno] = [] | ||
| var knownErrnos: [Errno: FileSystemError.Code] = [.notSupported: .unsupported] | ||
| #if canImport(Darwin) | ||
| #if canImport(Darwin) || os(FreeBSD) |
There was a problem hiding this comment.
This represents a behavioural change on macOS, which no longer has noData in its nonThrowingErrnos.
| // These platforms require a dependency on `NIOPosix` from `NIOHTTP1` to maintain backward | ||
| // compatibility with previous NIO versions. | ||
| let historicalNIOPosixDependencyRequired: [Platform] = [.macOS, .iOS, .tvOS, .watchOS, .linux, .android] | ||
| let historicalNIOPosixDependencyRequired: [Platform] = [.macOS, .iOS, .tvOS, .watchOS, .linux, .android, .custom("freebsd"), .custom("FreeBSD")] |
There was a problem hiding this comment.
These platforms do not require a historical NIOPosix dependency, as they did not compile before.
I don't think it's a bug in Foundation, the initializer seems to come from skipping the test now sounds good to me |
|
Let's also not merge this until swiftlang/swift#79261 lands, because the compiler guards will all change at that point. |
I'm not sure that's a good idea, since we have announced support for FreeBSD and without swift-nio, anything swift on server would pretty much become useless on the platform which is less than ideal |
|
Let me be more precise: if we land the change as-is, you must not change the name of the libc in FreeBSD, as doing so will be source-breaking. So, to put not too fine a point on it: if the above PR doesn't land now, it doesn't land until a source break is acceptable. |
| } | ||
| #endif | ||
|
|
||
| #if !os(FreeBSD) && (canImport(Glibc) || canImport(Musl) || canImport(Android)) |
There was a problem hiding this comment.
Would it be stylistically better to have
#if !os(FreeBSD)
#if canImport(Glibc) ...
#endif
#endif
or retain the rather complex boolean expression here?
I ran into this when considering OpenBSD support; the stacked conditionals are easier to read but I don't know if there's a stylistic preference.
There was a problem hiding this comment.
I erred on calling this just CNIOBSD in #3394. Would it be better to do the same here, so that we don't need to have one CNIO.* module per operating system? Or should I go and rename CNIOBSD to CNIOOpenBSD there?
There was a problem hiding this comment.
Do the two have similar-enough libcs?
There was a problem hiding this comment.
There are quite similar, especially due to pedigree, but there is definitely divergence, such as functionality in FreeBSD that OpenBSD does not have.
There was a problem hiding this comment.
Let's keep them different then, at least for now.
| // | ||
| // This source file is part of the SwiftNIO open source project | ||
| // | ||
| // Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors |
There was a problem hiding this comment.
| // Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors | |
| // Copyright (c) 2025 Apple Inc. and the SwiftNIO project authors |
| // | ||
| // This source file is part of the SwiftNIO open source project | ||
| // | ||
| // Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors |
There was a problem hiding this comment.
| // Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors | |
| // Copyright (c) 2025 Apple Inc. and the SwiftNIO project authors |
elsewhere too
Lukasa
left a comment
There was a problem hiding this comment.
Ok @michael-yuji, I'm happy enough to go ahead with the Glibc module name for now. There are several outstanding pieces of review feedback, do you mind picking them up?
|
Thanks @Lukasa, I'll picking those up hopefully sometime around next week. |
This PR add FreeBSD support, currently build is passing against main swift branch, tests are not fully building but does not seems related to swift-nio itself
Apart from using kqueue, here's a list of non-trivial changes:
File copying
For file copying, we use
copy_file_rangeinstead ofsendfileas it is available on all supported FreeBSD option, so now,copyItem's implementation is like this:extended attributes
Unlike Linux and Darwin, the extended attribute apis are different in freebsd, e.g.
extattr_get_fdinstead ofgetxattrpktinfo
FreeBSD does not have
IP_PKTINFOfor IPv4, instead we combineIP_RECVIFandIP_ORIGDSTADDRto get the destination address and interface index.