Skip to content

[spirv-cross,spirv-headers,spirv-tools] 2021-01-15 update#15831

Merged
strega-nil merged 14 commits intomicrosoft:masterfrom
barcharcraz:spirv-tools-update
Apr 20, 2021
Merged

[spirv-cross,spirv-headers,spirv-tools] 2021-01-15 update#15831
strega-nil merged 14 commits intomicrosoft:masterfrom
barcharcraz:spirv-tools-update

Conversation

@barcharcraz
Copy link
Contributor

Describe the pull request

  • update spirv-tools to version 2020.6
  • update spirv-headers to version 1.5.4.raytracing.fixed (a required update to get spirv-tools to build)
  • update spirv-cross to version 2021-01-15

Additionally removed several no-longer-needed patches from spirv-tools.

I think that spirv-tools and spirv-cross actually will build shared libs on windows now, but I have not gone and gotten that working.

@JonLiu1993 JonLiu1993 assigned JonLiu1993 and JackBoosY and unassigned JonLiu1993 Jan 25, 2021
@JackBoosY JackBoosY added the category:port-update The issue is with a library, which is requesting update new revision label Jan 25, 2021
@JackBoosY
Copy link
Contributor

Most of them are good, but I have some questions:

  1. spirv-cross generated additional cmake configuration files share/spirv_cross_c/cmake/spirv_cross_cConfig.cmake, they should be deleted.
  2. spirv-tools generated two pkgconfig lib/pkgconfig/SPIRV-Tools-shared.pc and lib/pkgconfig/SPIRV-Tools.pc at the same time. Is this expected?
  3. spirv-tools generated shared library lib/libSPIRV-Tools-shared.so on x64-linux, that should be incorrect.
  4. Note glslang generates spirv binary and header files(include/SPIRV/GLSL.ext.AMD.h, lib/SPIRV.lib and tools/spirv-remap.exe), which may conflict with spirv hidden files or violate our policy (use the internal port of vcpkg instead of generating a separate copy).

@barcharcraz
Copy link
Contributor Author

why should we delete the config files from spirv-cross?

I'll check the package config files later today

why is the library incorrect on linux, I didn't touch the library names at all and I don't want to force the package to generate libraries with a linuxey name if it doesn't do that on it's own (I don't want to change the library names from upstream).

@barcharcraz
Copy link
Contributor Author

also I didn't touch glslang, I will check if spirv has an embedded dependency on glslang, but if not I'm not sure how to proceed there. I don't want to modify either package if they both generate those headers from the glsl specification xml.

@JackBoosY
Copy link
Contributor

why should we delete the config files from spirv-cross?

Because all cmake configuration files should be installed in share/NAME, so I guess vcpkg_fixup_cmake_targets(CONFIG_PATH share/spirv_cross_c/cmake TARGET_PATH share/spirv_cross_c) is missing in spirv-cross's portfile.cmake.

why is the library incorrect on linux

Becuase our triplet x64-linux is a static triplet, all libraries should be built as static.

also I didn't touch glslang

Yes, glslang's issue doesn't related to your changes, I just notify it.

@barcharcraz
Copy link
Contributor Author

@JackBoosY I have addressed the review comments, and also moved to a newer version of spirv-tools which required a newer (non released!) version of spirv-headers, I wrote the version as the date I accessed the repo.

@JackBoosY
Copy link
Contributor

Waiting for merge #16904.

@JackBoosY JackBoosY added depends:different-pr This PR or Issue depends on a PR which has been filed and removed requires:author-response depends:different-pr This PR or Issue depends on a PR which has been filed labels Mar 26, 2021
Charlie Barto and others added 3 commits April 1, 2021 13:19
Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>
@JackBoosY JackBoosY added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Apr 2, 2021
@strega-nil strega-nil changed the title Spirv tools update [spirv-cross,spirv-headers,spirv-tools] 2021-01-15 update Apr 20, 2021
@strega-nil strega-nil merged commit 9c5b302 into microsoft:master Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-update The issue is with a library, which is requesting update new revision info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants