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

Generate build information from CHANGES.md #2277

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

ben-clayton
Copy link
Contributor

@ben-clayton ben-clayton commented Jun 17, 2020

This PR significantly reworks the way glslang is versioned.

Instead of committing changes to the GLSLANG_MINOR_VERSION define in glslang/Public/ShaderLang.h, and using make-revision to generate GLSLANG_PATCH_LEVEL in glslang/Include/revision.h, all version information is now derived from the new CHANGES file.

CHANGES acts as the single source of truth for glslang version information, along with a convenient place to put all release notes for each notable change made.

CHANGES is parsed using the new build_info.py python script. This script can read basic template files to produce new source files - which it does to read the new build_info.h.tmpl to generate (at build time) a glslang private header at <build-dir>/include/glslang/build_info.h. I've written generators for each of the CMake, Bazel, gn, and Android.mk build scripts.

The new version code conforms to the Semantic Versioning 2.0 spec.

This new version is also used by the CMake rules to produce versioned shared objects, including a major-versioned SONAME.

Breaking API changes:

  • The public defines GLSLANG_MINOR_VERSION and GLSLANG_PATCH_LEVEL have been entirely removed.
  • glslang/Public/ShaderLang.h and glslang/Include/revision.h have been deleted.
  • Instead <build-dir>/include/glslang/build_info.h is created in the build directory, and <build-dir>/include is a CMake PUBLIC (dependee-inherited) include directory for the glslang targets.
  • <build-dir>/include/glslang/build_info.h contains the following new #defines:
    • GLSLANG_VERSION_MAJOR
    • GLSLANG_VERSION_MINOR
    • GLSLANG_VERSION_PATCH
    • GLSLANG_VERSION_FLAVOR
    • GLSLANG_VERSION_GREATER_THAN(major, minor, patch)
    • GLSLANG_VERSION_GREATER_OR_EQUAL_TO(major, minor, patch)
    • GLSLANG_VERSION_LESS_THAN(major, minor, patch)
    • GLSLANG_VERSION_LESS_OR_EQUAL_TO(major, minor, patch)
  • The CMake install output directory contains a copy of build_info.h at: include/glslang/build_info.h

Python3 is now always required to build glslang (likely always required for transitive dependency builds).

There is a new glslang::GetVersion() function which returns a Version struct with the version major, minor, patch and flavor.


Related issues:

@johnkslang
Copy link
Member

Couple of questions, before looking in detail.

  1. Could we see the new output of glslangValidator -v, perhaps next to the previous output?
  2. What becomes of this?
