-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
cross-platform: ChakraCore on OSX #1134
Conversation
Thanks for jumping into it so quickly. I tested it locally and I saw it already compiles, that's great. Let me know if there's anything I can help with. I'd also be more than happy to start looking into iOS after that. |
|
||
# By default, don't export any symbols from this library | ||
# We'll manually export the relevant individual functions | ||
set_target_properties(ChakraCore PROPERTIES CXX_VISIBILITY_PRESET hidden) |
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.
Is this removed intentionally even on Linux, the comments not true?
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.
This is a false setting..So, it doesn't affect statically inherited libraries, hence linker gives warnings.
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.
Can you provide some more context here please? If we wanted a dynamic library on Linux, is that still not needed?
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.
This SO inherits other static libraries with visibility=default and expect to be a proxy. If we want a particular interface is hidden, this is not the place in our case.
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.
Should this line then be moved to the root CMakeLists.txt if that's the behavior we want?
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.
See fvisibility=hidden
under root/CMakeLists.txt
PR is updated. @tadeuzagallo @jianchun @digitalinfinity Thanks for the reviews so far. |
COMMAND ${CMAKE_COMMAND} -E copy_if_different | ||
"${CHAKRACORE_BINARY_DIR}/bin/ChakraCore/libChakraCore.so" | ||
${CHAKRACORE_BINARY_DIR}/) | ||
elseif(CMAKE_SYSTEM_NAME STREQUAL Darwin) |
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.
One thing is that its a little hard to read what the difference is between OSX and Linux- can we take this list, assign it to a CMakeList variable and then append just the diffs in the OSX/Linux cases (and then assign the customized list to target_link_libraries). If not, a comment on what the differences are in this particular case between OSX and Linux would make this easier to read in the future.
Hmm. CI is down I guess? |
Disclaimer: Currently, ChakraCore is not officially supported on OSX. This PR targets the individuals interested with experimenting ChakraCore on OSX. After seeing #1131, spent couple of hours to fix the compile, and link time issues on OSX target. Steps to compile : ``` brew install icu4c ./build.sh --debug -j:2 --icu=/usr/local/Cellar/icu4c/54.1/include/ ``` Status: CMAKE has an issue with OSX shared bundles and currently CH does not play well with dylib output.
Merge pull request #1134 from obastemur:osx_support Disclaimer: Currently, ChakraCore is not officially supported on OSX. This PR targets the individuals interested with experimenting ChakraCore on OSX. After seeing #1131, spent couple of hours to fix the compile, and link time issues on OSX target. Steps to compile : ``` brew install icu4c ./build.sh --debug -j:2 --icu=/usr/local/Cellar/icu4c/54.1/include/ ``` Status: CMAKE has an issue with shared bundles on OSX (a.k.a. so) and currently CH does not play well with dylib.
Disclaimer: Currently, ChakraCore is not officially supported on OSX.
This PR targets the individuals interested with experimenting ChakraCore on
OSX. After seeing #1131, spent couple of hours to fix the compile, and link
time issues on OSX target.
Steps to compile :
Status:
CMAKE has an issue with shared bundles on OSX (a.k.a. so) and currently CH does not
play well with dylib.