-
Notifications
You must be signed in to change notification settings - Fork 953
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
adds FreeBSD OS, and potentially other BSD OSes #476
Conversation
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.
Thank you for adding support for BSD! 👍 We would definitely like to merge the PR. However, I have included several comments and suggestions that should be resolved first.
The heading looks a bit nicer with the newly inserted empty comment.
Hello @gwquk. Have you closed this PR by accident or have you stumbled upon something that has rendered the PR unusable? |
Yes, trying to solve this problem... git push origin masterUsername for 'https://github.com': |
I have made all the suggested changes... including the README.md (which I edited manually) just can't get git to push. |
It seems that you are trying to push to https://github.com/avast-tl/retdec.git You will have to push to your fork instead: https://github.com/gwquk/retdec.git |
Have i managed to fix the issue? |
I do not see the changes here. Have you pushed them? Anyway, let's open the PR. |
Now that the PR is opened, I can see the changes. |
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.
Thank you for the changes and README improvements 👍. There are a few more things that need to be resolved before we can merge this.
All of our builds have succeeded after the latest fixes 👏. Would you like to change/add anything else or can I merge the PR? |
Yes please do. 👍 There are a couple of compiler warnings that need further investigating but the retdec-decompiler program seems to run fine with initial testing. |
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.
Great, thank you! 👍
As for the Clang compilation warnings, I have fixed several of them in 4c37f60 and d9077c4 (on Linux), but feel free to either report the missed ones to us by opening an issue or submitting a PR which fixes the warnings Clang reports on your system. |
OK, Thanks
On Thursday, 31 January 2019, 6:42:34 am GMT, Petr Zemek <[email protected]> wrote:
As for the Clang compilation warnings, I have fixed several of them in 4c37f60 and d9077c4 (on Linux), but feel free to either report the missed ones to us by opening an issue or submitting a PR which fixes the warnings Clang reports on your system.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Thanks for working on FreeBSD support! Is there any plan to have a hosted CI with FreeBSD integrated? Currently there is https://cirrus-ci.com/ supports FreeBSD. Also, I would like to help on getting this to FreeBSD ports tree to have a pre-built package available, @gwquk , are you interested in doing this? |
Successfully compiles on FreeBSD 12 (which is the latest release). Other BSDs may also work with these changes. Clang (LLVM) 7 had a problem (error) with the line in bold from ./src/utils/memory.cpp
bool limitSystemMemoryOnPOSIX(std::size_t limit) {
struct rlimit rl = {
.rlim_cur = limit, // Soft limit.
I had to add this to the ./CMakeLists.txt, so Clang created a 'warning' during compilation not an 'error'
# Clang does Wc++11-narrowing, must be off for memory.ccp line 35
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-error=c++11-narrowing")
I have fixed my 'push' identity to gwquk
Thanks for making this Open Source!
Greg