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

Update glslang submodule #19952

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update glslang submodule #19952

wants to merge 1 commit into from

Conversation

oltolm
Copy link
Contributor

@oltolm oltolm commented Feb 5, 2025

This is also necessary to fix the GCC build.

@hrydgard
Copy link
Owner

hrydgard commented Feb 5, 2025

Can you explain why we need to add two submodules? Also, this didn't quite build.

@oltolm
Copy link
Contributor Author

oltolm commented Feb 6, 2025

I think they are needed by glslang now, I am not sure, I'll double check. It does not build for different reasons. On Windows I didn't add the new submodules to MSVC solution because I don't use it. On other systems that use CMake the problems are different.

@hrydgard
Copy link
Owner

hrydgard commented Feb 7, 2025

Well, I don't want to add code that we're not using. I'm doubtful this is needed since it's not needed on the other platforms. We carry all the headers and stuff we need in the current dependencies.

@oltolm
Copy link
Contributor Author

oltolm commented Feb 7, 2025

This is from glslang\CHANGES.md

## 14.1.0 2024-03-08
* Add SPIRV-Tools-opt dependency if ENABLE_OPT

If I disable ENABLE_OPT then CMake prints the warning

spirv-tools not linked - illegal SPIRV may be generated for HLSL

To disable this warning ENABLE_HLSL needs to be disabled.

option(ENABLE_HLSL "Enables HLSL input support" ON)

What should I do? Should I disable ENABLE_OPT and ENABLE_HLSL?

@hrydgard
Copy link
Owner

hrydgard commented Feb 10, 2025

ENABLE_HLSL should be disabled. We do not use HLSL as input to glslang currently.

In addition, ENABLE_OPT should also be fine to disable. Though it may be interesting to experiment with using it at some point.

@oltolm oltolm changed the title Update glslang submodule and add SPIRV-Tools and SPIRV-Headers submodules Update glslang submodule Feb 10, 2025
@hrydgard
Copy link
Owner

This PR still adds the submodules, please remove them.

@oltolm
Copy link
Contributor Author

oltolm commented Feb 10, 2025

I have removed them.

@oltolm oltolm force-pushed the 3rdparty branch 2 times, most recently from fa225de to 08a0167 Compare February 10, 2025 21:29
@hrydgard
Copy link
Owner

Still doesn't build. Seems there's some inconsistency in C++ edition parameters?

@oltolm
Copy link
Contributor Author

oltolm commented Feb 18, 2025

I need some help here.

iOS build is failing with

/Applications/Xcode_15.4.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS17.5.sdk/usr/include/c++/v1/__filesystem/path.h:509:3: note: '~path' has been explicitly marked unavailable here
  ~path() = default;
  ^
In file included from <built-in>:1:
In file included from /Users/runner/work/ppsspp/ppsspp/build-ios/ext/glslang/glslang/CMakeFiles/glslang.dir/cmake_pch.hxx:5:
In file included from /Users/runner/work/ppsspp/ppsspp/ext/glslang/glslang/MachineIndependent/pch.h:44:
In file included from /Users/runner/work/ppsspp/ppsspp/ext/glslang/glslang/MachineIndependent/SymbolTable.h:70:
/Users/runner/work/ppsspp/ppsspp/ext/glslang/glslang/MachineIndependent/../Include/InfoSink.h:112:60: error: 'string' is unavailable: introduced in iOS 13.0
                append(std::filesystem::absolute(location).string());
                                                           ^

Needs a newer iOS version?

macOS fails with

In file included from <built-in>:1:
In file included from /Users/runner/work/ppsspp/ppsspp/build/ext/glslang/glslang/CMakeFiles/glslang.dir/cmake_pch_x86_64.hxx:5:
In file included from /Users/runner/work/ppsspp/ppsspp/ext/glslang/glslang/MachineIndependent/pch.h:44:
In file included from /Users/runner/work/ppsspp/ppsspp/ext/glslang/glslang/MachineIndependent/SymbolTable.h:70:
/Users/runner/work/ppsspp/ppsspp/ext/glslang/glslang/MachineIndependent/../Include/InfoSink.h:112:50: error: 'path' is unavailable: introduced in macOS 10.15
                append(std::filesystem::absolute(location).string());
                                                 ^

needs a newer macOS version?

Android fails with

In file included from ../ext/glslang-build/../glslang/glslang/GenericCodeGen/Link.cpp:40:
In file included from ../ext/glslang-build/../glslang/glslang/GenericCodeGen/../Include/ShHandle.h:48:
../ext/glslang-build/../glslang/glslang/GenericCodeGen/../Include/InfoSink.h:108:20: error: no member named 'filesystem' in namespace 'std'; did you mean 'std::__fs::filesystem'?
            append(std::filesystem::absolute(shaderFileName).string());
                   ^~~~~~~~~~~~~~~
                   std::__fs::filesystem
/opt/hostedtoolcache/ndk/r21e/x64/sources/cxx-stl/llvm-libc++/include/filesystem:259:1: note: 'std::__fs::filesystem' declared here
_LIBCPP_BEGIN_NAMESPACE_FILESYSTEM
^
/opt/hostedtoolcache/ndk/r21e/x64/sources/cxx-stl/llvm-libc++/include/__config:763:58: note: expanded from macro '_LIBCPP_BEGIN_NAMESPACE_FILESYSTEM'
  _LIBCPP_BEGIN_NAMESPACE_STD namespace __fs { namespace filesystem {
                                                         ^
In file included from ../ext/glslang-build/../glslang/glslang/GenericCodeGen/Link.cpp:40:
In file included from ../ext/glslang-build/../glslang/glslang/GenericCodeGen/../Include/ShHandle.h:48:
../ext/glslang-build/../glslang/glslang/GenericCodeGen/../Include/InfoSink.h:112:24: error: no member named 'filesystem' in namespace 'std'; did you mean 'std::__fs::filesystem'?
                append(std::filesystem::absolute(location).string());

no idea why.

hrydgard added a commit that referenced this pull request Feb 18, 2025
@hrydgard
Copy link
Owner

hrydgard commented Feb 18, 2025

Once #20000 is in, try rebasing on master. Should fix at least the Android failures..

@hrydgard
Copy link
Owner

hrydgard commented Feb 18, 2025

Hm, the Mac/iOS issue seems worse :/ But might just have to increase the required versions, though I don't like doing that...

Or we just stick with old glslang, it works..

@hrydgard
Copy link
Owner

Hm, apparently we need to link to something too in the Android.mk / Locals.mk script...

I hate it when dependencies start using way too new C++ features just because..

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.

2 participants