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

Fix WIN32 .lib installs #182

Closed
juan-lunarg opened this issue Apr 11, 2023 · 4 comments · Fixed by #186 or #191
Closed

Fix WIN32 .lib installs #182

juan-lunarg opened this issue Apr 11, 2023 · 4 comments · Fixed by #186 or #191
Assignees

Comments

@juan-lunarg
Copy link
Contributor

juan-lunarg commented Apr 11, 2023

The Windows install target currently installs various .lib files. It should not, as these are not shipped by the SDK.

The SDK is also planning on not shipping 32-bit artifacts for the VK_LAYER_KHRONOS_shader_object (though it will continue to ship 32-bit VK_LAYER_KHRONOS_synchronization2 artifacts). So the 32-bit install target should not ship Bin\VkLayer_khronos_shader_object.dll, Bin\VkLayer_khronos_shader_object.json, or ,Bin\VkLayer_khronos_shader_object.pdb.

@juan-lunarg juan-lunarg self-assigned this Apr 11, 2023
@juan-lunarg juan-lunarg changed the title Fix WIN32 install Fix WIN32 .lib installs Apr 11, 2023
jeremyg-lunarg pushed a commit that referenced this issue Apr 13, 2023
Layers should be `MODULE`s not `SHARED`

They are only meant to be loaded via runtime means like dlopen.

The main practical effect of this is that CMake now doesn't
install a corresponding .lib file. Since users aren't trying to
link against the library.

closes #182
juan-lunarg added a commit that referenced this issue Apr 13, 2023
Layers should be `MODULE`s not `SHARED`

They are only meant to be loaded via runtime means like dlopen.

The main practical effect of this is that CMake now doesn't
install a corresponding .lib file. Since users aren't trying to
link against the library.

closes #182
@lunarpapillo
Copy link
Contributor

The latest SDK build still shows issues: http://erusea:8080/view/Manual/job/manual-sdk/529/console

WARNING: generated BOM C:\j\msdk\vulkantest-results\bom\qt-repo-BOM.txt added entries not in reference BOM C:\j\msdk\build\Vulkan-SDK-Packaging\repo\SDK_creation\Windows\qt-repo-BOM.txt:
+ CoreSDK,Bin\VkLayer_khronos_shader_object.dll
+ CoreSDK,Bin\VkLayer_khronos_shader_object.json
+ CoreSDK,Bin\VkLayer_khronos_shader_object.pdb
+ CoreSDK,Include\vulkan\vulkan_extension_inspection.hpp
+ CoreSDK,Lib\VkLayer_khronos_shader_object.lib
+ CoreSDK32,Bin32\VkLayer_khronos_shader_object.dll
+ CoreSDK32,Bin32\VkLayer_khronos_shader_object.json
+ CoreSDK32,Bin32\VkLayer_khronos_shader_object.pdb

I observe that VkLayer_khronos_shader_object.lib is still being shipped (I suspect it was overlooked in the earlier fix.

I also note that the 32-bit install target still ships VkLayer_khronos_shader_object, though it's not supposed to; see the SDK design document at https://docs.google.com/document/d/14HULgqYy4k3-UDRKbUr_jWQcRGX4RGPN05iLoz0jSB0 . In the interest of keeping install control at the repository level, that means this repository must not ship the VkLayer_khronos_shader_object files in a 32-bit build.

@lunarpapillo lunarpapillo reopened this Apr 13, 2023
@juan-lunarg
Copy link
Contributor Author

I observe that VkLayer_khronos_shader_object.lib is still being shipped (I suspect it was overlooked in the earlier fix.

I just checked the build logs from github actions:

https://github.com/KhronosGroup/Vulkan-ExtensionLayer/actions/runs/4690667744/jobs/8314079421
https://github.com/KhronosGroup/Vulkan-ExtensionLayer/actions/runs/4690667744/jobs/8314079208

No .lib files are being installed. I cannot reproduce that.

I also note that the 32-bit install target still ships VkLayer_khronos_shader_object, though it's not supposed to; see the SDK design document at https://docs.google.com/document/d/14HULgqYy4k3-UDRKbUr_jWQcRGX4RGPN05iLoz0jSB0 . In the interest of keeping install control at the repository level, that means this repository must not ship the VkLayer_khronos_shader_object files in a 32-bit build.

Sure but why?

@juan-lunarg
Copy link
Contributor Author

Just had Karen explain to me the rational. I have a new PR up that will remove 32 bit support.

@lunarpapillo
Copy link
Contributor

Sure but why?

We debated in an SDK planning video meeting the need for 32-bit. Recalling my notes, the general thoughts went (:heavy_plus_sign: marks arguments for 32-bit support, :heavy_minus_sign: marks arguments against):

  • ➕ we ship 32-bit for everything else
  • ➕ we believe there are 32-bit applications still under active development who cannot or will not make the jump to 64-bit
  • ➖ it's unlikely a 32-bit application still in active development would want to completely revise their pipeline and shader structure to use this new object
  • ➖ if we ship 32-bit once, we have to ship 32-bit forever

IIRC, we decided to ship 64-bit only for now, and reconsider if and when someone complains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants