Skip to content

Conversation

@emmenlau
Copy link
Member

This PR adds a few improvements in cmake dependency handling. Previously, the cmake build did not pass dependencies so well downstream to dependent projects. This PR ensures that all possible dependencies (including thriftz and thriftnb) are passed correctly.

This PR also removes the find-script for libevent. Generally libevent can nowadays often be found with the cmake config. However this may be a breaking change(?)

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

Copy link
Contributor

@ulidtko ulidtko left a comment

Choose a reason for hiding this comment

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

LGTM (though ThriftConfig.cmake.in doesn't look that good to me...)

Copy link
Contributor

@ulidtko ulidtko left a comment

Choose a reason for hiding this comment

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

Actually, I think you'll need to bring back the FindLibevent.cmake file, @emmenlau. It's not included in CMake distribution, this will break.

@emmenlau emmenlau force-pushed the bda_cmake_improvements branch from c1b6afe to 7876758 Compare November 19, 2020 14:19
@emmenlau
Copy link
Member Author

@ulidtko Yes you are right, I've come to the same thinking. I'm using libevent with cmake, but other people may use it from their distro. Re-added FindLibevent.cmake. Please review?

Copy link
Contributor

@ulidtko ulidtko left a comment

Choose a reason for hiding this comment

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

Yes sure, LGTM now.

@emmenlau emmenlau force-pushed the bda_cmake_improvements branch 2 times, most recently from 49ad4e2 to 97e8f8d Compare November 25, 2020 08:57
@emmenlau
Copy link
Member Author

I've re-added all the support for libevent found via the provided cmake find script, but also added detection for the newer cmake configuration via targets. Basically, cmake should now support libevent in all cases.

Lets see if the CI agrees.

@emmenlau emmenlau force-pushed the bda_cmake_improvements branch from 97e8f8d to 0e84ca5 Compare November 25, 2020 13:21
@emmenlau
Copy link
Member Author

The Travis CI build errors are from autotools builds and should be unrelated. Please consider for merge.

@emmenlau emmenlau force-pushed the bda_cmake_improvements branch from 0e84ca5 to 5556275 Compare December 14, 2020 10:51
Copy link
Member

@zeshuai007 zeshuai007 left a comment

Choose a reason for hiding this comment

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

LGTM.

@emmenlau emmenlau force-pushed the bda_cmake_improvements branch from 5556275 to c981d93 Compare January 13, 2021 15:19
@emmenlau emmenlau force-pushed the bda_cmake_improvements branch from c981d93 to 7553fc5 Compare February 12, 2021 10:10
@emmenlau
Copy link
Member Author

Rebased on latest master. I plan to merge this PR ASAP (if CI remains green against latest master).

@emmenlau emmenlau merged commit e5e7d1d into apache:master Feb 12, 2021
@emmenlau emmenlau deleted the bda_cmake_improvements branch February 12, 2021 12:04
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.

3 participants