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

Move install directory for SPIRV/ folder. #1919

Merged
merged 3 commits into from
Oct 7, 2019
Merged

Move install directory for SPIRV/ folder. #1919

merged 3 commits into from
Oct 7, 2019

Conversation

dj2
Copy link
Contributor

@dj2 dj2 commented Oct 3, 2019

Currently the SPIRV/ folder will get installed into the include
directory. This folder is part of GLSLang, so it makes more sense under
glslang/SPIRV.

Currently, GLSLang will install a SPIRV/ folder while spirv-headers will
install a spirv/ folder. This is confusing and will cause issues on a
case sensitive filesystem if both are installed at the same time.

dj2 added 2 commits October 3, 2019 19:35
Currently the SPIRV/ folder will get installed into the include
directory. This folder is part of GLSLang, so it makes more sense under
glslang/SPIRV.

Currently, GLSLang will install a SPIRV/ folder while spirv-headers will
install a spirv/ folder. This is confusing and will cause issues on a
case sensitive filesystem if both are installed at the same time.
@johnkslang
Copy link
Member

Upper case is the way the projects are all named and is correct for the SPIR-V acronym. I find it unfortunate that some projects did not follow this lead.

@@ -87,5 +87,6 @@ if(ENABLE_GLSLANG_INSTALL)
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
endif()

install(FILES ${HEADERS} ${SPVREMAP_HEADERS} DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/SPIRV/)
install(FILES ${HEADERS} ${SPVREMAP_HEADERS}
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/glslang/SPIRV/)
Copy link
Member

Choose a reason for hiding this comment

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

The line length is fine. It is easier to see the real diff, with only the actual change in the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@johnkslang
Copy link
Member

What all is at risk of breaking by relocating this?

@dj2
Copy link
Contributor Author

dj2 commented Oct 6, 2019

I left the casing for SPIRV, I just moved it from SPIRV/ to glslang/SPIRV. The risk being, if someone downstream is depending on GLSLang being installed, and using the SPIRV/ folder from GLSLang then their build will break until they change their code to add the glslang/.

I don't know how many projects use the SPIRV/ from glslang instead of spirv-headers.

@johnkslang
Copy link
Member

Thanks.

Glslang is not supposed to be the provider of official SPIR-V headers, I think this was done by someone else who thought (?) otherwise? There is some bootstrapping history here, as SPIR-V was coming online.

So, actually, maybe this is a good first step in getting consumers to use headers from the official path instead.

@johnkslang johnkslang merged commit 4b97a11 into KhronosGroup:master Oct 7, 2019
@johnkslang
Copy link
Member

I don't know how many projects use the SPIRV/ from glslang instead of spirv-headers.

But, this moved headers that no one else provides, breaking downstream consumers.

@dj2
Copy link
Contributor Author

dj2 commented Nov 6, 2019

Yes, there is more stuff in the glslang SPIRV/ then in the spirv-headers spirv/ but there is also duplicated things. It's moved not removed, so downstream would need to change their import locations.

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.

2 participants