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

Added Spirv-tools recipe #1349

Merged
merged 11 commits into from
Jun 15, 2020
Merged

Added Spirv-tools recipe #1349

merged 11 commits into from
Jun 15, 2020

Conversation

GavinNL
Copy link
Contributor

@GavinNL GavinNL commented Apr 14, 2020

Specify library name and version: spirv-tools/2019.2

  • This recipe depends on spirv-headers

  • Tested this on linux mint 19

  • This recipe contains two test_package sources, test_package.c and test_package.cpp, the one that is built is depends on whether the c_only option is selected. The project's CMakeLists file produces multiple libraries, plus an additional spirv-tools-shared.so which is the same as the combination of all the other libraries produced, except it only exports the C-interface, therefore only this file is linked when c_only is set. It is always built as a shared-library even when shared is set to true. The other C++ libs are built appropriately.

  • I've read the guidelines for contributing.

  • I've followed the PEP8 style guides for Python code in the recipes.

  • I've used the latest Conan client version.

  • I've tried at least one configuration locally with the
    conan-center hook activated.

cmake.configure(build_folder=self._build_subfolder)
return cmake

def build(self):
Copy link
Contributor

@SpaceIm SpaceIm Apr 15, 2020

Choose a reason for hiding this comment

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

In CMakeLists, SPIRV-tools has set(CMAKE_POSITION_INDEPENDENT_CODE ON): https://github.com/KhronosGroup/SPIRV-Tools/blob/8dd174809fb86498e5870276be7c84a8bf4c8254/CMakeLists.txt#L33
It should be removed, otherwise this recipe will lie to consumers using shared=False and fPIC=False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So would I have to make a patch that would delete that line from the CMakeLists.txt file?

cmake = CMake(self)

# Required by the project's CMakeLists.txt
cmake.definitions["SPIRV-Headers_SOURCE_DIR"] = self.deps_cpp_info["spirv-headers"].rootpath
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add an option in order to set SPIRV_SKIP_EXECUTABLES (OFF by default).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add this in, but is there any reason to NOT have the executable when building a dependency? You're not forced to use them (eg: by linking), you simply don't use them. All it does is save a few seconds of compiling time. Adding this extra option causes an extra set of permutations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please, leave the executables there, someone might want to use this packages as a build_requires, or they may install it using a virtualenv generator just to use those executables.

else:
self.cpp_info.libs.append("SPIRV-Tools-reduce")
self.cpp_info.libs.append("SPIRV-Tools-opt")
self.cpp_info.libs.append("SPIRV-Tools")
Copy link
Contributor

Choose a reason for hiding this comment

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

you should add bin folder in PATH env if SPIRV_SKIP_EXECUTABLES is OFF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this?

            bin_path = os.path.join(self.package_folder, "bin")
            self.env_info.path.append(bin_path)

default_options = {
"shared": True,
"fPIC": True,
"c_only": False
Copy link
Contributor

@SpaceIm SpaceIm Apr 15, 2020

Choose a reason for hiding this comment

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

I don't think this option should be provided. SPIRV-Tools-shared should be completly removed.

With the current recipe:
if spirv-tools:shared=True: SPIRV-Tools-shared and SPIRV-Tools seems to be the same shared lib. But only SPIRV-Tools is linked into SPIRV-Tools-reduce and SPIRV-Tools-opt (and executables).
Anyway, looking to CMakeLists, SPIRV-Tools-shared is not the combination of all the other libraries produced.

EDIT: mmhh, if shared SPIRV-Tools is probably wrong because it doesn't define SPIRV_TOOLS_IMPLEMENTATION and SPIRV_TOOLS_SHAREDLIB while SPIRV-Tools-shared does. It is also less optimized, since SPIRV-Tools-shared has CXX_VISIBILITY_PRESET hidden

I think that they are aware that how shared is handled is a little bit broken: KhronosGroup/SPIRV-Tools#3214

Copy link
Contributor

@SpaceIm SpaceIm Apr 15, 2020

Choose a reason for hiding this comment

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

I think that shared is simply not supported in SPIRV-tools at least on Windows, because others libs than SPIRV-Tools-shared (SPIRV-Tools, SPIRV-Tools-reduce, SPIRV-Tools-link, SPIRV-Tools-opt) don't export their symbols.
Therefore, I would recommend to patch CMakeLists in order to completly remove any reference to SPIRV-Tools-shared, and raise a ConanInvalidConfiguration if shared (maybe only for Windows).
You can keep C test_package, but you must add property LINKER_LANGUAGE CXX

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spirv-tools-shared.so confused me as well. You would link only to this library if you were building a C-only application. spirv-tools-shared.so contains all the code from the other libraries, but only exposes the C interface.

Running patchelf --print-needed spirv-tools-shared.so shows that it does not link to any of the other built shared libraries.

What may need to be done is delete the other shared objects, keeping only spirv-tools-shared.so when c_only=true.

The C-Interface cannot be built as a static library. So I'm not entirely sure what to do about shared=false when c_only=true. Maybe throw the ConanInvalidConfiguration error saying `"the C-only interface MUST be built as a shared library" ?

I don't use windows, does test_package.cpp compile and link on windows when shared=True and c_only=False?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@GavinNL GavinNL Apr 16, 2020

Choose a reason for hiding this comment

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

Here is the summary of the two options I'm thinking of.

  1. Do not allow spirv-tools-shared.so to be built. C-only users are on their own. This allows the rest of the libraries to be built in shared/static mode

  2. If c_only=true, Keep only spirv-tools-shared.so and delete the other libraries. C-only users will have to live without the use cases of the other libraries. Throw a ConanInvalidConfiguration error if this c_only=True and shared=False. This allows the C++ interface to be built in shared/static mode, and the C-only interface to only be built in shared mode.

I'm really surprised at the Khronos group's CMake skills. For an organisation of their reputation, their CMake skills are very lacking, lol. I've had such a hard time compiling any of their libraries

Copy link
Contributor

@SpaceIm SpaceIm Apr 20, 2020

Choose a reason for hiding this comment

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

Looking closely to this library and discussions in issues, it appears that there shouldn't be a c_api_only (or c_only) option.
They try to fix the shared libs, therefore this option won't make any sense when fixed (but it will be very hard because most of them and also their executables also depends on private headers of their dependencies !).

For this version, I would:

  • patch CMakeLists.txt to remove any reference to SPIRV-Tools-shared
  • remove c_only option
  • either raise ConanInvalidConfiguration for shared and Visual Studio, or use CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS (which is bad practice but there is no easy solution without this, I've tried to fix the libs but it's really hard).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I can do that. Is there a reason to explicitly patch the CMakeLists.txt file to remove SPIRV-Tools-shared? Can't we just delete the file after building/before packaging?

Copy link
Contributor

@SpaceIm SpaceIm Apr 20, 2020

Choose a reason for hiding this comment

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

you're probably right, it should work. By the way, you should only add one version (2020.1 or 2019.2 as you wish, I was not able to compile 2020.2. At some point, I will require 2020.1 for recent versions of Vulkan-ValidationLayers since 2019.2 is not compatible with them).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SpaceIm , I made the changes you suggested.

I probably shouldn't have included 2020.1 since it's not a stable release. The reason it was there was actually because the glslang package that I'm working on needs a newer version of the headers to enable HSLSL. I'll remove 2020.1 until that branch has released a stable version.

I'll be pushing a glslang conan package after this spirv-tools is merged, but it wont have HLSL support until a newer version of spirv-tools is released in conan

if self.options.c_only:
self.cpp_info.libs.append("SPIRV-Tools-shared")
else:
self.cpp_info.libs.append("SPIRV-Tools-reduce")
Copy link
Contributor

Choose a reason for hiding this comment

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

what about SPIRV-Tools-link (which depends on SPIRV-Tools-opt)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops.

They'd have to be linked in a specific order, correct?

            self.cpp_info.libs.append("SPIRV-Tools-reduce")
            self.cpp_info.libs.append("SPIRV-Tools-link")
            self.cpp_info.libs.append("SPIRV-Tools-opt")
            self.cpp_info.libs.append("SPIRV-Tools")

- cmake is cached
- patched lists file to remove CMAKE_POSITION_INDEPENDENT_CODE
- removed c_only_api and SPIRV-Tools-shared.so/lib/dll
added librt for Linux
- added short_paths
- there's really no reason to add this option. If you don't want the executables, don't use them. It just adds an extra permutation of the options to test
@conan-center-bot
Copy link
Collaborator

All green in build 2 (a8cc3487fa1e640ecc742e78ffb7302f543835b6)! 😊

# it is built by the original CMakeLists.txt file. It is the same
# as the other libraries except only the C interface is exposed
# This library is being removed because it is only causing confusion.
# As a result, you are restricted to to using the C++ api only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consumers can still use C API, its symbols are exported in SPIRV-Tools (actually all symbols are exported).

for lib_file in glob.glob(os.path.join(self.package_folder, "lib", "*SPIRV-Tools-shared*")):
os.remove(lib_file)

def package_info(self):
Copy link
Contributor

@SpaceIm SpaceIm Apr 21, 2020

Choose a reason for hiding this comment

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

Suggested change
def package_info(self):
def package_info(self):
# TODO: set targets names when components available in conan
self.cpp_info.names["cmake_find_package"] = "SPIRV-Tools"
self.cpp_info.names["cmake_find_package_multi"] = "SPIRV-Tools"

Vulkan-ValidationLayers expects this target (which is a component actually). At least add TODO comment ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added and pushed

Copy link
Member

Choose a reason for hiding this comment

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

Components is available

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what needs to be done for the components, @SpaceIm , could you make a suggestion?

Copy link
Contributor

@SpaceIm SpaceIm May 8, 2020

Choose a reason for hiding this comment

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

It's pretty new. From what I understand from https://docs.conan.io/en/latest/reference/conanfile/attributes.html#cpp-info, I would try this:

    def package_info(self):
        self.cpp_info.names["cmake_find_package"] = "SPIRV-Tools"       # not sure, SPIRV-Tools doesn't
        self.cpp_info.names["cmake_find_package_multi"] = "SPIRV-Tools" # export a namespace
        self.cpp_info.names["pkg_config"] = "SPIRV-Tools"
        # SPIRV-Tools component
        self.cpp_info.components["spirv-tools-core"].names["cmake_find_package"] = "SPIRV-Tools"
        self.cpp_info.components["spirv-tools-core"].names["cmake_find_package_multi"] = "SPIRV-Tools"
        self.cpp_info.components["spirv-tools-core"].libs = ["SPIRV-Tools"]
        self.cpp_info.components["spirv-tools-core"].requires = ["spirv-headers::spirv-headers"]
        if self.settings.os == "Linux":
            self.cpp_info.components["spirv-tools-core"].system_libs.append("rt")
        if not self.options.shared and self._stdcpp_library:
            self.cpp_info.components["spirv-tools-core"].system_libs.append(self._stdcpp_library)
        # SPIRV-Tools-opt component
        self.cpp_info.components["spirv-tools-opt"].names["cmake_find_package"] = "SPIRV-Tools-opt"
        self.cpp_info.components["spirv-tools-opt"].names["cmake_find_package_multi"] = "SPIRV-Tools-opt"
        self.cpp_info.components["spirv-tools-opt"].libs = ["SPIRV-Tools-opt"]
        self.cpp_info.components["spirv-tools-opt"].requires = ["spirv-tools-core", "spirv-headers::spirv-headers"]
        if self.settings.os == "Linux":
            self.cpp_info.components["spirv-tools-opt"].system_libs.append("m")
        # SPIRV-Tools-link component
        self.cpp_info.components["spirv-tools-link"].names["cmake_find_package"] = "SPIRV-Tools-link"
        self.cpp_info.components["spirv-tools-link"].names["cmake_find_package_multi"] = "SPIRV-Tools-link"
        self.cpp_info.components["spirv-tools-link"].libs = ["SPIRV-Tools-link"]
        self.cpp_info.components["spirv-tools-link"].requires = ["spirv-tools-core", "spirv-tools-opt"]
        # SPIRV-Tools-reduce component
        self.cpp_info.components["spirv-tools-reduce"].names["cmake_find_package"] = "SPIRV-Tools-reduce"
        self.cpp_info.components["spirv-tools-reduce"].names["cmake_find_package_multi"] = "SPIRV-Tools-reduce"
        self.cpp_info.components["spirv-tools-reduce"].libs = ["SPIRV-Tools-reduce"]
        self.cpp_info.components["spirv-tools-reduce"].requires = ["spirv-tools-core", "spirv-tools-opt"]

        bin_path = os.path.join(self.package_folder, "bin")
        self.output.info("Appending PATH environment variable: {}".format(bin_path))
        self.env_info.path.append(bin_path)

    @property
    def _stdcpp_library(self):
        libcxx = self.settings.get_safe("compiler.libcxx")
        if libcxx in ("libstdc++", "libstdc++11"):
            return "stdc++"
        elif libcxx in ("libc++",):
            return "c++"
        else:
            return False

(_stdcpp_library is here to propagate c++ lib into SPIRV-Tools for the C API)

Copy link
Contributor

@SpaceIm SpaceIm May 8, 2020

Choose a reason for hiding this comment

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

By the way, if components are used in this recipe, maybe we should check conan version. Because I tried first with 1.24.0, I was able to create the package without any error or warning, but libs ordering was incorrect in cmake generator (it's fine in 1.25.0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering if we should use components because the feature is still listed as experimental..

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.
@uilianries What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

No problem including it. but global info is not allowed, you will need to remove:

self.cpp_info.names["cmake_find_package"] = "SPIRV-Tools"       # not sure, SPIRV-Tools doesn't
        self.cpp_info.names["cmake_find_package_multi"] = "SPIRV-Tools" # export a namespace
        self.cpp_info.names["pkg_config"] = "SPIRV-Tools"

Copy link
Contributor

Choose a reason for hiding this comment

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

global names are allowed I think (at least it properly work with conan 1.25.0), how would you define a namespace without that?

@conan-center-bot
Copy link
Collaborator

All green in build 3 (b13f711a5a6c4100e4b1c757824eaa3ad496ce99)! 😊

@GavinNL
Copy link
Contributor Author

GavinNL commented May 3, 2020

@SpaceIm , were you able to take a look at the latest changes to the recipe?

cmake_minimum_required(VERSION 2.8.12)
project(test_package)

if( ${BUILD_C_TEST_PACKAGE} )
Copy link
Contributor

Choose a reason for hiding this comment

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

if( ${BUILD_C_TEST_PACKAGE} )
    set(CMAKE_C_STANDARD 11)
else()
    set(CMAKE_CXX_STANDARD 11)
endif()

set(CMAKE_VERBOSE_MAKEFILE TRUE)

All of this can be removed now.

@Hopobcn
Copy link
Contributor

Hopobcn commented May 5, 2020

It's me or the CI is stuck? The last commit is 23h ago but the message is still Waiting for status to be reported

@GavinNL
Copy link
Contributor Author

GavinNL commented May 5, 2020

It's me or the CI is stuck? The last commit is 23h ago but the message is still Waiting for status to be reported

I'm not sure. I was wondering about this as well.

@SSE4 SSE4 added the infrastructure Waiting on tools or services belonging to the infra label May 6, 2020
@SSE4
Copy link
Contributor

SSE4 commented May 6, 2020

@danimtb we have a problem

@uilianries
Copy link
Member

@danimtb we have a problem

@danimtb danimtb removed the infrastructure Waiting on tools or services belonging to the infra label May 6, 2020
@conan-center-bot
Copy link
Collaborator

Some configurations of 'spirv-tools/2019.2' failed in build 4 (82d965758efbd5bdeaa6541334988661c1ee2e75):

  • Linux x86_64, Release, gcc 5, libstdc++ . Options: spirv-tools:shared-True
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [NO REQUIRES.ADD() (KB-H044)] The method 'self.requires.add()' is not allowed. Use 'self.requires()' instead. (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H044)
    • Log download
    • Log error Download
    • Profile
  • Access to all the logs

@conan-center-bot
Copy link
Collaborator

All green in build 5 (531a9b2d3b2ff8e57d827df96460cdb19113a3fc)! 😊

Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
@conan-center-bot
Copy link
Collaborator

All green in build 6 (035f30dd1905a018ac5392c74aacdd17175f131b)! 😊

@SpaceIm
Copy link
Contributor

SpaceIm commented May 12, 2020

@SSE4 @uilianries @danimtb @jgsogo
Could you review this recipe? It is required for glslang, shaderc and vulkan-validation-layers

@jgsogo
Copy link
Contributor

jgsogo commented May 13, 2020

Versioning schema for this library is available in the authors' repo https://github.com/KhronosGroup/SPIRV-Tools#versioning-spirv-tools

image

and reading some issues there is no commitment of API compatibility. Should we change the version to match the one from the developers, are projects using version_ranges and trusting on API/ABI compatibility for this library?

@GavinNL
Copy link
Contributor Author

GavinNL commented May 13, 2020

Versioning schema for this library is available in the authors' repo https://github.com/KhronosGroup/SPIRV-Tools#versioning-spirv-tools

image

and reading some issues there is no commitment of API compatibility. Should we change the version to match the one from the developers, are projects using version_ranges and trusting on API/ABI compatibility for this library?

The 2016 versions aren't the newest. I believe that's just an example. I'm using the versions which have been released ( https://github.com/KhronosGroup/SPIRV-Tools/releases ), the latest non-dev version is 2019.2

@jgsogo
Copy link
Contributor

jgsogo commented May 20, 2020

My only concern about the versioning schema is related to API compatibility.

If we use just 2019.2 then consumers would be able to declare a version range like [>=2019] and Conan will select the requirement accordingly (while I'm writing it I have doubts, 2019.2 is not valid SemVer, but probably it is parsed as 2019.2.0 when using loose=True). If this library doesn't have any API-compatibility it will break consumers because API for 2019.2 will be different from 2019.3... if we use v2019.2, then it is not parsed as SemVer and version_ranges can't be used.

I haven't used this library, I don't know if they break the API on every version or it is just they do their best but without any further compromise. Would you let Conan choose the version for you or using this library you do really care about the version you want to use?

@GavinNL
Copy link
Contributor Author

GavinNL commented May 20, 2020

My only concern about the versioning schema is related to API compatibility.

If we use just 2019.2 then consumers would be able to declare a version range like [>=2019] and Conan will select the requirement accordingly (while I'm writing it I have doubts, 2019.2 is not valid SemVer, but probably it is parsed as 2019.2.0 when using loose=True). If this library doesn't have any API-compatibility it will break consumers because API for 2019.2 will be different from 2019.3... if we use v2019.2, then it is not parsed as SemVer and version_ranges can't be used.

I haven't used this library, I don't know if they break the API on every version or it is just they do their best but without any further compromise. Would you let Conan choose the version for you or using this library you do really care about the version you want to use?

Ahh I see what you mean. Sorry I didn't realise 2019.2 was different from v2019.2. I agree with you then. We should probably do v2019.2 since if I was using this library, I would need an exact version.

Do I just need to change the library version or is there something else I should add to the recipe?

@SpaceIm
Copy link
Contributor

SpaceIm commented May 20, 2020

Most of the time spirv-tools seems to be backward compatible.

@conan-center-bot
Copy link
Collaborator

All green in build 7 (7c41676cac859697eee6ed30f6d18ffd83798761)! 😊

@SSE4 SSE4 requested a review from uilianries May 26, 2020 17:09
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.

8 participants