-
Notifications
You must be signed in to change notification settings - Fork 185
OpenBSD support. #1126
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.
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
OpenBSD support. #1126
Conversation
@swift-ci please test |
1 similar comment
@swift-ci please test |
Good to see you finally joined the swiftlang org and got CI access. 😃 |
@swift-ci please test |
In swiftlang#1075 the change was already made for BSD (thank you!); my working edit had this guidance to ensure future porters get an error directing them where to make a necessary change. Otherwise, the FoundationEssentials build will fail and complain these variables are not defined but not have guidance as to where they are sourced from.
* OpenBSD also needs `pthread_mutex_t?`. * Originally I followed Darwin's check with `d_namlen`, but this should work too.
On OpenBSD, fsblkcnt_t -- the type of f_blocks -- is a UInt64; therefore, so must `blockSize` be. Ultimately, both sides of the `totalSizeBytes` multiplication should probably be type cast for all platforms, but that's a more significant functional change for another time.
After a rather tedious debugging session trying to figure out why swiftpm-bootstrap appeared to be deadlocked, this turned out to be the culprit. Perhaps this should be #error instead, but for now, set a sensible default.
This is what Dispatch does in some places for OpenBSD anyway, so do likewise here.
@swift-ci please test. |
@@ -481,7 +481,7 @@ extension _ProcessInfo { | |||
GetSystemInfo(&sysInfo) | |||
return sysInfo.dwActiveProcessorMask.nonzeroBitCount | |||
#else | |||
return 0 | |||
return 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in the commit message, which I didn't transpose here, sorry:
After a rather tedious debugging session trying to figure out why
swiftpm-bootstrap appeared to be deadlocked, this turned out to be the
culprit. Perhaps this should be #error instead, but for now, set a
sensible default.
spm's bootstrap sets up an operation queue with activeProcessorCount
. If this isn't twiddled, this will be 0, so you have an operation queue that doesn't process, and isn't immediately clear this is the effect unless you dig very deep. It doesn't make sense to have the active processor count to be 0 as I understand it, hence the default change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I didn't read all the commit messages, just looked at the final patch. I guess that's up to the Foundation people, just wait till they chime in before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the two default changes, only affects OpenBSD, so should be fine.
@parkera, just need someone to chime in about the defaults. |
#elif TARGET_OS_WINDOWS | ||
/* not required */ | ||
#else | ||
#error "possibly define TZDIR and TZDEFAULT for this platform" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to cause a new error on any other platforms with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to this change, if there are other platforms that require TZDIR/TZDEFAULT that aren't TARGET_OS_MAC, TARGET_OS_LINUX, TARGET_OS_BSD or TARGET_OS_WINDOWS, this manifests as a missing symbol error where they are referenced, so there should be no new errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
* Advise porter on where to make necessary change. In #1075 the change was already made for BSD (thank you!); my working edit had this guidance to ensure future porters get an error directing them where to make a necessary change. Otherwise, the FoundationEssentials build will fail and complain these variables are not defined but not have guidance as to where they are sourced from. * OpenBSD does not support extended attributes. * OpenBSD does not have secure_getenv. * Remaining OpenBSD changes. * OpenBSD also needs `pthread_mutex_t?`. * Originally I followed Darwin's check with `d_namlen`, but this should work too. * Correct statvfs type casts for OpenBSD. On OpenBSD, fsblkcnt_t -- the type of f_blocks -- is a UInt64; therefore, so must `blockSize` be. Ultimately, both sides of the `totalSizeBytes` multiplication should probably be type cast for all platforms, but that's a more significant functional change for another time. * Default activeProcessorCount to 1, not 0. After a rather tedious debugging session trying to figure out why swiftpm-bootstrap appeared to be deadlocked, this turned out to be the culprit. Perhaps this should be #error instead, but for now, set a sensible default. * Use sysconf for activeProcessorCount. This is what Dispatch does in some places for OpenBSD anyway, so do likewise here.
* Advise porter on where to make necessary change. In swiftlang#1075 the change was already made for BSD (thank you!); my working edit had this guidance to ensure future porters get an error directing them where to make a necessary change. Otherwise, the FoundationEssentials build will fail and complain these variables are not defined but not have guidance as to where they are sourced from. * OpenBSD does not support extended attributes. * OpenBSD does not have secure_getenv. * Remaining OpenBSD changes. * OpenBSD also needs `pthread_mutex_t?`. * Originally I followed Darwin's check with `d_namlen`, but this should work too. * Correct statvfs type casts for OpenBSD. On OpenBSD, fsblkcnt_t -- the type of f_blocks -- is a UInt64; therefore, so must `blockSize` be. Ultimately, both sides of the `totalSizeBytes` multiplication should probably be type cast for all platforms, but that's a more significant functional change for another time. * Default activeProcessorCount to 1, not 0. After a rather tedious debugging session trying to figure out why swiftpm-bootstrap appeared to be deadlocked, this turned out to be the culprit. Perhaps this should be #error instead, but for now, set a sensible default. * Use sysconf for activeProcessorCount. This is what Dispatch does in some places for OpenBSD anyway, so do likewise here.
Fixes a few small matters:
OpenBSD also needs
pthread_mutex_t?
.OpenBSD does not have secure_getenv.
OpenBSD does not support extended attributes.
Correct statvfs type casts for OpenBSD.
On OpenBSD, fsblkcnt_t -- the type of f_blocks -- is a UInt64; therefore, so must
blockSize
be.Ultimately, both sides of the
totalSizeBytes
multiplication should probably be type cast for all platforms, but that's a more significant functional change for another time.Update the default to activeProcessorCount and add sysconf support. This is a more sensible default; it does not make sense to have 0 active processors on a running system as I understand the semantics.
Additionally, #1075 beat me to a few small changes; some follow-ups:
In
FileOperations+Enumeration.swift
originally I followed Darwin's check withd_namlen
, but this should work too.In my working edit for
_CStdlib.h
I incorporated a small piece of guidance in my working edit; add this guidance in now.