KhronosGroup / OpenCL-SDK

OpenCL SDK
Apache License 2.0
578 stars 120 forks source link

confirm intended install directories after merging CI/CD changes #112

Closed bashbaug closed 1 month ago

bashbaug commented 1 month ago

We'll check that the install directories are working as we intend after merging. Specifically, with a non-multi-config generator (e.g. Makefiles), does the install go into per-config directories, or into a single lib or bin directory?

Originally posted by @bashbaug in https://github.com/KhronosGroup/OpenCL-SDK/issues/88#issuecomment-2386424254

bashbaug commented 1 month ago

I checked the install directories on my Linux system with the latest changes, after merging #88. Here is what I found, after building with the Makefile generator:

  1. The OpenCL headers and C++ bindings go into a single "include" directory.
  2. The OpenCL ICD loader goes into a single "lib" directory.
  3. The cllayerinfo utility that is built with the OpenCL ICD loader goes into a single "bin" directory.
  4. clinfo also goes into the same single "bin" directory.
  5. The OpenCL SDK utility headers go into a single "include" directory.
  6. The OpenCL SDK libraries go into a single "lib" directory.
  7. Any supporting libraries for the samples (e.g. sfml, cargs) goes into the same single "lib" directory.
  8. The sample applications and kernels go into a per-config "bin" directory, for example "bin/Release".

This seems like the opposite of what was described in the October 1st teleconference.

Do we intend for the sample applications and kernels in (8) to be installed into a per-config "bin" directory? Note, this is different behavior than we had before merging #88.

CC @MathiasMagnus @Beanavil

Beanavil commented 1 month ago

@bashbaug hm yes looking again at the logic of this CMakeLists file it does seem like it will install the kernels and executables in /<install_dir>/bin/<config> regardless of the generator used. If we want to maintain the previous behaviour for the non-multiconfig generators, I think this is easily fixable.

However, I've been thinking about why would this be a problem. You did mention that it could affect packaging, but as far as I can remember there was no binary packaging for the SDK before and also afaik we do not package the samples' binaries anyway, or am I missing something here?

MathiasMagnus commented 1 month ago

@bashbaug Generally speaking, the OpenCL-SDK's build tree isn't guaranteed to be stable, not that it's even possible, given how different it is with auto-built deps vs. user-provided pre-built deps. Our install tree is intended to be stable and wishes to emulate a proper *nix layout, mostly replicated by the GNUInstallDirs module. As such the intention was/is:

Windows behaving as it does necessitates some changes due to ABI reasons. Some changes as compelling as they may be can't be done in a backward compatible manner, but the newer components did get tweaks early on. One such early break was OpenCLUtilsCpp.lib getting a d suffix in Debug builds to be able to handle STL types on its interface. (Much like how other projects do it, for eg. our dep SFML has d and s suffixes in all combinations for debug and static builds.) This was to allow installing Debug and Release builds side-by-side.

The bin\Release and bin\Debug folder are indeed Windows-isms, commonly used by applications. We've already added the filename suffix to the OpenCLUtils[Cpp] so it's not strictly necessary IF we decide that the samples along with the SDK lib should behave like the loader itself, where configs overwrite each other and only the latest install remains on disc.

To enumerate all all your findings:

  1. It has always been that way since the inception of the OpenCL-SDK, our very first binary packages for Windows did that. Moreover this is how all vendor SDKs look like. (The ones I used)
  2. IIRC it has always been that way.
  3. Going by the rule of thumb, it should be that way.
  4. Same here.
  5. Again, we never separated components into different include dirs in our install tree.
  6. I starting to feel we're mixing build/install tree findings, the SDK libs aren't installed.
  7. The support libraries we try to build as static, so we need no install them. Currently some do get installed when using auto-fetched deps, that should be fixed.
  8. See earlier.

My understanding is that the issue is that our build tree looks too much like an install tree with most artifacts going into a singular folder, is that it? The idea of using a singular bin folder (and why not go all the way then) was to have the samples use the same loader we just built.

If it's deemed unorthodox to have such tight grip over the build tree layout and mold so many artifacts into the same folders, it can be left at CMake defaults. Then however, all samples will use the system loader on Windows. When layers came around, system loaders by definition were outdated, and it can happen in the future again, should the loader pick up new capabilities.

bashbaug commented 1 month ago

Discussed in the October 8th teleconference. We're going to go back to the previous behavior, where the sample applications and kernels do NOT going into config-specific install directories.

bashbaug commented 1 month ago

Fixed by #113.