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

Remove libvulkan.so from 3P builds and disable linux installer #5205

Merged
merged 5 commits into from
Mar 27, 2025

Conversation

niranjanyardi
Copy link
Contributor

b/406533849

@niranjanyardi niranjanyardi marked this pull request as ready for review March 26, 2025 20:08
@niranjanyardi niranjanyardi requested a review from a team as a code owner March 26, 2025 20:08
Copy link
Contributor

@andrewsavage1 andrewsavage1 left a comment

Choose a reason for hiding this comment

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

This might be fine, but it might just push our problems til later as it waits to compile the vulkan sources (I'm assuming that angle_shared_libvulkan=false just compiles vulkan as a static library instead of a shlib). I would think there's some flag to ensure we don't depend on vulkan at all.
Regardless, this seems fine to get in and address later, just please have Sherry take a quick look

@niranjanyardi niranjanyardi marked this pull request as draft March 26, 2025 20:31
@niranjanyardi
Copy link
Contributor Author

niranjanyardi commented Mar 26, 2025

This might be fine, but it might just push our problems til later as it waits to compile the vulkan sources (I'm assuming that angle_shared_libvulkan=false just compiles vulkan as a static library instead of a shlib). I would think there's some flag to ensure we don't depend on vulkan at all. Regardless, this seems fine to get in and address later, just please have Sherry take a quick look

From the delcaration in third_party/angle/gni/angle.gni i thought it compiles statically only on mac - i might be fully wrong
Vulkan loader is statically linked on Mac. http://anglebug.com/40096682
angle_shared_libvulkan = !is_mac

@andrewsavage1
Copy link
Contributor

This might be fine, but it might just push our problems til later as it waits to compile the vulkan sources (I'm assuming that angle_shared_libvulkan=false just compiles vulkan as a static library instead of a shlib). I would think there's some flag to ensure we don't depend on vulkan at all. Regardless, this seems fine to get in and address later, just please have Sherry take a quick look

From the delcaration in third_party/angle/gni/angle.gni i thought it compiles statically only on mac - i might be fully wrong Vulkan loader is statically linked on Mac. http://anglebug.com/40096682 angle_shared_libvulkan = !is_mac

My point is that making vulkan compile statically will just make it link later and possibly push the issues to that later linking step instead of removing them

Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

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

We can remove this from Android deps as well actually

@niranjanyardi niranjanyardi marked this pull request as ready for review March 26, 2025 23:55
@niranjanyardi
Copy link
Contributor Author

We can remove this from Android deps as well actually

ack, will do in a follow up

@niranjanyardi niranjanyardi changed the title Remove libvulkan.so from 3P builds Remove libvulkan.so from 3P builds and disable linux installer Mar 26, 2025
@niranjanyardi niranjanyardi requested a review from kaidokert March 26, 2025 23:56
Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

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

lgtm

@niranjanyardi niranjanyardi enabled auto-merge (squash) March 26, 2025 23:57
@niranjanyardi niranjanyardi disabled auto-merge March 27, 2025 00:08
Copy link
Contributor

@sherryzy sherryzy left a comment

Choose a reason for hiding this comment

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

lgtm.

@niranjanyardi niranjanyardi merged commit 78cb4d2 into youtube:main Mar 27, 2025
151 checks passed
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.

4 participants