int GetSpirvGeneratorVersion()
{
    // return 1; // start
    // return 2; // EOpAtomicCounterDecrement gets a post decrement, to map between GLSL -> SPIR-V
    // return 3; // change/correct barrier-instruction operands, to match memory model group decisions
    // return 4; // some deeper access chains: for dynamic vector component, and local Boolean component
    // return 5; // make OpArrayLength result type be an int with signedness of 0
    // return 6; // revert version 5 change, which makes a different (new) kind of incorrect code,
                 // versions 4 and 6 each generate OpArrayLength as it has long been done
    // return 7; // GLSL volatile keyword maps to both SPIR-V decorations Volatile and Coherent
    // return 8; // switch to new dead block eliminator; use OpUnreachable
    return 9; // don't include opaque function parameters in OpEntryPoint global's operand list

@ben-clayton
Copy link
Contributor Author

  1. Could we see the new output of glslangValidator -v, perhaps next to the previous output?

New (master branch, release won't have -dev):

Glslang Version: 9:9.0.0-dev
ESSL Version: OpenGL ES GLSL 3.20 glslang Khronos. 9.0.0-dev
GLSL Version: 4.60 glslang Khronos. 9.0.0-dev
SPIR-V Version 0x00010500, Revision 3
GLSL.std.450 Version 100, Revision 1
Khronos Tool ID 8
SPIR-V Generator Version 9
GL_KHR_vulkan_glsl version 100
ARB_GL_gl_spirv version 100

Old:

Glslang Version: 9.15.3802
ESSL Version: OpenGL ES GLSL 3.20 glslang Khronos. 15.3802
GLSL Version: 4.60 glslang Khronos. 15.3802
SPIR-V Version 0x00010500, Revision 3
GLSL.std.450 Version 100, Revision 1
Khronos Tool ID 8
SPIR-V Generator Version 9
GL_KHR_vulkan_glsl version 100
ARB_GL_gl_spirv version 100

Note the new version is not final - the major should probably be at least 10.

  1. What becomes of this?

It rather depends on what is using it.

We could keep it as the PR currently does - the 9: of the 9:9.0.0-dev is the number returned by GetSpirvGeneratorVersion().

If I understand correctly, you say that some people are using the SPIR-V version for knowing when to invalidate the shader caches? That can still work if we expose the SPIR-V version in the glslang::Version structure returned by glslang::GetVersion().
I do wonder however whether they should always be regenerating their caches if they're pulling a new version of this library.

@jengelh
Copy link
Contributor

jengelh commented Jun 17, 2020

The public defines GLSLANG_MINOR_VERSION and GLSLANG_PATCH_LEVEL have been entirely removed

Too bad, this is a common idiom seen that saves time implementing function existence checks in one's build description:

// mycode.cpp
#if GLSLANG_MINOR_VERSION >= 3559
blah->beginParameterParsing(...)
#else
something else
#endif

@ben-clayton
Copy link
Contributor Author

Thanks for the feedback!

Too bad, this is a common idiom seen that saves time implementing function existence checks in one's build description

Is this idiom in active use for glslang? I took a look around public repos, and I couldn't see use of this pattern. GitHub's code search is awful though, and I clearly cannot inspect closed source projects.

For what it's worth, the semantic versioning proposed here will guarantee API backwards compatibility for each minor and patch release of the same major. I appreciate your argument can be extended to major versions, and use of newly introduced functions in minor releases.

If there's strong desire, we could make the generated header containing the version defines be public for targets depending on glslang. I didn't start with this, as I prefer to keep the public API small and grow only if needed.

@johnkslang
Copy link
Member

A few random notes:

  • I'd rather not see any of the numbers going backward. Is that actually critical?
  • At least historically, there have been several important private uses of glslang.
  • I think the caching issue (which is subtle) is satisfied by keeping all the different version strings, as you have done.

@ben-clayton
Copy link
Contributor Author

I'd rather not see any of the numbers going backward. Is that actually critical?

I can certainly make the major 10. However when you say "any of the numbers", the less significant numbers should be reset to zero when bumping a more significant number - see 7. and 8. of the Semantic Versioning spec. Are you suggesting always having the minor / patch numbers increase, even when a more significant number is bumped?

At least historically, there have been several important private uses of glslang.

Understandable. I can take a look at making the build_info.h public if this is something people want (which it seems @jengelh certainly wants).

I think the caching issue (which is subtle) is satisfied by keeping all the different version strings, as you have done.

Ack.

@jengelh
Copy link
Contributor

jengelh commented Jun 18, 2020

Is this idiom [testing for the values of preproc macros] in active use for glslang?

I do not know about glslang (it is kinda new), but it is something that has made appearances in Linux kernel modules, FUSE modules, software that would have to support libmysqlclient 5.5 & 8 APIs simultaneously, and I think libusb users do something similar.
I am a distro packager, and I "could swear I've seen it quite often", but because it has become daily routine, I would have to do an active search in the package base to deliver further concrete examples. But I'd surely find them in short order.

@ben-clayton
Copy link
Contributor Author

Okay, compelling enough. I'll try and get these versions public.

Thanks @jengelh !

@ben-clayton
Copy link
Contributor Author

@jengelh - I've updated the CMake rules to make the generated header include directory PUBLIC. Dependee targets should be able to #include "glslang/build_info.h", and this also is now emitted into the install directory at include/glslang/build_info.h.

@johnkslang
Copy link
Member

Note the new version is not final - the major should probably be at least 10.

Right, and I think maybe a compromise of first going from (say) 9.15.3802 to 10.15.3802, and then later get to 11.0.0, just to give a bit more warning it is coming.

I'm also going to update the README now to announce this.

(Because I recall real push, long ago, to have an increasing number, but do imagine that by now the new scheme should suffice.)

@ben-clayton
Copy link
Contributor Author

ben-clayton commented Jun 22, 2020

Right, and I think maybe a compromise of first going from (say) 9.15.3802 to 10.15.3802, and then later get to 11.0.0, just to give a bit more warning it is coming.

Done.

I've rebased so that we can have kokoro test the android-ndk and cmake script changes. Note - we still don't have presubmits for the .gn build. I'm going to spend a bit of time today to see if I can get this going. It looks too heavily bound to the chrome build env to make this build standalone.

I've also renamed CHANGES to CHANGES.md so that this renders nicely in github.

@ben-clayton ben-clayton changed the title Generate build information from CHANGES Generate build information from CHANGES.md Jun 22, 2020
@ben-clayton ben-clayton force-pushed the build-info branch 2 times, most recently from 4cb7923 to 9350a22 Compare June 22, 2020 14:39
@ben-clayton ben-clayton force-pushed the build-info branch 6 times, most recently from ade114e to db2e709 Compare July 6, 2020 12:13
@ben-clayton
Copy link
Contributor Author

I believe this is now in reviewable shape.

@ben-clayton ben-clayton marked this pull request as ready for review July 6, 2020 17:31
SPIRV/GlslangToSpv.cpp Outdated Show resolved Hide resolved
@ben-clayton ben-clayton force-pushed the build-info branch 3 times, most recently from f4ba92b to fc6ca20 Compare July 7, 2020 17:21
This PR significantly reworks the way glslang is versioned.

Instead of committing changes to the `GLSLANG_MINOR_VERSION` define in
`glslang/Public/ShaderLang.h`, and using `make-revision` to generate
`GLSLANG_PATCH_LEVEL` in `glslang/Include/revision.h`, all version
information is now derived from the new `CHANGES.md` file.

`CHANGES.md` acts as the single source of truth for glslang version
information, along with a convenient place to put all release notes for
each notable change made.

`CHANGES.md` is parsed using the new `build_info.py` python script.
This script can read basic template files to produce new source files,
which it does to read the new `build_info.h.tmpl` to generate (at build
time) a glslang private header at
`<build-dir>/include/glslang/build_info.h`.
I've written generators for each of the CMake, Bazel, gn, and
`Android.mk` build scripts.

The new version code conforms to the Semantic Versioning 2.0 spec.

This new version is also used by the CMake rules to produce versioned
shared objects, including a major-versioned SONAME.

New APIs:
---------

* `glslang::GetVersion()` returns a `Version` struct with the version
  major, minor, patch and flavor.

Breaking API changes:
---------------------

* The public defines `GLSLANG_MINOR_VERSION` and `GLSLANG_PATCH_LEVEL`
  have been entirely removed.
* `glslang/Public/ShaderLang.h` and `glslang/Include/revision.h` have
  been deleted.
* Instead, `<build-dir>/include/glslang/build_info.h` is created in
  the build directory, and `<build-dir>/include` is a CMake `PUBLIC`
  (dependee-inherited) include directory for the glslang targets.
* `<build-dir>/include/glslang/build_info.h` contains the following
   new #defines:
   `GLSLANG_VERSION_MAJOR`, `GLSLANG_VERSION_MINOR`,
   `GLSLANG_VERSION_PATCH`, `GLSLANG_VERSION_FLAVOR`,
   `GLSLANG_VERSION_GREATER_THAN(major, minor, patch)`,
   `GLSLANG_VERSION_GREATER_OR_EQUAL_TO(major, minor, patch)`,
   `GLSLANG_VERSION_LESS_THAN(major, minor, patch)`,
   `GLSLANG_VERSION_LESS_OR_EQUAL_TO(major, minor, patch)`
*  The CMake install output directory contains a copy of
   `build_info.h` at: `include/glslang/build_info.h`
*  Python3 is now always required to build glslang (likely always
   required for transitive dependency builds).
@johnkslang johnkslang merged commit 0203976 into KhronosGroup:master Jul 10, 2020
@ben-clayton ben-clayton deleted the build-info branch July 10, 2020 11:44
TimothyGu pushed a commit to FFmpeg/FFmpeg that referenced this pull request Jul 11, 2020
The <glslang/Include/revision.h> include was not used, and revision.h has
been removed from glslang master.
See: KhronosGroup/glslang#2277
GYZHANG2019 pushed a commit to VeriSilicon/ffmpeg that referenced this pull request Aug 20, 2020
The <glslang/Include/revision.h> include was not used, and revision.h has
been removed from glslang master.
See: KhronosGroup/glslang#2277
GYZHANG2019 pushed a commit to VeriSilicon/ffmpeg that referenced this pull request Sep 16, 2020
The <glslang/Include/revision.h> include was not used, and revision.h has
been removed from glslang master.
See: KhronosGroup/glslang#2277
GYZHANG2019 pushed a commit to VeriSilicon/ffmpeg that referenced this pull request Sep 16, 2020
The <glslang/Include/revision.h> include was not used, and revision.h has
been removed from glslang master.
See: KhronosGroup/glslang#2277
@ben-clayton
Copy link
Contributor Author

Hi @orbea,

Sorry I didn't spot this use of GLSLANG_PATCH_LEVEL while working on these changes.

Looking through the build files you have linked to and I see the "work-arounds for glslang braindeath" - argh, not pretty.
I can fully appreciate that this change must be frustrating for projects continually searching for reliable ways to determine glslang version, and I'm sorry this change has caused even more trouble.
This change was the start of work to properly semantically version glslang, so that dependencies like this one can confidently assume compatibility of glslang releases with the same major version, and so we can all avoid these sorts of nasty workarounds moving forward. Unfortunately in order to fix things for the long term, we had to break things in the short term.

I'm not overly familiar with the meson build system, but my understanding is that these build rules are trying to determine the glslang_ver from the system installed glslang package headers.
If that's correct, then new glslang packages will contain the glslang/build_info.h header, which at the time of writing contains:

#ifndef GLSLANG_BUILD_INFO
#define GLSLANG_BUILD_INFO

#define GLSLANG_VERSION_MAJOR 11
#define GLSLANG_VERSION_MINOR 0
#define GLSLANG_VERSION_PATCH 0
#define GLSLANG_VERSION_FLAVOR ""

#define GLSLANG_VERSION_GREATER_THAN(major, minor, patch) \
    (((major) > GLSLANG_VERSION_MAJOR) || ((major) == GLSLANG_VERSION_MAJOR && \
    (((minor) > GLSLANG_VERSION_MINOR) || ((minor) == GLSLANG_VERSION_MINOR && \
     ((patch) > GLSLANG_VERSION_PATCH)))))

#define GLSLANG_VERSION_GREATER_OR_EQUAL_TO(major, minor, patch) \
    (((major) > GLSLANG_VERSION_MAJOR) || ((major) == GLSLANG_VERSION_MAJOR && \
    (((minor) > GLSLANG_VERSION_MINOR) || ((minor) == GLSLANG_VERSION_MINOR && \
     ((patch) >= GLSLANG_VERSION_PATCH)))))

#define GLSLANG_VERSION_LESS_THAN(major, minor, patch) \
    (((major) < GLSLANG_VERSION_MAJOR) || ((major) == GLSLANG_VERSION_MAJOR && \
    (((minor) < GLSLANG_VERSION_MINOR) || ((minor) == GLSLANG_VERSION_MINOR && \
     ((patch) < GLSLANG_VERSION_PATCH)))))

#define GLSLANG_VERSION_LESS_OR_EQUAL_TO(major, minor, patch) \
    (((major) < GLSLANG_VERSION_MAJOR) || ((major) == GLSLANG_VERSION_MAJOR && \
    (((minor) < GLSLANG_VERSION_MINOR) || ((minor) == GLSLANG_VERSION_MINOR && \
     ((patch) <= GLSLANG_VERSION_PATCH)))))

#endif // GLSLANG_BUILD_INFO

This header was first introduced at version 10.15.3847 (major 10, minor 15, patch 3847).

To fix this project, I'd probably suggest:

  • Add more logic to src/meson.build so that it can recognise the new glslang/build_info.h header, and extract version information from it.
  • Pass the build-time recognised glslang version into glslang.cc via a compiler command line define.
  • Remove the #include <glslang/Include/revision.h> line from src/glsl/glslang.cc.
  • Use the new src/meson.build glslang version define where ever GLSLANG_PATCH_LEVEL is currently used.

If you'd like to discuss this further, may I suggest opening a new issue? Comments on issues / PRs that are closed/merged can easily go unnoticed.

Many thanks,
Ben

haasn added a commit to haasn/libplacebo that referenced this pull request Oct 21, 2020
This updates our checks to use the new header locations as introduced in
KhronosGroup/glslang#2277. Fortunately, it seems
that the new version scheme is backwards compatible with the old one, so
we don't need any excessively complicated logic updates.

Fixes #83
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