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

Determine libSPIRV target 'public' interface #2308

Closed
ben-clayton opened this issue Jul 2, 2020 · 3 comments · Fixed by #3705
Closed

Determine libSPIRV target 'public' interface #2308

ben-clayton opened this issue Jul 2, 2020 · 3 comments · Fixed by #3705

Comments

@ben-clayton
Copy link
Contributor

As part of the work to apply Semantic Versioning to the glslang project, we need to determine what we consider as part of the public, stable API and what is internal.

The Semantic Versioning introduction states:

For this system to work, you first need to declare a public API. This may consist of documentation or be enforced by the code itself. Regardless, it is important that this API be clear and precise. Once you identify your public API, you communicate changes to it with specific increments to your version number.

For the glslang target we have been able to classify glslang/Public/ShaderLang.h as the public API. This is now (#2303) enforced through defaulting to -fvisibility=hidden and only making public the methods and types annotated with GLSLANG_EXPORT.

libSPIRV is another library that is already found in linux distribution packages, but AFAICT, there is no clear separation between the public and private interface. As such, any changes to the SPIRV source files will likely require a major semantic version bump, which severely reduces the benefits for semantic versioning.

I see a few options:

  1. We spend the time to try classifying the public interface, and build a SPIRV/Public/SPIRV.h file, similar to glslang.

  2. Remove libSPIRV from the 'public' interface.
    It's hard to know how many people out there are using the libSPIRV target - but one potential solution is to make this an entirely 'private' library, and ensure this is statically linked into the glslangValidator executable, removing it from the install target. This would remove it from existing linux distributions, breaking anyone who is currently depending on it.

  3. We simply ignore semantic versioning for libSPIRV. Breaking API changes to libSPIRV are not considered for major version bumping. Semantic versioning will only apply for the glslang target.

Feedback and opinions welcome.

@johnkslang
Copy link
Member

Right, the AST -> SPIR-V generator is not based on a publicly supported interface, nor did this project ever say it was or say it should be an SO or advise that Linux distributors distribute it that way. This project intended a few small static libraries to have their necessary smaller subsets pulled into parent projects, either into a parent's static library or a parent's SO. As you note, the ShaderLang.h was in a public folder, while the other headers were not. (This SO thing is like people saying a motorbike should be a car and then complaining about how stupid the motorbike was to only have 2 wheels, no windows, etc., like whoever made the motorbike really didn't know how to make cars.) Note that most of glslangValidator's size is from adding side things like SPIRV-Tools and Google's test suite, not the core parsers/translators most other projects need.

So, we don't need to do 1.

@arcady-lunarg
Copy link
Contributor

At the moment the proposed public interface to SPIRV is basically all the symbols declared in GlslangToSpv.h, disassemble.h, and SpvTools.h. From Logger.h, spv::SpvBuildLogger::getAllMessages() is exported. Also everything in CInterface is exported. Is there anything else that is needed?

@arcady-lunarg
Copy link
Contributor

The symbols tagged with GLSLANG_EXPORT in #3705 are the proposed public interface to the SPIRV library (which got merged into glslang proper recently). If anyone has any comments about that, I'm going to leave the PR open for a week or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants