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

build: install glslang-config.cmake to libdir #3009

Merged

Conversation

Tachi107
Copy link
Contributor

As glslang ships architecture dependant files, the Config file should be installed to libdir, not datadir. See #2989 (comment) for more details.

Here's the diff between the install tree before and after this patch:

$ diff <(tree install-datadir) <(tree install)
1c1
< install-datadir
---
> install
74,99c74,98
<         ├── lib
<         │   ├── cmake
<         │   │   ├── glslang-default-resource-limitsTargets.cmake
<         │   │   ├── glslangTargets.cmake
<         │   │   ├── glslangValidatorTargets.cmake
<         │   │   ├── HLSLTargets.cmake
<         │   │   ├── OGLCompilerTargets.cmake
<         │   │   ├── OSDependentTargets.cmake
<         │   │   ├── spirv-remapTargets.cmake
<         │   │   ├── SPIRVTargets.cmake
<         │   │   └── SPVRemapperTargets.cmake
<         │   ├── libGenericCodeGen.a
<         │   ├── libglslang.a
<         │   ├── libglslang-default-resource-limits.a
<         │   ├── libHLSL.a
<         │   ├── libMachineIndependent.a
<         │   ├── libOGLCompiler.a
<         │   ├── libOSDependent.a
<         │   ├── libSPIRV.a
<         │   └── libSPVRemapper.a
<         └── share
<             └── glslang
<                 ├── glslang-config.cmake
<                 ├── glslang-config-version.cmake
<                 ├── glslang-targets.cmake
<                 └── glslang-targets-debug.cmake
---
>         └── lib
>             ├── cmake
>             │   ├── glslang-default-resource-limitsTargets.cmake
>             │   ├── glslangTargets.cmake
>             │   ├── glslangValidatorTargets.cmake
>             │   ├── HLSLTargets.cmake
>             │   ├── OGLCompilerTargets.cmake
>             │   ├── OSDependentTargets.cmake
>             │   ├── spirv-remapTargets.cmake
>             │   ├── SPIRVTargets.cmake
>             │   └── SPVRemapperTargets.cmake
>             ├── glslang
>             │   ├── glslang-config.cmake
>             │   ├── glslang-config-version.cmake
>             │   ├── glslang-targets.cmake
>             │   └── glslang-targets-debug.cmake
>             ├── libGenericCodeGen.a
>             ├── libglslang.a
>             ├── libglslang-default-resource-limits.a
>             ├── libHLSL.a
>             ├── libMachineIndependent.a
>             ├── libOGLCompiler.a
>             ├── libOSDependent.a
>             ├── libSPIRV.a
>             └── libSPVRemapper.a
101c100
< 15 directories, 83 files
---
> 14 directories, 83 files

As glslang ships architecture dependant files, the Config file should be
installed to libdir, not datadir. See
KhronosGroup#2989 (comment)
for more details.

