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

Build: support minimal link flags #100

Merged
merged 2 commits into from
Jan 20, 2023
Merged

Build: support minimal link flags #100

merged 2 commits into from
Jan 20, 2023

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Dec 23, 2022

Removing these flags seems have no side effects, but make life easier with my package manager (nix), where I only need to install librocksdb, rather than installing all the indirect dependencies, those are indirectly brought in anyway.

In many cases, we don't need the extra link flags, either because librocksdb already referenced the indirect dependencies by path, or it's not built with it at all, so this PR introduce a build tag grocksdb_clean_link to only link minimal dependencies.

@linxGnu
Copy link
Owner

linxGnu commented Jan 9, 2023

Hello @yihuang , I am afraid of many current builds does not contains linking to lz4, snappy. Removing links might break those systems.

@yihuang
Copy link
Contributor Author

yihuang commented Jan 9, 2023

Hello @yihuang , I am afraid of many current builds does not contains linking to lz4, snappy. Removing links might break those systems.

are those libs are built with librocksdb itself? if librocksdb is not built with them, it doesn't matter if we specify them here?

@linxGnu
Copy link
Owner

linxGnu commented Jan 9, 2023

@yihuang yes, fyi, they are custom builds. There are many custom-builds like this.

@yihuang
Copy link
Contributor Author

yihuang commented Jan 9, 2023

@yihuang yes, fyi, they are custom builds. There are many custom-builds like this.

I mean if the librocksdb is built with them, it should still doesn't matter if we specify here or not, do you get a link error if we don't specify the flags here?
We've been successfully using gorocksdb without these flags.

@yihuang
Copy link
Contributor Author

yihuang commented Jan 9, 2023

Did you do static link?

@yihuang yihuang force-pushed the master branch 2 times, most recently from 055adc2 to d59f4b7 Compare January 12, 2023 02:48
@yihuang
Copy link
Contributor Author

yihuang commented Jan 12, 2023

@linxGnu how about make it optional, I added a build tag grocksdb_clean_link, so the default behavior is not changed.

@yihuang yihuang changed the title Build: don't specifiy the indirect libs Build: support cleaner link flags Jan 12, 2023
@linxGnu
Copy link
Owner

linxGnu commented Jan 12, 2023

Sound great to me when using directive building. Let me have a look this week and feedback to you on Monday. Thank you for the contribution

@linxGnu linxGnu changed the title Build: support cleaner link flags Build: support minimal link flags Jan 20, 2023
@linxGnu linxGnu merged commit bb759a8 into linxGnu:master Jan 20, 2023
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.

2 participants