Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3130186
Add checks for strlcat and strlcpy symbols in CMake
bc-lee Jul 17, 2024
ffeaad9
Do not depend on the HAVE_STRLCAT and HAVE_STRLCP` preprocessor defin…
bc-lee Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions Sources/CoreFoundation/internalInclude/CoreFoundation_Prefix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,14 @@ static dispatch_queue_t __ ## PREFIX ## Queue(void) { \
#endif
#endif

#if !TARGET_OS_MAC
#if !HAVE_STRLCPY
#if TARGET_OS_MAC || \
(TARGET_OS_LINUX && \
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

(defined(__GLIBC__) && \
(__GLIBC__ > 2 || (__GLIBC__ == 2 && __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) {
const size_t srclen = strlen(src);
Expand All @@ -212,9 +218,7 @@ strlcpy(char * dst, const char * src, size_t maxlen) {
}
return srclen;
}
#endif

#if !HAVE_STRLCAT
CF_INLINE size_t
strlcat(char * dst, const char * src, size_t maxlen) {
const size_t srclen = strlen(src);
Expand All @@ -228,8 +232,8 @@ strlcat(char * dst, const char * src, size_t maxlen) {
}
return dstlen + srclen;
}
#endif
#endif // !TARGET_OS_MAC
#endif // TARGET_OS_MAC || (TARGET_OS_LINUX && defined(__GLIBC__) && (__GLIBC__
// > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 38)))

#if TARGET_OS_WIN32
// Compatibility with boolean.h
Expand Down