Skip to content

[shaderc] Use build type to build targets#16137

Merged
ras0219-msft merged 5 commits intomicrosoft:masterfrom
JackBoosY:dev/jack/15452
Feb 11, 2021
Merged

[shaderc] Use build type to build targets#16137
ras0219-msft merged 5 commits intomicrosoft:masterfrom
JackBoosY:dev/jack/15452

Conversation

@JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Feb 9, 2021

Currently, shaderc has generated dynamic libraries, static libraries and static libraries containing all dependent binaries at the same time.
This PR will:

  • Determine whether to generate a dynamic library or a static library according to BUILD_SHARED_LIBS.
  • Disable generating static libraries containing all dependent binaries.

Partly solved #15452.
Fixes #16122.

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support info:internal labels Feb 9, 2021
@JackBoosY JackBoosY requested a review from NancyLi1013 February 9, 2021 07:51
JackBoosY and others added 3 commits February 8, 2021 23:59
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@JackBoosY JackBoosY requested a review from NancyLi1013 February 9, 2021 08:07
@JackBoosY
Copy link
Contributor Author

Waiting for merge #16138.

@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Feb 9, 2021
@ras0219-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed depends:different-pr This PR or Issue depends on a PR which has been filed labels Feb 10, 2021
@ras0219-msft ras0219-msft merged commit f6a8772 into microsoft:master Feb 11, 2021
@ras0219-msft
Copy link
Contributor

This LGTM

@JackBoosY JackBoosY deleted the dev/jack/15452 branch February 18, 2021 05:48
@jmoguillansky-gpsw
Copy link

Hi @JackBoosY I updated vcpkg to the latest version. It seems that only static shaderc was built.
How do I build the shared lib?
Am I supposed to set BUILD_SHARED_LIBS environment variable or something?

@jmoguillansky-gpsw
Copy link

Seems that this PR disables shaderc shared library build?

#Note: glslang and spir tools doesn't export symbol and need to be build as static lib for cmake to work
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)

This part doesn't make sense. If glslang and spir-tools require static shaderc then they can link against the static shaderc, that doesn't mean that shaderc doesn't support shared library build?

@JackBoosY
Copy link
Contributor Author

@jmoguillansky-gpsw For building shared, use command ./vcpkg install shaderc.
You should use the right triplet to build it.

@jmoguillansky-gpsw
Copy link

@jmoguillansky-gpsw For building shared, use command ./vcpkg install shaderc.
You should use the right triplet to build it.

Hi @JackBoosY ,
Yes I tried that: vcpkg install shaderc:x64-windows, but it still only built the static lib?

@JackBoosY
Copy link
Contributor Author

@jmoguillansky-gpsw Ops, sorry, I forgot we force it to build as a static library.
I will investigate later.

@ras0219-msft
Copy link
Contributor

The issue is not that glslang and spir-tools require static shaderc -- rather, the problem is that glslang and spir-tools can only be built statically, which means building a shared library for shaderc is extremely suspect (it will contain a duplicate copy of the glslang/spir-tools code along with the main application binary as well as any other consumers). Shaderc consumes glslang, not the other way around.

Therefore, until glslang and spir-tools can be built dynamically, we do not want to build shaderc dynamically.

@jmoguillansky-gpsw
Copy link

Shaderc upstream supports shared lib build, and its shipped as part of vulkan sdk. There's no issue with shaderc , the tool just decides to use the static lib, thats all

@jmoguillansky-gpsw
Copy link

I think there's a broader discussion with regards to vcpkg architecture. Each module doesn't know how it's used, only what it can build. Some executables may choose to always use static lib, some shared lib, etc. From my perspective the vcpkg architecture should accomodate these cases.
One option:
The module should build all supported configurations (static, shared, etc).
Vcpkg integrate install can have a param of default lib type (e.g. shared or static) the same way that a linker (ld on linux) has a param to specify default lib type.

@jmoguillansky-gpsw
Copy link

jmoguillansky-gpsw commented Mar 19, 2021

Regarding vcpkg integrate install:
Maybe one option to consider is:
let the user navigate to a project and type something like "vcpkg integrate <module name>" and then vcpkg will integrate the module into the project.
This gives the user convenience, but perhaps safer than making all libs globally available for all msvc projects,etc.

@JackBoosY
Copy link
Contributor Author

glslang and spir-tools only depend on the static library, not dynamic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-bug The issue is with a library, which is something the port should already support 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.

vcpkg integrate install logic is incorrect

4 participants