Do not depend on the HAVE_STRLCAT and HAVE_STRLCPY preprocessor definitions. #5013

bc-lee · 2024-07-17T23:41:54Z

While checking whether the system provides the strlcat and strlcpy
functions, using an autoconf-style check to determine their availability
does not seem to be a good idea. This is because the code is also
used with SwiftPM, which does not have a similar feature test.
Instead, we should directly check the availability of these functions
in the system.

Given that strlcpy and strlcat are not standard C functions,
we should check whether the system is macOS, Linux with Bionic or
Musl libc, or glibc version 2.38 or later. This patch adds the necessary
checks in the header file so that SwiftPM can use these checks as well.

Fixes #5012

These checks are required because glibc 2.38+ added strlcat and strlcpy functions. swift-corelibs-foundation should use these functions if they are available. The checks originally existed in commit 8d1a679, but were not added to the new CMakeLists.txt in commit c772413. Fixes swiftlang#5012

jmschonfeld · 2024-07-18T17:13:02Z

CMakeLists.txt

 include(CheckLinkerFlag)

+check_symbol_exists(strlcat "string.h" HAVE_STRLCAT)
+check_symbol_exists(strlcpy "string.h" HAVE_STRLCPY)


Is there a way that we can replicate this behavior in the package manifest as well so that when building via SwiftPM we get the same behavior? Or a way that we can add this check into the code itself rather than on the CMake/SwiftPM side?

Generally, no, SwiftPM doesn't have a way to add such checks or generate "config.h" files.

However, in these cases (HAVE_STRLCAT and HAVE_STRLCPY in Swift), I think we can change swift-corelibs-foundation (and other projects) to use local versions of these functions only if certain conditions are not met. Below is an example of how we can do this:

#if !TARGET_OS_MAC #if defined(__linux__) && ((defined(__GLIBC__) && __GLIBC__ >= 2 && defined(__GLIBC_MINOR__) && __GLIBC_MINOR__ >= 38) || !defined(__GLIBC__)) // strlcat and strlcpy are available in the system #else // Define local versions of strlcpy and strlcat for glibc < 2.38 CF_INLINE size_t strlcpy(char * dst, const char * src, size_t maxlen) { ... } #endif #endif // !TARGET_OS_MAC

This code assumes the following things:

Swift is supported on macOS (and other Apple platforms), Linux, and Windows.

Android's bionic supports strlcpy and strlcat. Link

Other than Android, Linux uses glibc or musl. Musl supports strlcpy and strlcat. Link

glibc 2.38 and later supports strlcpy and strlcat.

Windows doesn't have strlcpy and strlcat.

This approach is somewhat error-prone and may not be the best way to handle this, but I think this is the only way to handle this in SwiftPM. Also, I don't think supported platforms change frequently, so this approach should be fine with appropriate compile flags. Do you think this approach is acceptable?

I think this is an alright way to right this given that SwiftPM can't perform a check - assuming there's no other macro defined in the header that we can use to indicate the presence or lack of this function. @parkera had to do something similar for CURL: https://github.com/apple/swift-corelibs-foundation/blob/15ee5c32d2b808fd912c586c535feaf9af2eb2d5/Sources/_CFURLSessionInterface/include/CFURLSessionInterface.h#L39-L57

The only tweak I'd suggest - rather than doing this for !TARGET_OS_MAC, should we just only do this for TARGET_OS_LINUX (or whatever the right macro is for that)? In other words, do we need to define strlcpy for Windows/Android or does the source code in Foundation already handle not calling that function?

If you think it is a good idea to remove autoconf-style feature test preprocessor macros and check features in the code for compatibility with SwiftPM, I'll revisit this patch to do so.

👍 yep since the alternative would break the SwiftPM build of this project, checking conditionally in the code is the way to go here to avoid breaking the build

compnerd

This definitely seems like a good improvement once we add the Swift case as well.

compnerd · 2024-07-18T20:54:56Z

CMakeLists.txt

+endif()
+if(HAVE_STRLCPY)
+    add_compile_definitions($<$<OR:$<COMPILE_LANGUAGE:C>,$<COMPILE_LANGUAGE:CXX>>:HAVE_STRLCPY>)
+endif()


I think that we need to have an entry for Swift as well to pass this to the ClangImporter. Something like $<$<COMPILE_LANGUAGE:Swift>:SHELL:-Xcc -DHAVE_STRLCPY>

All changes within CMakeLists.txt were reverted in the latest commit. Please check the header file for the changes.