Here's the diff between the install tree before and after this patch:

    $ diff <(tree install-datadir) <(tree install)
    1c1
    < install-datadir
    ---
    > install
    74,99c74,98
    <         ├── lib
    <         │   ├── cmake
    <         │   │   ├── glslang-default-resource-limitsTargets.cmake
    <         │   │   ├── glslangTargets.cmake
    <         │   │   ├── glslangValidatorTargets.cmake
    <         │   │   ├── HLSLTargets.cmake
    <         │   │   ├── OGLCompilerTargets.cmake
    <         │   │   ├── OSDependentTargets.cmake
    <         │   │   ├── spirv-remapTargets.cmake
    <         │   │   ├── SPIRVTargets.cmake
    <         │   │   └── SPVRemapperTargets.cmake
    <         │   ├── libGenericCodeGen.a
    <         │   ├── libglslang.a
    <         │   ├── libglslang-default-resource-limits.a
    <         │   ├── libHLSL.a
    <         │   ├── libMachineIndependent.a
    <         │   ├── libOGLCompiler.a
    <         │   ├── libOSDependent.a
    <         │   ├── libSPIRV.a
    <         │   └── libSPVRemapper.a
    <         └── share
    <             └── glslang
    <                 ├── glslang-config.cmake
    <                 ├── glslang-config-version.cmake
    <                 ├── glslang-targets.cmake
    <                 └── glslang-targets-debug.cmake
    ---
    >         └── lib
    >             ├── cmake
    >             │   ├── glslang-default-resource-limitsTargets.cmake
    >             │   ├── glslangTargets.cmake
    >             │   ├── glslangValidatorTargets.cmake
    >             │   ├── HLSLTargets.cmake
    >             │   ├── OGLCompilerTargets.cmake
    >             │   ├── OSDependentTargets.cmake
    >             │   ├── spirv-remapTargets.cmake
    >             │   ├── SPIRVTargets.cmake
    >             │   └── SPVRemapperTargets.cmake
    >             ├── glslang
    >             │   ├── glslang-config.cmake
    >             │   ├── glslang-config-version.cmake
    >             │   ├── glslang-targets.cmake
    >             │   └── glslang-targets-debug.cmake
    >             ├── libGenericCodeGen.a
    >             ├── libglslang.a
    >             ├── libglslang-default-resource-limits.a
    >             ├── libHLSL.a
    >             ├── libMachineIndependent.a
    >             ├── libOGLCompiler.a
    >             ├── libOSDependent.a
    >             ├── libSPIRV.a
    >             └── libSPVRemapper.a
    101c100
    < 15 directories, 83 files
    ---
    > 14 directories, 83 files
@CLAassistant
Copy link

CLAassistant commented Aug 25, 2022

CLA assistant check
All committers have signed the CLA.

@theblackunknown
Copy link
Contributor

I was re-reading CMake find_package Config Mode Search Procedure page, and was wondering if we could use a more precise path to stress the arch dependent facet that you were looking for.

Something like <prefix>/lib/<arch>/<name>*/ seems to make sense on both Windows and Unix conventions according to the docs, what do you think ?

For convenience, copied here are all supported path patterns listed on CMake documentation page.

<prefix>/                                                       (W)
<prefix>/(cmake|CMake)/                                         (W)
<prefix>/<name>*/                                               (W)
<prefix>/<name>*/(cmake|CMake)/                                 (W)
<prefix>/(lib/<arch>|lib*|share)/cmake/<name>*/                 (U)
<prefix>/(lib/<arch>|lib*|share)/<name>*/                       (U)
<prefix>/(lib/<arch>|lib*|share)/<name>*/(cmake|CMake)/         (U)
<prefix>/<name>*/(lib/<arch>|lib*|share)/cmake/<name>*/         (W/U)
<prefix>/<name>*/(lib/<arch>|lib*|share)/<name>*/               (W/U)
<prefix>/<name>*/(lib/<arch>|lib*|share)/<name>*/(cmake|CMake)/ (W/U)

@Tachi107
Copy link
Contributor Author

This is not needed, as it is already handled by distributions and CMake automatically. On Debian, for example, when <prefix> is /usr, CMAKE_INSTALL_LIBDIR is lib/arch-triplet, e.g. lib/x86_64-linux-gnu.

@Tachi107
Copy link
Contributor Author

Tachi107 commented Aug 26, 2022

Different distros do this differently. Debian as Ubuntu use lib/arch-triplet, while others use lib, lib32, and lib64. This is always handled automatically when it matters, and upstreams should generally not care about it :)

@theblackunknown
Copy link
Contributor

theblackunknown commented Aug 26, 2022

Perfect then ! Looks much more straightforward than what I thought !

Copy link
Contributor

@jeremy-lunarg jeremy-lunarg left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for contributions @Tachi107 and @theblackunknown!

@jeremy-lunarg jeremy-lunarg merged commit 9e78bc8 into KhronosGroup:master Aug 26, 2022
@Tachi107 Tachi107 deleted the cmake-config-file-libdir branch August 26, 2022 15:43
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