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

Depend on system snappy (With OSX fix) #272

Merged
merged 20 commits into from
Mar 6, 2023
Merged

Conversation

codeadict
Copy link
Contributor

@codeadict codeadict commented Feb 20, 2023

This builds on top of @hmmr 's #271.

  • Fixes compilation in OSX
  • Adds OSX tests to CI
  • Add some port compiler cleanup on the rebar.config, it was using the old config style

Fixes #270

@codeadict
Copy link
Contributor Author

@hmmr @ioolkos @martinsumner This one is passing now on both Linux and OSX, don't have a FreeBSD box myself but should be fine there too 🤞🏽 . See passing tests at https://github.com/codeadict/eleveldb/actions/runs/4239145446/jobs/7366922850

@ioolkos
Copy link

ioolkos commented Feb 22, 2023

Confirming this works on Ubuntu.
➜ eleveldb git:(272) ldd -r _build/default/lib/eleveldb/priv/eleveldb.so shows no more missing Snappy symbols.

@codeadict
Copy link
Contributor Author

Looks like Github actions are stuck on the OTP 22 job:

Job is about to start running on the hosted runner: GitHub Actions 3 (hosted)

Might be a good idea to cancel and re-run them.

@ioolkos
Copy link

ioolkos commented Feb 25, 2023

Just made a visit here, scanning through #269 to #271 on which this PR is based. Seems to keep the FreeBSD fixes done previously. Looks good to me, but more importantly let me know when I can help test any specifics.

@codeadict
Copy link
Contributor Author

Indeed @ioolkos , confirmed it works on FreeBSD:

Building LevelDB...
gmake LDFLAGS="" -C leveldb all
gmake[2]: Entering directory '/root/eleveldb/c_src/leveldb'
gmake[2]: Nothing to be done for 'all'.
gmake[2]: Leaving directory '/root/eleveldb/c_src/leveldb'
gmake LDFLAGS="-L/usr/local/lib -lsnappy -lpthread" -C leveldb tools
gmake[2]: Entering directory '/root/eleveldb/c_src/leveldb'
gmake[2]: Nothing to be done for 'tools'.
gmake[2]: Leaving directory '/root/eleveldb/c_src/leveldb'
for tool in leveldb_repair perf_dump sst_rewrite sst_scan; do cp leveldb/$tool /root/eleveldb/priv/; done
gmake[1]: Leaving directory '/root/eleveldb/c_src'
===> Analyzing applications...
===> Compiling eleveldb
===> Compiling c_src/eleveldb.cc
===> Compiling c_src/refobjects.cc
===> Compiling c_src/workitems.cc
===> Linking /root/eleveldb/priv/eleveldb.so
root@freebsd:~/eleveldb # uname -a
FreeBSD freebsd 13.1-RELEASE FreeBSD 13.1-RELEASE releng/13.1-n250148-fc952ac2212 GENERIC arm64

@ioolkos
Copy link

ioolkos commented Feb 28, 2023

@hmmr, @martinsumner do you see a possibility to merge this?

@hmmr
Copy link
Contributor

hmmr commented Mar 1, 2023

There's a fair bit of details specific to OSX way beyond my expertise, so I have to trust those details are done right. As long as it builds on linux (which it does), I'm giving my +1, but others should have a look.

@ioolkos
Copy link

ioolkos commented Mar 1, 2023

@hmmr thanks for your answer and your +1. Fair enough on the OSX remark. @codeadict can consult on this, maybe ensuring enough "separation" between OSX and Linux builds is enough. Or maybe that's already cleanly given here.

@martinsumner
Copy link
Contributor

Will do some testing of my own over the next couple of days just to double-check everything is all OK. I don't have the skills or knowledge to do a full review of the changes myself, but I think it might be fair to say there has been sufficient eyes on this, and sufficient testing to move forward.

This PR does resolve the problem we had running Riak on OSX Catalina, which is helpful to me personally.

@nsaadouni - do you have any objections to this being merged?

@codeadict
Copy link
Contributor Author

I'm free for any help needed on this, have some macs to test with different versions but also the CI in this PR runs on OSX ensuring this works well in OSX with all the OTP versions supported.

@hmmr hmmr mentioned this pull request Mar 4, 2023
@martinsumner martinsumner self-requested a review March 6, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compilation error when snappy is already installed
4 participants