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

Arch linux snappy #175

Closed
wants to merge 5 commits into from
Closed

Arch linux snappy #175

wants to merge 5 commits into from

Conversation

pr4u4t
Copy link

@pr4u4t pr4u4t commented Sep 9, 2022

#171 according to my last comment check_library_exists(snappy snappy_compress "" HAVE_SNAPPY) should be changed to find_library(SNAPPY_LIBRARY snappy REQUIRED) as this library is needed by LevelDB and if shared only exists on the system it must be added to target linking.

@HalosGhost HalosGhost linked an issue Sep 12, 2022 that may be closed by this pull request
3 tasks
@HalosGhost
Copy link
Collaborator

solid Concept ACK. Can you rebase on trunk (to get to a single commit)? Then I'll test locally (looks like it'll be quick to test and merge).

@HalosGhost
Copy link
Collaborator

HalosGhost commented Sep 18, 2022

Can this be appropriately rebased and cherry-pick dc7dcc5? I just got a chance to test and this PR including that commit corrects the clean-build issue. I'd love to get that merged ASAP (possibly today if you have time to cherry-pick and rebase).

@HalosGhost
Copy link
Collaborator

@pr4u4t I'd really like to get this merged ASAP. I'm happy to help you get it appropriately rebased and squashed, or I can cherry-pick your commits to a new branch, and merge it separately if that would be easier. Given that trunk is actually broken for anyone without an appropriate env already setup, I'll probably move forward with the later option in the next few days if I don't hear back.

@pr4u4t
Copy link
Author

pr4u4t commented Sep 30, 2022

I may find time today or tomorrow to make a rebase I was busy with a thing that you mentioned in #176. I'm not that mad to shell client-cli. Command results for restd needs to be convertible to json or xml, for that I needed to create utility class to store command result take a look here. I also see a possibility to use TypedData for storing configuration #173. Configuration file may be designed to be more structured like the one from nginx or lighttpd and parsed using flex, bison couple. It's possible because configuration options are not deAlthough this interface is not yet complete as I need to make map and vector shared to get rid of copying.

@HalosGhost
Copy link
Collaborator

HalosGhost commented Sep 30, 2022

@pr4u4t I actually went ahead and setup a branch with the cherry-picking, rebasing/squashing done in case I didn't hear back. If you want to take a look and reconfirm your sign-off, I'd happily open a new PR from that branch and get it merged ASAP: afc129d

Note that there's really only one substantive change from what you submitted (squashing/commit-message/whitespace aside): the cmake find_library call didn't work for me correctly unless I referred to snappy rather than snappy_compress.

@pr4u4t
Copy link
Author

pr4u4t commented Sep 30, 2022

I don't mind unless this changes gets commited. What is really interesting that both snappy and snappy_compressoptions work for me. While we can consider that as resolved, let's focus on changes that I've proposed for command handling as there is still more work to do.

@HalosGhost
Copy link
Collaborator

I've opened the new PR and will merge it as soon as it has >0 tested ACKs. Once it's merged, it will make sense for you to rebase your other PR(s) on top of it so they all have a shared base.

As for which things it makes sense to merge after that, I'll take another review ASAP.

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.

Multiple occurences of unresolved symbols from snappy in leveldb
2 participants