Removed Linux use of sys/sysctl.h #2771
Conversation
| @@ -27,7 +27,7 @@ | |||
|
|
|||
| #if TARGET_OS_MAC || TARGET_OS_LINUX || TARGET_OS_BSD | |||
| #include <unistd.h> | |||
| #if !TARGET_OS_ANDROID | |||
| #if !TARGET_OS_ANDROID || !TARGET_OS_LINUX | |||
There was a problem hiding this comment.
I think TARGET_OS_ANDROID implies TARGET_IS_LINUX.
From CoreFoundation/Base.subproj/SwiftRuntime/TargetConditionals.h:
#elif __ANDROID__
#define TARGET_OS_DARWIN 0
#define TARGET_OS_LINUX 1
#define TARGET_OS_WINDOWS 0
#define TARGET_OS_BSD 0
#define TARGET_OS_ANDROID 1
#define TARGET_OS_CYGWIN 0
There was a problem hiding this comment.
I changed the patch to be inclusive of BSD and macOS, and not exclusive of Linux/Android
There was a problem hiding this comment.
Included CFBundle_InfoPlist.c in the PR as it too includes sys/sysctl.h
There was a problem hiding this comment.
Hmm, I confused this with the other file (CFBundle_InfoPlist.c). Is the #include even needed here on any target?
There was a problem hiding this comment.
sysctl.h is still included for all Apple SDKs and is not marked as deprecated. I do not have a BSD box to verify what its status is.
There was a problem hiding this comment.
I meant that nothing in this file appears to use facilities from <sys/sysctl.h>, as far as I can see. There are no sysctl*, KERN_*, or CTL_* identifiers used.
There was a problem hiding this comment.
Ah, I understand. I see what you mean, but I wanted to keep the PR Linux-focused and not presume that it could be removed, regardless of platform.
There was a problem hiding this comment.
This change was dropped in the final merge, @tachoknight, mistake?
There was a problem hiding this comment.
Btw, I ran the tests natively on Android with both changes, ie including this one that was dropped in the final merge, and didn't have a problem.
There was a problem hiding this comment.
@buttaface Sigh, yes, a rebasing mistake. I submitted this PR for the other file. Thanks a lot for noticing it.
|
@swift-ci test linux |
| @@ -17,7 +17,7 @@ | |||
|
|
|||
| #if (TARGET_OS_MAC || TARGET_OS_LINUX || TARGET_OS_BSD) && !TARGET_OS_CYGWIN | |||
| #include <dirent.h> | |||
| #if !TARGET_OS_ANDROID | |||
| #if TARGET_OS_MAC || TARGET_OS_BSD | |||
There was a problem hiding this comment.
The call site for sysctlbyname is guarded by TARGET_OS_IPHONE, so perhaps use that here, too?
There was a problem hiding this comment.
Well, TARGET_OS_IPHONE wasn't included in the original code, I'm leery of adding anything that could cause unintended side effects.
|
@swift-ci please test Linux platform |
|
The checks failed, but looking at the details, the errors don't seem related to this PR. |
|
@swift-ci please test Linux platform |
|
Huh, that's really weird that it didn't work. |
|
@tachoknight I don't think thats related to your change, please do address the last comment by @fweimer though. |
|
@swift-ci please test Linux platform |
1 similar comment
|
@swift-ci please test Linux platform |
|
Is there anyway to test this change against android? |
|
For the record, this builds OK standalone on OpenBSD (WIP, and modulo still open PRs), when cherrypicking this PR and using only |
|
@spevans sure, I can do a run against android, seems that the last comment has been addressed, I’ll kick it off in a bit |
|
@swift-ci please test Linux platform |
|
Can these 3 commits be squashed down into 1 since this PR is only a 2line change? |
|
@spevans Would you like me to close this PR and open a new one with just the one change? |
|
@tachoknight No need, you can do a rebase locally to squash all of the commits and then if you force push to github it will update the PR. |
|
@spevans: Done, sorry for the mess :) |
|
@swift-ci linux test |
|
@swift-ci please test Linux platform |
|
Windows, Android, Linux, OpenBSD reporting in: passing. Lets go ahead with this. |
The header is deprecated and removed in 5.5 of the kernel which causes Swift not to build.