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

Use system glslang #5

Open
wants to merge 246 commits into
base: master
Choose a base branch
from
Open

Use system glslang #5

wants to merge 246 commits into from

Conversation

psi29a
Copy link
Owner

@psi29a psi29a commented Dec 7, 2023

Description

Allow support of using system's glslang if available, otherwise fallback to original git based gslang method.

Fixes vsgopenmw-dev/vsgopenmw#9

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@psi29a psi29a changed the base branch from master to glslang_support_alt December 7, 2023 11:46
@psi29a psi29a changed the base branch from glslang_support_alt to master December 7, 2023 11:46
@psi29a psi29a force-pushed the use_system_glslang branch 2 times, most recently from c8d3069 to bff90af Compare December 7, 2023 13:09
…Buffer parameter.

Added reference value to the Instrumentation::enter(..) & leave(..) methods
@psi29a psi29a force-pushed the use_system_glslang branch 2 times, most recently from 225ef7d to a38ae0b Compare December 7, 2023 14:23
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.7)
cmake_minimum_required(VERSION 3.24)

Choose a reason for hiding this comment

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

Have you checked nothing would be affected by any of the policy changes between 3.7 and 3.24. Changing cmake_minimum_required changes all policies introduced in the version range from their OLD behaviour to their NEW behaviour.

Copy link
Owner Author

Choose a reason for hiding this comment

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

There have been no changes (oddly enough), here is the complete output:

/Applications/CLion.app/Contents/bin/cmake/mac/aarch64/bin/cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_MAKE_PROGRAM=/Applications/CLion.app/Contents/bin/ninja/mac/aarch64/ninja -G Ninja -S /Users/psi29a/Workspace/private/VulkanSceneGraph -B /Users/psi29a/Workspace/private/VulkanSceneGraph/cmake-build-relwithdebinfo
-- The CXX compiler identification is AppleClang 15.0.0.15000040
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /Library/Developer/CommandLineTools/usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Reading 'vsg_...' macros from /Users/psi29a/Workspace/private/VulkanSceneGraph/cmake/vsgMacros.cmake - look there for documentation
-- Found Vulkan: /opt/homebrew/lib/libvulkan.dylib (found suitable version "1.3.268", minimum required is "1.1.70.0") found components: glslang SPIRV-Tools glslc glslangValidator 
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE  
-- The following REQUIRED packages have been found:

 * Vulkan (required version >= 1.1.70.0)
 * Threads

-- Configuring done (4.8s)
-- Generating done (0.0s)
-- Build files have been written to: /Users/psi29a/Workspace/private/VulkanSceneGraph/cmake-build-relwithdebinfo

[Finished]

Choose a reason for hiding this comment

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

This changes the behaviour of 72 CMake policies. It's unlikely that none of them would have affected anything. The differences might only end up breaking setups you've not tried, or might only result in runtime changes. There're probably a few changes that mean something that's being handled explicitly can be simplified as CMake will do it implicitly. You really need to look through the policy list https://cmake.org/cmake/help/latest/manual/cmake-policies.7.html#manual:cmake-policies(7) and check each one rather than just try building and assume everything is fine if it looks like it worked.

Copy link
Owner Author

@psi29a psi29a Dec 11, 2023

Choose a reason for hiding this comment

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

The differences might only end up breaking setups you've not tried

This is a sure fire way to prevent any commit from being merged. I hope you understand that.

Which is why we try to make sure the CI is as robust as possible, within reason. It is impossible to test every place that VSG or any library would be used on.

Thanks for the link, I'll review.

Building is not enough, I've validated on 3 platforms except Windows. So I'll need someone's help making sure that it actually runs there.

Choose a reason for hiding this comment

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

This is a sure fire way to prevent any commit from being merged. I hope you understand that.

Yes. Changing what's passed to cmake_minimum_required is a big deal, and needs an abundance of caution. It's not as minor as doing something like changing the C++ language standard, and that's already a pretty big deal.

@psi29a psi29a force-pushed the use_system_glslang branch 2 times, most recently from c9d04e8 to 77944e8 Compare December 11, 2023 10:42
robertosfield and others added 29 commits February 21, 2024 10:49
Add is_compatible to Visitor and ConstVisitor
Animation and object cloning/copy constrcutors
… start time to the Animation::start(..) method
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.

Reproducible and usable builds of vsgopenmw
8 participants