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

Add pkg-config files #2083

Closed
wants to merge 2 commits into from
Closed

Conversation

danyspin97
Copy link

CMakeLists.txt Outdated

if(ENABLE_OPT)
# This is always true
if(${SPIRV_TOOLS_FOUND} EQUAL 1)
Copy link
Member

Choose a reason for hiding this comment

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

This, or something like it, seems to have broken CMake, at least for AppVeyor.

Copy link
Author

Choose a reason for hiding this comment

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

AppVeyor build uses Windows and does not ship pkg-config tool. I'm looking for a way to fix this. If Windows cannot be fixed, I'm going to add a check and use the provided SPIRV-Tools on it.

Copy link
Author

Choose a reason for hiding this comment

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

I'll open a separate PR to build using system SPIRV-Tools.

@danyspin97 danyspin97 changed the title Link to system SPIRV-Tools package and add pkg-config files Add pkg-config files Feb 14, 2020
@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

@@ -40,6 +40,9 @@ target_include_directories(SPIRV PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/..>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)

set(SPIRV_NAME spirv)
set(SPIRV_VERSION 1.5)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? We seem to be increasing the number of random places that will need to be changed when SPIR-V is updated.

Choose a reason for hiding this comment

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

the better way for this is create a cmake function which read from where Spirv set the version and set automagically by cmake

a example:

# Get FOO_VERSION property from 'foo/bar/file'
file(READ "foo/bar/file" versioning)
string(REGEX MATCH "FOO_MAJOR_VER     ([0-9]*)" _ ${versioning})
set(version_major ${CMAKE_MATCH_1})
string(REGEX MATCH "FOO_MINOR_VER     ([0-9]*)" _ ${versioning})
set(version_minor ${CMAKE_MATCH_1})
string(REGEX MATCH "FOO_BUGFIX_VER    ([0-9]*)" _ ${versioning})
set(version_bugfix ${CMAKE_MATCH_1})
set(FOO_VERSION ${version_major}.${version_minor}.${version_bugfix})

with this, only need set the version in one place

Copy link
Author

Choose a reason for hiding this comment

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

It is needed to set a version in the spirv.pc file. What could be done is to pass SPIRV version from cmake to spirv.hpp.

@@ -80,6 +80,7 @@ set(HEADERS
MachineIndependent/preprocessor/PpContext.h
MachineIndependent/preprocessor/PpTokens.h)

set(VERSION 8.13.3559)
Copy link
Member

Choose a reason for hiding this comment

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

This number is already out of date...

Choose a reason for hiding this comment

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

same as avobe. with a own cmake function can do this automagic

greetings

Copy link
Author

Choose a reason for hiding this comment

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

Same thing here.

@johnkslang
Copy link
Member

There is enough feedback here that a better alternate way is needed, so closing this one. Thanks.

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