…itions. While checking whether the system provides the `strlcat` and `strlcpy` functions, using an autoconf-style check to determine their availability does not seem to be a good idea. This is because the code is also used with SwiftPM, which does not have a similar feature test. Instead, we should directly check the availability of these functions in the system. Given that `strlcpy` and `strlcat` are not standard C functions, we should check whether the system is macOS, Linux with Bionic or Musl libc, or glibc version 2.38 or later. This patch adds the necessary checks in the header file so that SwiftPM can use these checks as well. Fixes swiftlang#5012

parkera · 2024-07-19T03:39:52Z

@swift-ci test

parkera · 2024-07-19T03:40:23Z

Sources/CoreFoundation/internalInclude/CoreFoundation_Prefix.h

-#if !TARGET_OS_MAC
-#if !HAVE_STRLCPY
+#if TARGET_OS_MAC ||                                                           \
+    (TARGET_OS_LINUX &&                                                        \


Is this going to be a problem for platforms which are not Linux but do have strlcat (FreeBSD, e.g.)?

(maybe not depending on the definition of __GLIBC__ but I'm unsure of where that's set)

That's the weakness of this approach, and that's also why every project has a list of supporting platforms. From what I know, Swift isn't officially supported on BSD variants (obviously, except for Darwin). So, I think it's not a problem. I don't even have a FreeBSD machine to test it. But if it is absolutely necessary, I may add another check for BSD variants based on https://sourceforge.net/p/predef/wiki/OperatingSystems/ and other sources, but honestly, I don't think it's necessary.

FreeBSD does have a port as does Haiku, PS4, etc. I think that the target os macros do not work even on Linux. Does this cover uclibc and newlib? There is a plethora of libcs, and the fact that this "breaks" on SPM is fine - you can mirror this into SPM as:

cSettings: [ .define("HAVE_STRLCAT", .when(platforms: [.linux]))), .define("HAVE_STRLCPY", .when(platforms: [.linux]))), ]

@jmschonfeld - I think that this is a sufficient means of mapping this in SPM, which will need to be changed if your environment is different as SPM does not have the necessary flexibility.

What if this (swift-corelibs-foundation) library is used as a transitive dependency in a Swift Package Manager project? Since SPM can't replace dependencies on the consumer side, it is extremely annoying to patch existing projects, as all dependent projects need to be patched as well. If we had a similar mechanism like go mod replace or npm's override mechanism, it would be much easier to handle this kind of situation, but that is a different topic.

You would need to update the dependency. Fortunately, swift-corelibs-foundation cannot be used as a package dependency and is shipped as part of the toolchain.

It seems that the annoyance of replacing transitive dependencies is not a problem for the swift-corelibs-foundation repository. However, I'm still unsure about defining HAVE_STRLCAT and HAVE_STRLCPY in the SwiftPM manifest file. Even though it's the same OS (Linux), the actual availability of these functions can vary. I think it would be better to define these macros in the source code, even though it might be a bit messy.

I also wish Swift had clearer documentation on which platforms are supported and which are not, so we can avoid confusions with platforms like FreeBSD. I have made a post on the Swift forums to clarify this: https://forums.swift.org/t/clarifying-supported-platforms-for-swift/73309

aoberoi · 2024-12-20T06:45:33Z

Hi there, I'm wondering if there are any updates regarding this change landing. I'm a little out of my depth wrt the discussion of multiple techniques of implementing the conditional checks required, but it sounds to me that there is some consensus on a preferred solution that would require a new capability of SwiftPM. If that is true, is there an issue or discussion of to spec out this new capability? If someone were able to volunteer some guidance, I may even be able to pitch or implement such a change.

I'm interested in using swift-corelibs-foundation as a package dependency instead of using the binaries within the toolchain so that I can more easily debug issues by stepping into the source. I believe this is aligned with the project and work group's goal of minimizing this compatibility layer over time, since it will help people debug issues, find inconsistencies, and contribute changes to swift-foundation.

jmschonfeld · 2024-12-20T19:00:50Z

Hi there, I'm wondering if there are any updates regarding this change landing. I'm a little out of my depth wrt the discussion of multiple techniques of implementing the conditional checks required, but it sounds to me that there is some consensus on a preferred solution that would require a new capability of SwiftPM. If that is true, is there an issue or discussion of to spec out this new capability? If someone were able to volunteer some guidance, I may even be able to pitch or implement such a change.

This change was landed, in effect, in some other PRs I believe. The current state is:

