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

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

Closed
bashbaug opened this issue Oct 1, 2024 · 5 comments
Closed

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

bashbaug opened this issue Oct 1, 2024 · 5 comments

Comments

@bashbaug
Copy link
Contributor

bashbaug commented Oct 1, 2024

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 #88 (comment)

@bashbaug
Copy link
Contributor Author

bashbaug commented Oct 1, 2024

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
Copy link
Contributor

Beanavil commented Oct 3, 2024

@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
Copy link
Collaborator

@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:

  • DLLs and executables go in bin
  • import libs and shared objects go in lib,
  • headers go into include,
  • arch-independent data goes into share.

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.

  • On Linux, RPATH is handled in the build tree automatically (see docs) but needs the INSTALL_RPATH property in the install tree (which we set).
  • On Windows, in the absence of an RPATH-like mechanism, best we can do is copy all DLLs next to the executables. This can conveniently be done in the install tree via install(RUNTIME_DEPENDENCY_SET) (which we use), but it's just annoying in the build tree. We do this for the kernels using utility targets, but it's just extra hassle for every other target, hence the "clean-up" of the build tree to have a singular bin folder.

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
Copy link
Contributor Author

bashbaug commented Oct 8, 2024

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
Copy link
Contributor Author

Fixed by #113.

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

No branches or pull requests

3 participants