-
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
Fix issue 1811 -- Version 2 #2124
Conversation
This file defines a single macro that attempts to autoconfigure chakracore.
The Shared Library for ChakraCore already contains all symbols needed. This means that most applications will only need symbols that are exported by libChakraCore.so or libChakraCore.dylib.
This file is intended to be under MIT license.
This provides a solution to issue chakra-core#1811, but this also breaks the ch utility in the process.
There is no reason for xplat to have this function be non-reentrant. This also reduces the number of code changes in issue chakra-core#1811 since this change fixes the issue with running ch. v2: use ifdef, since this call is ran on windows.
Hi @kphillisjr, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
@kphillisjr could you please point me the part that fixes #1811 ? In other words, why we are doing these changes? |
The commit two commits specific to issue #1811 are 75182f3 and b45d0b5. However the rest of the commits are for the earlier pull request (#2111) which is in regards to issue #1616. EDIT: It took longer to better explain the changes since I thought I had a decent explanation to begin with. In general the current solution for issue #1811 is a byproduct of attempting to fully resolve issue #1616. Currently I have the Following fixed in regards to issue #1616 by this pull request...
What is left to Fix for issue #1616 is as follows...
|
@kphillisjr I do really appreciate the overall effort.
From these two, we could use a solution like this; After build is done, copy header files to BuildLinux/<BUILD_TYPE>/include folder. Feel free to send a single commit PR only doing this. You may even create a Github issue on this particular issue and send a PR separately fixing that issue.
We already removed our dependency to libunwind. We may approach libuuid in a similar manner. So, we better skip this.
IMO; We may need to skip this (too) for now as it kinda complicates the number of build arguments.
We have so many differences between Windows and Xplat builds. IMO something like;
Actually we could combine batch file and sh file and have a single file instead of build.bat and build.sh separately. There are couple of ways to trigger either Windows or Bash specific command set under the same file. This one is not that critical, once we have the build.bat is contributed, I can also combine them later. Thanks again. |
This implements the changes required to fix issue #1811. There is only two changes and this ensures that ch runs correctly after on Linux/OSX.
Also, this pull request is branched off of another pull request ( see: #2111 )
NOTE: The only change between this version and the other is that the DynamicProfileStorage change is only applied to xplat.