CMake: Move resolve dependency macros under velox_ prefix #11392
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
There was a problem hiding this comment.
Yeah good idea, thanks. I can see issues when getting arrow via fetch content without the prefix (arrow has 'resolve_dependency' as well).
Before we merge we will need to ping some people of the downstream projects I know are using our CMake that will need to adapt to this.
Nimble @pedro
Presto @majetideepak
Not sure who to ping for gluten?
Edit: velox_exec_test is failing on main.
There was a problem hiding this comment.
This pr makes sense. @assignUser, I just checked Gluten. It only requires few code changes to adapt to this. We will do that in Gluten after this pr is merged.
assignUser
added
the
ready-to-merge
|
@cryos CI is red. Would you rebase to see if CI passes. If not, please, investigate the failures. |
cryos
force-pushed
the
cmake-namespace-resolve-deps
branch
from
014b6d0 to
c5f06ba
Compare
Rebasing didn't help, I don't see anything related to this change in the test that is failing. The same test appears to be failing on other unrelated PRs that are active right now too. Anything related to this PR should fail early as it is all related to finding/building dependencies and linking to them. Maybe @assignUser will have a better overview, it would appear that it is seeing an empty file. |
|
It's been fixed on main now: #11436 |
It is best practice in CMake to use a project prefix for project specific macros, functions and variasbles. This makes it easier to compose projects in larger builds as well as differentiate general functions from project specific functions. This moves several macros and it will require some simple migration of any CMake code reusing the resolve dependency macros: * build_dependency -> velox_build_dependency * list_subdirs -> velox_list_subdirs * resolve_dependency -> velox_resolve_dependency * resolve_dependency_url -> velox_resolve_dependency_url * set_source -> velox_set_source * set_with_default -> velox_set_with_default
This is not used and in review it was suggested that we remove it.
cryos
force-pushed
the
cmake-namespace-resolve-deps
branch
from
c5f06ba to
6a83ba3
Compare
|
Everything looks green now. |
|
@mbasmanova can we merge this before something else causes conflicts? |
|
@kevinwilfong Kevin, would you help merge this PR? |
|
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
It is best practice in CMake to use a project prefix for project specific macros, functions and variables. This makes it easier to compose projects in larger builds as well as differentiate general functions from project specific functions.
This moves several macros and it will require some simple migration of any CMake code reusing the resolve dependency macros:
This should be a simple change as the other changes are just to adapt to the changed macro names.