Skip to content
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

Merged
merged 8 commits into from
Jan 31, 2019
Merged

adds FreeBSD OS, and potentially other BSD OSes #476

merged 8 commits into from
Jan 31, 2019

Conversation

gwquk
Copy link
Contributor

@gwquk gwquk commented Jan 28, 2019

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

Copy link
Member

@s3rvac s3rvac left a 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.

src/utils/memory.cpp Outdated Show resolved Hide resolved
include/retdec/utils/os.h Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
src/utils/memory.cpp Outdated Show resolved Hide resolved
src/utils/memory.cpp Outdated Show resolved Hide resolved
include/retdec/utils/os.h Show resolved Hide resolved
The heading looks a bit nicer with the newly inserted empty comment.
@gwquk gwquk closed this Jan 28, 2019
@s3rvac
Copy link
Member

s3rvac commented Jan 29, 2019

Hello @gwquk. Have you closed this PR by accident or have you stumbled upon something that has rendered the PR unusable?

@gwquk
Copy link
Contributor Author

gwquk commented Jan 29, 2019

Yes, trying to solve this problem...

git push origin master

Username for 'https://github.com':
Password for 'https://gwquk@github.com':
remote: Permission to avast-tl/retdec.git denied to gwquk.
fatal: unable to access 'https://github.com/avast-tl/retdec.git/': The requested URL returned error: 403

@gwquk
Copy link
Contributor Author

gwquk commented Jan 29, 2019

I have made all the suggested changes... including the README.md (which I edited manually) just can't get git to push.

@s3rvac
Copy link
Member

s3rvac commented Jan 29, 2019

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

@gwquk
Copy link
Contributor Author

gwquk commented Jan 29, 2019

Have i managed to fix the issue?

@s3rvac
Copy link
Member

s3rvac commented Jan 29, 2019

I do not see the changes here. Have you pushed them? Anyway, let's open the PR.

@s3rvac s3rvac reopened this Jan 29, 2019
@s3rvac
Copy link
Member

s3rvac commented Jan 29, 2019

Now that the PR is opened, I can see the changes.

Copy link
Member

@s3rvac s3rvac left a 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.

include/retdec/utils/os.h Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@s3rvac
Copy link
Member

s3rvac commented Jan 30, 2019

All of our builds have succeeded after the latest fixes 👏. Would you like to change/add anything else or can I merge the PR?

@gwquk
Copy link
Contributor Author

gwquk commented Jan 30, 2019

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.

Copy link
Member

@s3rvac s3rvac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you! 👍

@s3rvac s3rvac merged commit 6ca4c8c into avast:master Jan 31, 2019
s3rvac added a commit that referenced this pull request Jan 31, 2019
@s3rvac
Copy link
Member

s3rvac commented Jan 31, 2019

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.

@gwquk
Copy link
Contributor Author

gwquk commented Jan 31, 2019 via email

@s3rvac s3rvac added the O-bsd label Feb 5, 2019
@lwhsu
Copy link

lwhsu commented Feb 5, 2019

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?

@s3rvac
Copy link
Member

s3rvac commented Feb 6, 2019

@lwhsu Good idea. I have created an issue for connecting RetDec to a CI service supporting FreeBSD: #493. As we have no experience with FreeBSD, we encourage the community to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants