Skip to content

#550 cmake build is missing key features to be properly usable via fetchcontent #551

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
May 23, 2022
Merged

#550 cmake build is missing key features to be properly usable via fetchcontent #551

merged 9 commits into from
May 23, 2022

Conversation

globberwops
Copy link
Contributor

@globberwops globberwops commented Jul 20, 2021 •
edited
Loading

Reference to a related issue in the repository

Resolves #550

Add a description

This PR aims to modernize the installation logic of the project by:

  • removing redundant warnings
  • prefix cache variables with the project name
  • provide namespaced alias targets usable from the build tree
  • provide a namespace for the export set

This modernization will make the project usable via FetchContent by a downstream project.

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

  • My suggestion follows the style and contributors guidelines.
  • I have taken care about the documentation.
  • I have done the DCO signoff.
  • My changes generate no errors when passing CI tests.
  • I have successfully implemented and tested my fix/feature locally.
  • Appropriate reviewer(s) are assigned.

@tbleher
Copy link
Contributor

tbleher commented Aug 20, 2021

Thank you for the CMake improvements! I have not used all the CMake constructs you use here, but from what I see this all looks sensible :)

@ThomasNaderBMW ThomasNaderBMW self-requested a review August 23, 2021 07:53
ThomasNaderBMW
ThomasNaderBMW approved these changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@ThomasNaderBMW ThomasNaderBMW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @tbleher for the feedback.

From my side: I tested the basic functionality, so to build the osi-project and had no issues.

So together a +1 :)

@globberwops
Copy link
Contributor Author

@tbleher and @ThomasNaderBMW Thanks for your feedback.
I have merged the latest master branch and fixed the new flatbuffers option.
C++11 is now required, please tell me if this is intended or not.
I have also added the toplevel project check to restrict the doc target.

@globberwops globberwops changed the title WIP: #550 cmake build is missing key features to be properly usable via fetchcontent #550 cmake build is missing key features to be properly usable via fetchcontent Oct 7, 2021
@PhRosenberger PhRosenberger added this to the V3.5.0 milestone Jan 19, 2022
@stefancyliax stefancyliax added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Jan 19, 2022
@LukasElster
Copy link
Contributor

Our simulation models are still building with the changes. Therefore the changes are fine for me

@kmeids
Copy link

kmeids commented Jan 19, 2022

Output CCB 19.01.2022:

  1. @pmai to check the effect/dependencies of these changes on OSMP/OSI-Validator.

@kmeids
Copy link

kmeids commented Feb 2, 2022

Output CCB 02.02.2022:

  1. @pmai to check the effect/dependencies of these changes on OSMP/OSI-Validator.

@kmeids kmeids removed the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Feb 2, 2022
@kmeids
Copy link

kmeids commented Apr 25, 2022

Output CCB 25.04.2022:

  1. @pmai and @ThomasNaderBMW, could you please bring this request to the SetLevel agenda to check if it matches with the latest changes?.

@ThomasNaderBMW ThomasNaderBMW added the ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. label May 16, 2022
@ThomasNaderBMW
Copy link
Contributor

Output CCB 16.05.2022: Can be merged.

@globberwops @pmai
Prefix cache variables with OSI_ identifier
2d3587c 
Use option() to set cache variables

Signed-off-by: Martin Stump <11492152+globberwops@users.noreply.github.com>
@globberwops @pmai
OpenSimulationInterface#550 Provide namespaced aliases for all targets
c9b3dd4 
Signed-off-by: Martin Stump <11492152+globberwops@users.noreply.github.com>
@globberwops @pmai
OpenSimulationInterface#550 Rewrite the whole installation logic
120460b 
* Fix the cache variables
* Copy the headers to the appropriate location in the PROJECT_BINARY_DIR
* Rewrite the package config file template
* Create proper config, version, and target files
  using CMakePackageConfigHelpers
* Export and install namespaced targets

Signed-off-by: Martin Stump <11492152+globberwops@users.noreply.github.com>
@globberwops @pmai
OpenSimulationInterface#550 Remove redundant warning about Doxygen
8e435ff 
Signed-off-by: Martin Stump <11492152+globberwops@users.noreply.github.com>
@globberwops @pmai
OpenSimulationInterface#550 Add compile_commands.json to ignore list
c756766 
Signed-off-by: Martin Stump <11492152+globberwops@users.noreply.github.com>
@globberwops @pmai
OpenSimulationInterface#550 Configure osi_version.proto.in in binary dir
0907378 
Signed-off-by: Martin Stump <11492152+globberwops@users.noreply.github.com>
@globberwops @pmai
OpenSimulationInterface#550 Fix flatbuffers option
2f0c668 
Signed-off-by: Martin Stump <11492152+globberwops@users.noreply.github.com>
@globberwops @pmai
OpenSimulationInterface#550 Require C++ 11
5083adc 
Signed-off-by: Martin Stump <11492152+globberwops@users.noreply.github.com>
@globberwops @pmai
OpenSimulationInterface#550 Create doc target only if project is topl…
5a02aed 
…evel

Signed-off-by: Martin Stump <11492152+globberwops@users.noreply.github.com>
@pmai pmai force-pushed the 550-cmake-build-is-missing-key-features-to-be-properly-usable-via-fetchcontent branch from be31c99 to 5a02aed Compare May 23, 2022 06:07
@pmai pmai merged commit d553d1c into OpenSimulationInterface:master May 23, 2022
@pmai pmai mentioned this pull request Jun 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomasNaderBMW ThomasNaderBMW ThomasNaderBMW approved these changes

@LukasElster LukasElster LukasElster approved these changes

Assignees
No one assigned
Labels
ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Milestone
V3.5.0
Development

Successfully merging this pull request may close these issues.

CMake build is missing key features to be properly usable via FetchContent
8 participants
@globberwops @tbleher @LukasElster @kmeids @ThomasNaderBMW @pmai @PhRosenberger @stefancyliax