The CMake build sets HAVE_STRLCAT/HAVE_STRLCPY for platforms that define this symbol by performing the most accurate check (actually looking for the existence of the symbol). This means that the CMake build sets this correctly always
In the source code, a few platforms have explicitly set these values unconditionally for their platform (like WASM IIRC) which will affect both CMake and SwiftPM builds and ensure this is always correct regardless of the build system for that platform
SwiftPM unfortunately does not yet have the capability to check for the existence of the symbol, so the best it can do is make a best effort attempt (enabling/disabling on a per-platform basis), which is "good enough" for our uses of the SwiftPM build (at-desk building of this project on main supported platforms)

If you're hitting build failures related to this, I'd be interested to know your build setup (which platform/SDK, which build system - CMake or SwiftPM, etc.)

I'm interested in using swift-corelibs-foundation as a package dependency instead of using the binaries within the toolchain so that I can more easily debug issues by stepping into the source. I believe this is aligned with the project and work group's goal of minimizing this compatibility layer over time, since it will help people debug issues, find inconsistencies, and contribute changes to swift-foundation.

A word of caution here: the SwiftPM build of this project is intended to be used for development of this project but not as a stable dependency for other projects. Other projects need to use the Foundation from the toolchain so that everybody "agrees" on which Foundation to use. If your project imports Foundation from a package dependency and uses another package dependency that also imports Foundation from the toolchain, you will end up with "two Foundations" that are not compatible with each other. You can definitely use the SwiftPM build as a dependency for small test projects to debug issues (I often use it as a dependency for a small CLI that I can use to test out reported problematic code) but using it as a dependency for a full project with a tree of dependencies (especially in any production use cases) is not supported and likely will not work due to the "two Foundations" problem. Hopefully that clears this up a bit just to make sure you don't accidentally end up in unsupported territory 🙂.

bc-lee · 2024-12-21T10:32:42Z

I confirmed that this patch is no longer necessary, as the relevant PR has already landed in #5113. Closing this.

jmschonfeld reviewed Jul 18, 2024

View reviewed changes

compnerd reviewed Jul 18, 2024

View reviewed changes

bc-lee changed the title ~~Add checks for strlcat and strlcpy symbols in CMake~~ Do not depend on the HAVE_STRLCAT and HAVE_STRLCP` preprocessor definitions. Jul 19, 2024

bc-lee requested review from jmschonfeld and compnerd July 19, 2024 01:38

parkera reviewed Jul 19, 2024

View reviewed changes

bc-lee changed the title ~~Do not depend on the HAVE_STRLCAT and HAVE_STRLCP` preprocessor definitions.~~ Do not depend on the HAVE_STRLCAT and HAVE_STRLCPY preprocessor definitions. Jul 19, 2024

jmschonfeld mentioned this pull request Aug 8, 2024

Fixing post-swift-foundation corelibs-foundation static SDK builds #5060

Merged

bc-lee closed this Dec 21, 2024

bc-lee deleted the feature/fix-cmake-strlcpy-check branch December 23, 2024 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not depend on the HAVE_STRLCAT and HAVE_STRLCPY preprocessor definitions. #5013

Do not depend on the HAVE_STRLCAT and HAVE_STRLCPY preprocessor definitions. #5013

bc-lee commented Jul 17, 2024 •

edited

Loading

jmschonfeld Jul 18, 2024

bc-lee Jul 18, 2024

jmschonfeld Jul 18, 2024

bc-lee Jul 18, 2024

jmschonfeld Jul 18, 2024

compnerd left a comment

compnerd Jul 18, 2024

bc-lee Jul 19, 2024

parkera commented Jul 19, 2024

parkera Jul 19, 2024

parkera Jul 19, 2024

bc-lee Jul 19, 2024

compnerd Jul 19, 2024

bc-lee Jul 19, 2024

compnerd Jul 19, 2024

bc-lee Jul 21, 2024

aoberoi commented Dec 20, 2024

jmschonfeld commented Dec 20, 2024

bc-lee commented Dec 21, 2024

Do not depend on the HAVE_STRLCAT and HAVE_STRLCPY preprocessor definitions. #5013

Do not depend on the HAVE_STRLCAT and HAVE_STRLCPY preprocessor definitions. #5013

Conversation

bc-lee commented Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkera commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoberoi commented Dec 20, 2024

jmschonfeld commented Dec 20, 2024

bc-lee commented Dec 21, 2024

bc-lee commented Jul 17, 2024 •

edited

Loading