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.

Sign up for GitHub

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

Jump to bottom

Allow plugins to add pkg-config dependencies to rocksdb.pc #9198

Closed
wants to merge 2 commits into from
Closed

Allow plugins to add pkg-config dependencies to rocksdb.pc #9198

wants to merge 2 commits into from

Conversation

metaspace
Copy link
Contributor

This patch fixes an issue that occur when dependencies of plugins are not
installed to the same prefix as librocksdb. Because plugin dependencies are
declared in the Libs field of rocksdb.pc, programs that link against
librocksdb with pkg-config --libs rocksdb will link with -L flag for the
path of librocksdb only. This patch allows plugin dependencies to be declared in
the Requires field of rocksdb.pc, so that pkg-config will correctly provide
-L flags for dependencies of plugins that are installed in other locations.

@metaspace metaspace mentioned this pull request Nov 22, 2021
Merged
@metaspace
Allow plugins to add pkg-config dependencies to rocksdb.pc
fcae19f 
This patch fixes an issue that occur when dependencies of plugins are not
installed to the same prefix as librocksdb. Because plugin dependencies are
declared in the `Libs` field of rocksdb.pc, programs that link against
librocksdb with `pkg-config --libs rocksdb` will link with `-L` flag for the
path of librocksdb only. This patch allows plugin dependencies to be declared in
the `Requires` field of rocksdb.pc, so that pkg-config will correctly provide
`-L` flags for dependencies of plugins that are installed in other locations.

Signed-off-by: Andreas Hindborg <andreas.hindborg@wdc.com>
@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ajkr
ajkr approved these changes Nov 22, 2021
View reviewed changes
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@facebook-github-bot
Copy link
Contributor

@ajkr merged this pull request in 0745622.

@riversand963
Copy link
Contributor

@metaspace this PR causes the following command to break on my devserver

$ROCKSDB_NO_FBCODE=1 ROCKSDB_PLUGINS="dedupfs hdfs" make -j20 db_stress
Please specify at least one package name on the command line.
Makefile:259: *** pkg-config failed.  Stop.

Could you take a look?

@riversand963 riversand963 reopened this Dec 1, 2021
@metaspace
Copy link
Contributor Author

@metaspace this PR causes the following command to break on my devserver

$ROCKSDB_NO_FBCODE=1 ROCKSDB_PLUGINS="dedupfs hdfs" make -j20 db_stress
Please specify at least one package name on the command line.
Makefile:259: *** pkg-config failed.  Stop.

Could you take a look?

Yes, will have a look asap.

@riversand963
Copy link
Contributor

@metaspace this PR causes the following command to break on my devserver

$ROCKSDB_NO_FBCODE=1 ROCKSDB_PLUGINS="dedupfs hdfs" make -j20 db_stress
Please specify at least one package name on the command line.
Makefile:259: *** pkg-config failed.  Stop.

Could you take a look?

Yes, will have a look asap.

@metaspace oh btw, when testing, maybe the command to use is

ROCKSDB_NO_FBCODE=1 ROCKSDB_PLUGINS="dedupfs" make -j20 db_stress

that's because hdfs is not a plugin yet.

@facebook-github-bot
Copy link
Contributor

@metaspace has updated the pull request. You must reimport the pull request before landing.

@metaspace
Copy link
Contributor Author

@metaspace this PR causes the following command to break on my devserver

$ROCKSDB_NO_FBCODE=1 ROCKSDB_PLUGINS="dedupfs hdfs" make -j20 db_stress
Please specify at least one package name on the command line.
Makefile:259: *** pkg-config failed.  Stop.

Could you take a look?

Yes, will have a look asap.

Missed a space in a string that was supposed to be empty. Pushed a fix as a separate commit to this PR.

@metaspace
Fix a bug that occur when plugin pkg-config requirements are empty
d4ded4d 
Signed-off-by: Andreas Hindborg <andreas.hindborg@wdc.com>
@ajkr
Copy link
Contributor

ajkr commented Dec 1, 2021

Sorry for the confusion. We only support merging once per PR. Are you able to open a new PR?

@ajkr ajkr closed this Dec 1, 2021
@metaspace metaspace mentioned this pull request Dec 1, 2021
Closed
@metaspace
Copy link
Contributor Author

Sure. New PR in #9238. Sorry for the mess.

facebook-github-bot pushed a commit that referenced this pull request Dec 21, 2021
@metaspace @facebook-github-bot
Fix a bug that occur when plugin pkg-config requirements are empty (#…
2e51b33 
…9238)

Summary:
Fix a bug introduced by #9198. The bug is triggered when a plugin does not provide any pkg-config requirements.

Pull Request resolved: #9238

Reviewed By: riversand963

Differential Revision: D32771406

Pulled By: ajkr

fbshipit-source-id: 79301871a8bf4e624d5e5eb9d219d7f13948c64d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajkr ajkr ajkr approved these changes

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@metaspace @facebook-github-bot @riversand963 @ajkr