Executable folder utilities #77

MathiasMagnus · 2023-04-24T14:40:09Z

Motivation

One of the annoying aspects of OpenCL/GL, Vulkan, etc. is the fact that locating assets in general on disk is a pain. Of course large projects solve this using some utility, but toy projects that are still enjoying their dependency-free world are stuck with what the CRT/STL provides if they wish to be portable (a bit like SDK samples).

fopen and ifstream both interpret the paths relative to the current working directory (CWD), which is unfortunate, because execution success now depends on launching our code from a specific location our code assumes. This assumption definitely blows up in unsuspecting users' faces. C++ luckily has a passable solution: std::filesystem::canonical(argv[0]) which by some magical alignment happens to works on all compilers (CE recently broke execution for MSVC compilers, so those without a working Win env just have to trust me on that it works). Note that even this isn't ideal, because one has to relay the argv symbol all the way to where this translation occurs. In sample code, that's okay, but more complex programs want argv out of thin air. (D2567 targets just this, so maybe in C26 🤞🏻) A mildly elegant solution would be #embed in C23, but mandating a C23 compiler in 2023 is reckless at best. Unfortunately C18 and under doesn't have a similar feature in the CRT, therefore our C samples are subject to CWD relativeness. Seeing this blow up in my students' (and co-lecturers') faces makes me sad. 😞

Proposed solution

This problem needs a solution. I cooked up a utility for OpenCLUtils.lib. It follows the idiom of clGetXyzInfo() functions, first allocate, then return value. It's not dependent on argv so one may invoke it in the depths of any library init. It wraps whereami, a project with the most permissive dual-license to date: MIT/WTFPLv2. The use of this lib is an implementation detail, it does not leak to consumers of the utility library.

The samples in my opinion became much nicer and now they don't fail, regardless of where they're launched from.

Changes introduced

Added the previously mentioned utility libraries
Updated all samples using kernels to use them
Add an overdue option OPENCL_SDK_BUILD_UTILITY_LIBRARIES CMake option
- As courtesy to those not needing them, especially how the non-SDK utility picked up it's 1st dependency
Breaking change: added default arguments to all C++ file utility functions. I believe it was a bug that these default args were absent. Intention was to follow the behavior of opencl.hpp in all regards and make a utility library that is a natural extension to that.

keryell

That looks like a good improvement.

keryell · 2023-04-26T11:56:12Z

cmake/Dependencies/whereami/whereami.cmake

+)
+target_sources(whereami
+  INTERFACE "${CMAKE_CURRENT_BINARY_DIR}/_deps/whereami-external-src/src/whereami.c"
+)


Configure your editor to add EOL?

keryell · 2023-04-26T11:58:55Z

cmake/Dependencies/whereami/whereami.cmake

+  INTERFACE "${CMAKE_CURRENT_BINARY_DIR}/_deps/whereami-external-src/src"
+)
+target_sources(whereami
+  INTERFACE "${CMAKE_CURRENT_BINARY_DIR}/_deps/whereami-external-src/src/whereami.c"


Do you need this because you want to cope with old CMake?
Or just because whereami does not use modern CMake features.
You can clarify my brain by adding comments in the CMake configuration. :-)

Doing this because whereami is lacking CMake support completely.

lib/Utils.md

lib/src/Utils/File.c

keryell · 2023-04-26T12:10:27Z

samples/extensions/khr/nbody/main.cpp

-                                       shader_stream },
-                                   std::istreambuf_iterator<GLchar>{} };
+        std::string shader_string =
+            cl::util::read_exe_relative_text_file(file_path.c_str());


Add an overload taking a std::string. Actually a string_view would be even better when it is available.

I would like to make that a separate discussion, because some of the overloads would need to be version guarded.

I agree it would be cleaner if the C++ version had a const std::string& overload, which shouldn't need any version guards. This could be done in a subsequent PR, though.

Co-authored-by: Ronan Keryell <ronan.keryell@amd.com>

bashbaug

Found one typo, otherwise LGTM.

lib/Utils.md

bashbaug · 2023-11-13T23:56:52Z

samples/extensions/khr/nbody/main.cpp

-                                       shader_stream },
-                                   std::istreambuf_iterator<GLchar>{} };
+        std::string shader_string =
+            cl::util::read_exe_relative_text_file(file_path.c_str());


I agree it would be cleaner if the C++ version had a const std::string& overload, which shouldn't need any version guards. This could be done in a subsequent PR, though.

