-
Notifications
You must be signed in to change notification settings - Fork 10.5k
FreeBSD compatibility patch #4804
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
Changes from all commits
014963f
f11d057
6428295
0492374
33d2dec
afc95a5
29d5de1
4936a39
4c84a33
4dbf4de
7e51593
b267fda
98a8b58
7c50ccd
7edc310
47b4c7e
69085ae
ede70f2
1a148c6
8bda672
07166c0
419c832
db8878e
22c0b4a
f3b612a
fb70a1b
c1676a1
0b30970
97d3fba
5384630
7cebd11
6613f07
197b64f
134bd2e
536c1ac
0039860
5462fb2
6a121fb
9c53bb2
cd52ed6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,18 @@ set(LLVM_OPTIONAL_SOURCES | |
${swift_stubs_objc_sources} | ||
${swift_stubs_unicode_normalization_sources}) | ||
|
||
# ICU isn't required on Darwin, but is on every other platform. | ||
# Now in case we're cross-compiling from Darwin for another platform, | ||
# the find_package should still be executed. | ||
if(CMAKE_SYSTEM_NAME STREQUAL "Darwin") | ||
set(icu_required "") | ||
else() | ||
set(icu_required "REQUIRED") | ||
endif() | ||
|
||
find_package(ICU ${icu_required} COMPONENTS uc) | ||
set(ICU_UC_LIBRARY "") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems at odds with what Android and the newer port-to-Windows work (cc @modocache again and @hughbe). I don't understand what the 'REQUIRED' is doing, either—the code doesn't match the comment, which doesn't seem to be the desired behavior anyway. (Also, CMake doesn't use commas.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way I'm doing it on Windows is a bit different, as we don't have a build-script so I had to make things simple. See this: https://github.com/apple/swift/pull/5904/files#diff-af3b638bc2a3e6c650974192a53c7291R594 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intention of the "REQUIRED" if/else was to make find_package() not fail in case we're compiling on Darwin (since it isn't needed on that platform anyway), but fail on other platforms when no ICU library is found. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kstaring politely poke, any progress👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having looked at the Android and Windows solution, it's difficult for me to find the proper way forward.
In other words, both examples pointed to don't seem to provide solutions for the problem on FreeBSD (or other platforms for which the compiler doesn't have a default include path set where ICU can be found). find_package() provides the correct include paths on FreeBSD, but the include flags aren't used for Android and Windows. It would be far easiest to take the Android solution route and simply add -I/usr/local/include somewhere (e.g. in AddSwift.cmake), but that doesn't really feel like a good solution given that find_package() would provide the correct path and not some hardcoded path... So, if someone could comment (@jrose-apple ?) on the route that is deemed most correct, that would be greatly appreciated. Thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the reason the find_package() is present in the stubs/CMakeLists.txt is exactly because the include flags are needed at that point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kstaring Glad you have the BSD sprite to get it right, no worries, please take your time There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deferring to @llvm-beanz and maybe @modocache. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic here WRT "REQUIRES" is opposite the comment and the existing code in the standard library. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason I was totally looking over the problem pointed to by @jrose-apple .. It took @llvm-beanz 's comments to make me realise the problematic REQUIRED logic.. |
||
|
||
set(swift_stubs_c_compile_flags ${SWIFT_RUNTIME_CORE_CXX_FLAGS}) | ||
list(APPEND swift_stubs_c_compile_flags -DswiftCore_EXPORTS) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,10 @@ | |
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#if defined(__FreeBSD__) | ||
#define _WITH_GETLINE | ||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes to this file LGTM, feel free to submit as a separate PR if you would like them to be merged immediately. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This particular bit LGTM too. |
||
|
||
#include "swift/Basic/DemangleWrappers.h" | ||
#include "llvm/ADT/SmallString.h" | ||
#include "llvm/Support/CommandLine.h" | ||
|
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.
How about including
libutil.h
instead?(And for
utmp.h
instead includingutmpx.h
?)