Skip to content

Build velox_vector_test_lib if VELOX_BUILD_TEST_UTILS is ON #2531

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
Closed
beroyfb wants to merge 1 commit into from
Closed

Build velox_vector_test_lib if VELOX_BUILD_TEST_UTILS is ON #2531

beroyfb wants to merge 1 commit into from

Conversation

@beroyfb
Copy link
Contributor

@beroyfb beroyfb commented Sep 13, 2022 •
edited by mbasmanova
Loading

This allows Velox applications to use utilities such as VectorMaker without building all Velox tests.

@beroyfb beroyfb requested review from kgpai and mbasmanova September 13, 2022 18:44
@netlify
Copy link

netlify bot commented Sep 13, 2022 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 7169b4f
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6320f6754d03680008e7cecb

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 13, 2022
@beroyfb beroyfb force-pushed the velox_vector_test_lib_TEST_UTILS branch from 275f06c to 3ac0b2b Compare September 13, 2022 18:48
@mbasmanova mbasmanova changed the title Building velox_vector_test_lib if VELOX_BUILD_TEST_UTILS is ON Build velox_vector_test_lib if VELOX_BUILD_TEST_UTILS is ON Sep 13, 2022
@beroyfb beroyfb force-pushed the velox_vector_test_lib_TEST_UTILS branch from 3ac0b2b to 3c6d065 Compare September 13, 2022 18:51
mbasmanova
mbasmanova approved these changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@beroyfb Looks good to me, but CI is red. Please, fix before landing.

I edited the PR title and description to comply with the guidelines: https://github.com/facebookincubator/velox/blob/main/CONTRIBUTING.md#code

Please, take a moment to review these for future PRs.

@beroyfb beroyfb force-pushed the velox_vector_test_lib_TEST_UTILS branch from 3c6d065 to b41b4b3 Compare September 13, 2022 18:53
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 13, 2022

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

kgpai
kgpai reviewed Sep 13, 2022
View reviewed changes
velox/vector/tests/CMakeLists.txt
# limitations under the License.
add_library(velox_vector_test_lib VectorMaker.cpp VectorTestBase.cpp)
if(${VELOX_BUILD_TEST_UTILS})
add_library(velox_vector_test_lib VectorMaker.cpp VectorTestBase.cpp)
Copy link
Contributor

@kgpai kgpai Sep 13, 2022

Choose a reason for hiding this comment

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

These libraries are required even with VELOX_BUILD_TESTING ; I suspect This is why xsimd doesnt get picked up as its header only.

@beroyfb beroyfb force-pushed the velox_vector_test_lib_TEST_UTILS branch from b41b4b3 to bbd2d88 Compare September 13, 2022 20:31
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 13, 2022

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

@kgpai
Copy link
Contributor

kgpai commented Sep 13, 2022

FYI You are building with -DVELOX_ENABLE_BENCHMARKS_BASIC=ON , but benchmarks require : velox_vector_test_lib to be built; however they wont be built because VELOX_BUILD_TEST_UTILS isnt on ; So the fix is to set VELOX_BUILD_TEST_UTILS to be ON if benchmarks is enabled.

@beroyfb beroyfb force-pushed the velox_vector_test_lib_TEST_UTILS branch 2 times, most recently from 34b1477 to b0382b8 Compare September 13, 2022 21:29
@beroyfb
Building velox_vector_test_lib if VELOX_BUILD_TEST_UTILS is ON
7169b4f 
This will allow the applications using Velox to be able to use
utilities in vector lib such as vector maker.
@beroyfb beroyfb force-pushed the velox_vector_test_lib_TEST_UTILS branch from b0382b8 to 7169b4f Compare September 13, 2022 21:30
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 13, 2022

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

@mbasmanova
Copy link
Contributor

mbasmanova commented Sep 13, 2022

FYI You are building with -DVELOX_ENABLE_BENCHMARKS_BASIC=ON , but benchmarks require : velox_vector_test_lib to be built; however they wont be built because VELOX_BUILD_TEST_UTILS isnt on ; So the fix is to set VELOX_BUILD_TEST_UTILS to be ON if benchmarks is enabled.

@kgpai Should we have some logic somewhere to set VELOX_BUILD_TEST_UTILS to ON if VELOX_ENABLE_BENCHMARKS_BASIC is ON?

@majetideepak
Copy link
Collaborator

majetideepak commented Sep 13, 2022

Can we please merge this PR #2439 which addresses this issue in a clean way?

majetideepak
majetideepak requested changes Sep 13, 2022
View reviewed changes
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

Does #2439 help solve this?

kgpai
kgpai reviewed Sep 13, 2022
View reviewed changes
velox/vector/tests/CMakeLists.txt
add_library(velox_vector_test_lib VectorMaker.cpp VectorTestBase.cpp)
if(${VELOX_BUILD_TEST_UTILS}
OR ${VELOX_BUILD_TESTING}
OR ${VELOX_ENABLE_BENCHMARKS_BASIC})
Copy link
Contributor

@kgpai kgpai Sep 13, 2022

Choose a reason for hiding this comment

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

Its better to have this logic in the parent CMakeLists rather than have this if logic here as it makes it much harder to reason about what gets built etc.

if (${VELOX_ENABLE_BENCHMARKS_BASIC})
set($VEOX_BUILD_TEST_UTILS ON)
..
endif()

@kgpai
Copy link
Contributor

kgpai commented Sep 13, 2022

@mbasmanova @majetideepak Sorry saw your comments now.
As pointed above #2439 also does same thing.

@beroyfb
Copy link
Contributor Author

beroyfb commented Sep 13, 2022

Sure, either way work for me. I just have a big PR that needs this fix: prestodb/presto#18327

@beroyfb
Copy link
Contributor Author

beroyfb commented Sep 14, 2022

Tried my large PR on #2439 and failed with the same error: ld: library not found for -lvelox_exec_test_util

@mbasmanova
Copy link
Contributor

mbasmanova commented Sep 14, 2022

Tried my large PR on #2439 and failed with the same error: ld: library not found for -lvelox_exec_test_util

This is sad. I believe you were getting a different error (for a different library) earlier, no?

prestodb/presto#18327 (comment)

ld: library not found for -lvelox_vector_test_lib

CC: @majetideepak @kgpai

@mbasmanova
Copy link
Contributor

mbasmanova commented Sep 14, 2022

@beroyfb I wonder if the failure is because the library was renamed to velox_exec_test_lib. Would you try renaming and rebuilding?

@beroyfb
Copy link
Contributor Author

beroyfb commented Sep 14, 2022

@mbasmanova great point and you right! The following combination made it work:

        velox_exec_test_lib
        velox_vector_test_lib

So we are all good.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Dec 2, 2022

Hi @beroyfb!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@mbasmanova mbasmanova closed this Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kgpai kgpai kgpai left review comments

@majetideepak majetideepak majetideepak requested changes

@mbasmanova mbasmanova mbasmanova approved these changes

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@beroyfb @facebook-github-bot @kgpai @mbasmanova @majetideepak