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

[GLUTEN-7208][VL]fix: loading libvelox.so failed when using static glog #7209

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

JinHelin404
Copy link
Contributor

What changes were proposed in this pull request?

(Fixes: #7208)

How was this patch tested?

@github-actions github-actions bot added the VELOX label Sep 12, 2024
Copy link

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:

@JinHelin404
Copy link
Contributor Author

@PHILO-HE Hi. Could you please help to review this? 4434 Introduced gluten link glog, which will cause 7208.

@PHILO-HE
Copy link
Contributor

@JinHelin404, thanks for your pr! According to your change, linking glog is only required when ENABLE_GLUTEN_VCPKG is ON. Could you clarify a bit?

@JinHelin404
Copy link
Contributor Author

@PHILO-HE thanks for your reviewing! Actually I also want to figure out why glog is linked in your pr 4434 . This pr fix issue when vcpkg is on so I put linking glog in the if(ENABLE_GLUTEN_VCPKG) conditional statement.

@PHILO-HE
Copy link
Contributor

@PHILO-HE thanks for your reviewing! Actually I also want to figure out why glog is linked in your pr 4434 . This pr fix issue when vcpkg is on so I put linking glog in the if(ENABLE_GLUTEN_VCPKG) conditional statement.

@JinHelin404, I remember linking glog is necessary when debug build is enabled (at least it's true at that code point).

The issue of both statically and dynamically linking can happen when static glog lib is used, regardless of vcpkg is enabled or not (enabling vcpkg definitely makes static glog lib be used). I think we can remove the below check to also hide those conflict symbols when vcpkg is not enabled:
https://github.com/apache/incubator-gluten/blob/main/cpp/core/CMakeLists.txt#L216.

@JinHelin404
Copy link
Contributor Author

The issue of both statically and dynamically linking can happen when static glog lib is used, regardless of vcpkg is enabled or not (enabling vcpkg definitely makes static glog lib be used). I think we can remove the below check to also hide those conflict symbols when vcpkg is not enabled:

@PHILO-HE Thanks for your advice. I agree that we can always hide these symbols and I have fixed this pr.

@JinHelin404 JinHelin404 changed the title [VL] fix loading libvelox.so failed when using static glog [GLUTEN-7208][VL]fix: loading libvelox.so failed when using static glog Sep 13, 2024
Copy link

#7208

@PHILO-HE
Copy link
Contributor

@JinHelin404, please fix cmake format issue (you can just apply the patch generated in that failed CI log). BTW, please verify this patch in your local to ensure it can really fix your encountered issue .

Copy link
Contributor

@PHILO-HE PHILO-HE 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 your fix!

@PHILO-HE PHILO-HE merged commit 7861829 into apache:main Sep 13, 2024
44 checks passed
sharkdtu pushed a commit to sharkdtu/gluten that referenced this pull request Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using the static glog library will cause the loading of libvelox.so to fail
2 participants