Don't link Vulkan by default #526

Ralith · 2021-12-23T19:06:21Z

This was found to be unreasonably disruptive to downstream CI configurations. Fixes #525.

I opted to additionally enable loaded by default, and reposition Entry::load above Entry::linked, so that a working configuration is available and easily found out of the box.

ash/src/entry.rs

aloucks · 2021-12-23T20:47:56Z

examples/Cargo.toml

@@ -7,5 +7,5 @@ edition = "2018"
 [dependencies]
 winit = "0.25.0"
 image = "0.10.4"
-ash = { path = "../ash" }
+ash = { path = "../ash", default-features = false, features = ["linked", "debug"] }


It might be helpful to have some explanation as to why the examples are using linked instead loaded. I assume this is because the examples are requiring the validation layer from the SDK. If that's not the case, the examples should probably use loaded to minimize friction.

Added a note.

@aloucks The note is nice, but keep in mind that dynamic linking or (dynamic) runtime loading has no impact on being able to use the validation layers. It "just so happens" that both the .lib (needed for dynamic linking on Windows) (on Linux the runtime .so is used for this) and validation layers ship with the SDK.

FWIW I thought the examples would use linked to both showcase this feature, and because examples are generally ran on the same machine (ie. build environment == runtime environment) so it makes sense that the user intends to develop Vulkan applications, hence has the ability to both "develop" (with the SDK) and run them.

MarijnS95 · 2021-12-23T21:37:46Z

Does the CI test job still need to install libvulkan-dev now?

Ralith · 2021-12-23T22:08:01Z

Yes, to allow tests/examples to be built with the linked feature enabled.

aloucks · 2021-12-23T22:57:30Z

ash/src/entry.rs

    /// Load default Vulkan library for the current platform
    ///
+    /// Prefer this over [`linked`](Self::linked) when your application can gracefully handle
+    /// environments that lack Vulkan support, and when the build environment might not have the
+    /// Vulkan loader available.


It might be helpful to rephrase this in such a way that emphasizes the impact on downstream dependencies.

Entry::loaded should be preferred unless you explicitly want to impose a dependency on the Vulkan SDK (e.g. perhaps you depend on validation layers). For example, bevy depends on wgpu. If wgpu decided to used Entry::linked, every developer who attempted to experiment with bevy would need to install the Vulkan SDK or face compile time linker errors.

Adjusted the wording to make the build environment requirements clearer.

This was found to be unreasonably disruptive to downstream CI configurations.

Jasper-Bekkers

I think this reverts back to the old behavior so I can approve it. @MaikKlein I think it would make sense to push a new release with this fix as soon as it's merged. What do you think?

MarijnS95 · 2021-12-27T10:31:18Z

ash/src/entry.rs

+    /// Compared to [`load`](Self::load), this is infallible, but requires that the build
+    /// environment have Vulkan development packages installed (e.g. the Vulkan SDK, or Ubuntu's
+    /// libvulkan-dev), and prevents the resulting binary from starting in environments that do not
+    /// support Vulkan.


s/support Vulkan/ship with Vulkan libraries?

ash/src/entry.rs

MarijnS95 · 2021-12-27T10:37:40Z

examples/Cargo.toml

@@ -7,5 +7,5 @@ edition = "2018"
 [dependencies]
 winit = "0.25.0"
 image = "0.10.4"
-ash = { path = "../ash" }
+ash = { path = "../ash", default-features = false, features = ["linked", "debug"] }


@aloucks The note is nice, but keep in mind that dynamic linking or (dynamic) runtime loading has no impact on being able to use the validation layers. It "just so happens" that both the .lib (needed for dynamic linking on Windows) (on Linux the runtime .so is used for this) and validation layers ship with the SDK.

FWIW I thought the examples would use linked to both showcase this feature, and because examples are generally ran on the same machine (ie. build environment == runtime environment) so it makes sense that the user intends to develop Vulkan applications, hence has the ability to both "develop" (with the SDK) and run them.

MarijnS95 · 2021-12-27T10:42:15Z

ash-window/Cargo.toml

@@ -22,6 +22,7 @@ raw-window-metal = "0.1"

 [dev-dependencies]
 winit = "0.19.4"
+ash = { path = "../ash", version = "0.34", default-features = false, features = ["linked"] }


That's a nice way to satisfy required-features = ["ash/linked"] :)

Ralith requested review from Jasper-Bekkers and MaikKlein December 23, 2021 19:06

Ralith force-pushed the no-default-link branch from 5ba861c to 64ee2fe Compare December 23, 2021 19:07

Ralith mentioned this pull request Dec 23, 2021

Don't require explicitly linking libvulkan by default #525

Closed

Ralith force-pushed the no-default-link branch from 64ee2fe to cc45469 Compare December 23, 2021 19:12

MarijnS95 reviewed Dec 23, 2021

View reviewed changes

ash/src/entry.rs Outdated Show resolved Hide resolved

ash/src/entry.rs Show resolved Hide resolved

Ralith force-pushed the no-default-link branch 3 times, most recently from 887e355 to 71a63c4 Compare December 23, 2021 19:34

MarijnS95 reviewed Dec 23, 2021

View reviewed changes

ash/src/entry.rs Outdated Show resolved Hide resolved

Ralith force-pushed the no-default-link branch 2 times, most recently from 203ec3d to 44ed205 Compare December 23, 2021 19:59

aloucks reviewed Dec 23, 2021

View reviewed changes

Ralith force-pushed the no-default-link branch from 44ed205 to 4cdc097 Compare December 23, 2021 22:16

aloucks reviewed Dec 23, 2021

View reviewed changes

Ralith force-pushed the no-default-link branch from 4cdc097 to a583f6c Compare December 23, 2021 23:04

MaikKlein approved these changes Dec 24, 2021

View reviewed changes

Ralith force-pushed the no-default-link branch from a583f6c to 3f0fdef Compare December 24, 2021 18:21

Don't link Vulkan by default

48621f5

This was found to be unreasonably disruptive to downstream CI configurations.

Ralith force-pushed the no-default-link branch from 3f0fdef to 48621f5 Compare December 24, 2021 18:21

Jasper-Bekkers approved these changes Dec 26, 2021

View reviewed changes

MarijnS95 mentioned this pull request Dec 27, 2021

Update ash to 0.35 EmbarkStudios/ash-molten#63

Merged

MarijnS95 approved these changes Dec 27, 2021

View reviewed changes

Apply suggestions from code review

af9549f

MarijnS95 merged commit 7cf3b4f into ash-rs:master Dec 27, 2021

Jasper-Bekkers mentioned this pull request Dec 27, 2021

Publish new ash release #532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't link Vulkan by default #526

Don't link Vulkan by default #526

Ralith commented Dec 23, 2021 •

edited

Loading

aloucks Dec 23, 2021

Ralith Dec 23, 2021

MarijnS95 Dec 27, 2021

MarijnS95 commented Dec 23, 2021

Ralith commented Dec 23, 2021

aloucks Dec 23, 2021

Ralith Dec 23, 2021

Jasper-Bekkers left a comment

MarijnS95 Dec 27, 2021

MarijnS95 Dec 27, 2021

MarijnS95 Dec 27, 2021

Don't link Vulkan by default #526

Don't link Vulkan by default #526

Conversation

Ralith commented Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarijnS95 commented Dec 23, 2021

Ralith commented Dec 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jasper-Bekkers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith commented Dec 23, 2021 •

edited

Loading