samples/extensions/khr/nbody/main.cpp

bashbaug · 2023-11-13T23:59:49Z

@keryell can you please check if your comments have been addressed? We'd like to merge this shortly - thanks!

Co-authored-by: Ben Ashbaugh <ben.ashbaugh@intel.com>

keryell

Thanks.

bashbaug · 2023-11-16T00:08:32Z

Not sure what's caused the issue with the Windows builds but they do appear to be "real":

D:\a\OpenCL-SDK\OpenCL-SDK\samples\extensions\khr\conway\main.cpp(51,26): error C2397: conversion from 'sf::Style::' to 'sf::Uint32' requires a narrowing conversion [D:\a\OpenCL-SDK\OpenCL-SDK\build\samples\extensions\khr\conway\conwaycpp.vcxproj]

MathiasMagnus · 2023-11-16T14:15:10Z

@bashbaug @keryell I added a few fixes to the tip of the branch, partly to fix what Ben pointed out and addressed some warnings.

The sf:::Uint32 conversion is an SFML API facepalm. sf::Window takes its style enum flag as an sf::Uint32, however the enum which supplies the valid values is an anonymous enum. Our cl::sdk::InteropWindow followed sf::Window's suit and took the style param as an unsigned integer, but the default underlying type of the anon enum is int, hence the narrowing conversion.

I changed the API of cl::sdk::InteropWindow to take the underlying type of the enum in its CTOR and converts it internally before handing it over to sf::Window. This API change does not leak to consumers, because the SDK lib isn't exported, there we're free to change the API.

GCC also argued about unhandled switch cases, and adding a default there is safe as we don't intend to handle all cases anyway, just the ones relevant to us.

bashbaug

LGTM, and all CI is passing now 👍

I'll just merge this. Thanks!

MathiasMagnus added 7 commits April 24, 2023 08:20

Add BUILD_UTILITY_LIBRARIES option

f1b99df

Add whereami dependence

5393785

Add exe relative utilities

5778f03

Update samples to use exe relative utilities

8d3feeb

Improve diagnostic on missing file

585e900

Add missing default argument for error param

79c3fff

Add docs on file utilities

8e41898

MathiasMagnus requested review from Kerilk, keryell and bashbaug April 24, 2023 19:40

keryell requested changes Apr 26, 2023

View reviewed changes

MathiasMagnus and others added 3 commits November 7, 2023 18:33

Add EOL

207081b

Fix typo

582ddd5

Co-authored-by: Ronan Keryell <ronan.keryell@amd.com>

Simplify byte size calculation

da37358

Co-authored-by: Ronan Keryell <ronan.keryell@amd.com>

bashbaug approved these changes Nov 13, 2023

View reviewed changes

Fix typo

2ffa495

Co-authored-by: Ben Ashbaugh <ben.ashbaugh@intel.com>

keryell approved these changes Nov 15, 2023

View reviewed changes

Fix formatting

8cc4341

MathiasMagnus added 3 commits November 16, 2023 10:52

Remove implicit narrowing conversions

531f3a4

No unnamed type on libSDK surface

83b8048

warning: enumeration value x not handled in switch

b9633c8

MathiasMagnus requested review from keryell and bashbaug November 16, 2023 14:15

bashbaug approved these changes Nov 16, 2023

View reviewed changes

bashbaug merged commit de7cb8a into KhronosGroup:main Nov 16, 2023
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Executable folder utilities #77

Executable folder utilities #77

MathiasMagnus commented Apr 24, 2023

keryell left a comment

keryell Apr 26, 2023

MathiasMagnus Nov 7, 2023

keryell Apr 26, 2023

MathiasMagnus Nov 7, 2023

keryell Apr 26, 2023

MathiasMagnus Nov 7, 2023

bashbaug Nov 13, 2023

bashbaug left a comment

bashbaug Nov 13, 2023

bashbaug commented Nov 13, 2023

keryell left a comment

bashbaug commented Nov 16, 2023

MathiasMagnus commented Nov 16, 2023

bashbaug left a comment

Executable folder utilities #77

Executable folder utilities #77

Conversation

MathiasMagnus commented Apr 24, 2023

Motivation

Proposed solution

Changes introduced

keryell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bashbaug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bashbaug commented Nov 13, 2023

keryell left a comment

Choose a reason for hiding this comment

bashbaug commented Nov 16, 2023

MathiasMagnus commented Nov 16, 2023

bashbaug left a comment

Choose a reason for hiding this comment