Make the layers install into the data root directory #2603
Conversation
|
|
|
Please see CONTRIBUTING.md and the continuous-integration/travis-ci/pr results for commit format assistance. |
|
The description of the GNUInstallDirs suggests that both the SYSCONFDIR and the DATAROOTDIR are for "data" files, so I'm not sure I see the distinction. Do you have some sort of reference, like the FHS, that suggests DATAROOTDIR is preferable to SYSCONFDIR? I also believe that you can override the value of SYSCONFDIR on the CMake command line if the default location is not suitable for you. |
|
Hi Karl, Thanks for your comment and interest. Based on the GnuInstallDirs doc we can see that: I believe the JSON files that this area is installing fits better under the latter, as it appears to be architecture independent. In addition, we see that these values are expanded to /etc and /usr/share respectively. If we look at FHS, section 3.7 states that /etc is for the below purpose: Section 4.11 decesribes /usr/share: As far as I can tell, the installed files are not configuration files. Due to these reasons, I feel this data fits better in /usr/share (DATAROOTDIR) than /etc (SYSCONFIGDIR) I am a maintainer for Gentoo, and we are currently shipping a patch to fix this behavior as I feel it is an upstream issue. I could just change the value of SYSCONFDIR in CMake, but this solution is cleaner. Please let me know if you have any comments. If there is a chance of this being accepted, I will work on commit format. Thanks, |
|
We've always thought of these files as configuration files, but I concede that users rarely edit these files. And I just think that early momentum in this project placed the files in /etc. Another data point is that the proprietary nvidia driver installer places a similar file in /etc/vulkan/icd.d that does pretty much the same thing. We can't change that here, but it shows some precedent and there is some value in the sense of consistency if these layer "config" files are in the same place. The good news is that the Vulkan loader has, for a long time, searched the /usr/share (DATAROOTDIR) for these files when built with default settings. So moving them should not be a problem. But the loader's search paths can be configured at build time. It is possible, for example, for a package maintainer to build the loader with an alternate value for DATAROOTDIR such that /usr/share is not searched, but /etc still is. Then this change could result in a loader not finding the layers. But I think that the surface area of this risk is pretty small. And it may not be a problem at all if the loader and layers are built with the same settings. I think we're leaning towards accepting this change, so go ahead and fix up the commit message. If you know about any other package maintainers that may be interested in this change, see if you can get them to review this and give a 👍 or whatever. I do worry about breakage in that area. |
|
Thank you Karl. I have fixed the commit message. Please let me know if there is something I missed. I will email other maintainers now, but I can't say that any of them will actually look at this PR let alone review my change. Thank you, |
|
Debian/Ubuntu already move the layer files under /usr/share/vulkan, so making it the default would be a welcomed change. |
| @@ -142,7 +142,7 @@ if(UNIX) | |||
| VERBATIM | |||
| DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/linux/${config_file}.json | |||
| ) | |||
| install(FILES ${CMAKE_CURRENT_BINARY_DIR}/staging-json/${config_file}.json DESTINATION ${CMAKE_INSTALL_SYSCONFDIR}/vulkan/explicit_layer.d) | |||
| install(FILES ${CMAKE_CURRENT_BINARY_DIR}/staging-json/${config_file}.json DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/vulkan/explicit_layer.d) | |||
There was a problem hiding this comment.
I hate to do this, but could you also please fix the comment a few lines up? Perhaps just change it to
# Need to remove the "./" from the library path before installing to system directories.
|
I'm sorry I didn't catch the comment problem sooner. After that, we should be good, |
The layers files fit are not really configuration files and are rarely edited by users. So, let's move the install location into the data root directory. Signed-off-by: Nick Sarnie <sarnex@gentoo.org>
|
@karl-lunarg Done, good catch |
|
Thanks Karl! |
Hi,
I think the layers files should be installed in the data root directory. They are not configuration files, so they do not belong in /etc. Please review,
Sarnex