Skip to content

Conversation

@eessmann
Copy link
Contributor

Removed unnecessary path normalization and appending for installation.
closes #696

Removed unnecessary path normalization and appending for installation.
@otbrown otbrown self-assigned this Oct 24, 2025
@otbrown
Copy link
Contributor

otbrown commented Oct 24, 2025

Thanks for this Erich, I have two minor requests before merge:

  • Not related to your changes but I see line 578 of CMakeLists.txt says ## RATH instead of ## RPATH!
  • Could you change the executable we always build to be quest-min-example rather than min_example. If no CMAKE_INSTALL_PREFIX is set this will now end up in /usr/local/bin so we should namespace it...

@otbrown
Copy link
Contributor

otbrown commented Oct 24, 2025

Making the binaries specifically still be prepended by quest/ would also be fine I think, although is possibly more faff! Though actually I'm now realising that all the examples may end up dumped in /usr/local/bin so we probably do want to make sure a quest gets inserted in there 🤔

@eessmann
Copy link
Contributor Author

I don't think we should install the executables, they are all just examples.

@otbrown
Copy link
Contributor

otbrown commented Oct 27, 2025

Good point. I was pondering over the weekend, and I can think of one or two awkward workflows where the user might want binaries installed too (i.e. when the user sets an install prefix and is using the USER_SOURCE option), so rather than remove the binary install entirely I was thinking ye olde create a CMake option.

option(INSTALL_BINARIES "Whether to include example and user binaries in the install." OFF) means the default behaviour is unsurprising, but people are supported in being weird without having to modify the CMake themselves.

Modifies CMakeLists to conditionally build shared libraries and
install binaries only at the top-level project. Introduces the
INSTALL_BINARIES option to control the inclusion of example
binaries in the installation process. Corrects a typo from
'RATH' to 'RPATH' for build configurations.
@eessmann
Copy link
Contributor Author

Oliver, I’ve added your suggested option so we’re now conditionally installing the examples. I’ve also made it so that if we’re the top-level CMake project, we set the type of library we build to shared.

I encountered a bug when using Quest with the CMake FetchContent feature; Quest’s BUILD_SHARED_LIBS setting leaked into my build.

@otbrown
Copy link
Contributor

otbrown commented Oct 27, 2025

Excellent, thanks Erich!

@otbrown otbrown merged commit 5048463 into QuEST-Kit:devel Oct 27, 2025
130 checks passed
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