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

[VL] Use Velox's monolithic build #6731

Merged
merged 10 commits into from
Aug 8, 2024
Merged

Conversation

PHILO-HE
Copy link
Contributor

@PHILO-HE PHILO-HE commented Aug 6, 2024

What changes were proposed in this pull request?

Monolithic build was introduced into velox by facebookincubator/velox#9948. It will produce a unified velox lib, libvelox.a. With it, linking velox libs in Gluten becomes less complex and easier to maintain.

We also found Velox's test util libs are still separate. They are required when Gluten --build_tests=ON. In this case, we still keep linking these separate libs.

How was this patch tested?

CI.

Copy link

github-actions bot commented Aug 6, 2024

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@PHILO-HE PHILO-HE force-pushed the use-monolithic-build branch from 3cc41f5 to 6543e9d Compare August 7, 2024 01:21
${VELOX_BUILD_PATH}/lib/libvelox.a)

if(BUILD_TESTS)
add_library(facebook::velox::dbgen STATIC IMPORTED)
Copy link
Contributor

Choose a reason for hiding this comment

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

@PHILO-HE For the testing libraries, they are still separate libraries, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xumingming, yes, these test libs are still separate.

Copy link
Contributor

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

Thanks!

set_target_properties(
facebook::velox::dwio_common_test
PROPERTIES
IMPORTED_LOCATION
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider using a helper function to simply the code.

@PHILO-HE PHILO-HE force-pushed the use-monolithic-build branch from 6543e9d to 85e8d0f Compare August 7, 2024 01:42
@gaoyangxiaozhu
Copy link
Contributor

cool feature related to build!

@PHILO-HE PHILO-HE force-pushed the use-monolithic-build branch from e528c20 to 0bd8c8d Compare August 7, 2024 06:43
@PHILO-HE PHILO-HE force-pushed the use-monolithic-build branch from 0bd8c8d to 4dc86d3 Compare August 7, 2024 07:00
@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Aug 8, 2024

@zhztheplayer, @xumingming, do you have any comments?
I checked the overall size of all separate velox libs when monolithic build is off. It roughly equals to the size of libvelox.a.

Copy link
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

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

Thanks for the effort!

@PHILO-HE PHILO-HE merged commit a6d8347 into apache:main Aug 8, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants