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

Removes 1-to-1 relation of deployments and projects #1994

Merged
merged 7 commits into from
May 17, 2023

Conversation

LeStarch
Copy link
Collaborator

@LeStarch LeStarch commented Apr 28, 2023

Originating Project/Creator Infrastructure
Affected Component CMake Architecture
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n) TODO
Builds Without Errors (y/n) y
Unit Tests Pass (y/n) y
Documentation Included (y/n) y

Change Description

This removes the 1-to-1 implicit requirement between register_fprime_deployment and PROJECT_NAME. It also introduces the QoL improvement FPRIME_CURRENT_MODULE, which removes the need to constantly call get_module_name() in fprime modules (added with add_fprime_subdirectory()).

Rationale

Having a project call and thus PROJECT_NAME setting as a base requirement for adding a deployment has set F´ outside a more standard CMake structure (project is the root of a source tree).

In general this policy has the following disadvantages:

  1. Not possible to build a basic repo as a single unified item (standard case)
  2. Reach-around builds (add directory ../something)
  3. Standard CMake tool support weakened (e.g. IDEs break)
  4. Multiple deployments in a single repository require substantial rebuilding of common code
  5. Building at the root level of the repository does not build deployments

All of this structure was done for one very specific reason: to support the case where multiple deployments intended to be built with the same toolchain needed different core F´ settings (setting.ini and configuration folder). This is not the most common use case of F´ but the design does complicate the more common use cases..

By weakening the 1-to-1 requirement, the disadvantages are resolved, while still allowing the more complicated structure to continue to work.

Naturally, this solution has some different drawbacks:

  1. Detecting a topology's dictionary now uses a cache variable. This may cause the dictionary to be installed in a deployment in the erroneous condition where a deployment is defined but does not define its own topology. See Future Work.
  2. Care must be taken with the more complex structure to ensure it is setup properly.

Testing/Review Recommendations

Let CI fly!

Future Work

There is additional work that may be done to ease the drawbacks of this structure:

  1. Create a TOPOLOGY variable to be set when running register_fprime_deployment that specifies what module represents the topology for a given deployment. Default this to deployment/Top as this is standard. The dictionary would then register as a property to this module and the deployment would pick-up the dictionary to install from there.
  2. Add an add_fprime_subproject API call to add a directory as a new independently configured fprime project to ease the use of the complex case.
  3. Add CMake UTs to try and build this structure!

@LeStarch LeStarch requested review from bocchino and thomas-bc April 28, 2023 17:07
@LeStarch
Copy link
Collaborator Author

@bocchino this allows the structure you were thinking of using (multiple deployments to a single project/build).

@bocchino
Copy link
Collaborator

This looks great to me! Looks like CI is not passing yet. I'll re-review when CI passes.

@LeStarch
Copy link
Collaborator Author

LeStarch commented May 2, 2023

@bocchino should be ready!

Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

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

Looks good!

@LeStarch LeStarch merged commit afacd2f into nasa:devel May 17, 2023
)
install(FILES ${FPRIME_CURRENT_DICTIONARY_FILE} DESTINATION ${TOOLCHAIN_NAME}/${MODULE}/dict COMPONENT ${MODULE})
add_custom_command(TARGET "${MODULE}" POST_BUILD COMMAND "${CMAKE_COMMAND}"
-DCMAKE_INSTALL_COMPONENT=${MODULE} -P${CMAKE_BINARY_DIR}/cmake_install.cmake)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm opening #2031 to fix an issue introduced here:

CMake Error: No script specified for argument -P
gmake[3]: *** [CMakeFiles/Ref.dir/build.make:169: bin/Linux/Ref] Error 1
gmake[3]: *** Deleting file 'bin/Linux/Ref'
gmake[2]: *** [CMakeFiles/Makefile2:2352: CMakeFiles/Ref.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:2359: CMakeFiles/Ref.dir/rule] Error 2
gmake: *** [Makefile:184: Ref] Error 2
[ERROR] CMake erred with return code 2
root@2d500cea2ecc:/home/ptl# cmake --version
cmake version 3.18.4

Verification that this is the offending line:

root@2d500cea2ecc:/home/ptl/Ref/build-fprime-automatic-native# /usr/bin/cmake -DCMAKE_INSTALL_COMPONENT=Ref -P/home/ptl/Ref/build-fprime-automatic-native/cmake_install.cmake
CMake Error: No script specified for argument -P

With a space after -P builds work for my version of CMake.

Boehm-Michael pushed a commit to Boehm-Michael/fprime that referenced this pull request Jun 22, 2023
* Removes 1-to-1 relation of deployments and projects

* Fixing CMake UTs

* Making deployment subject to toolchain

* Fixing CI and minimum tools version

* Python formatting and fixing integration test run

* Fixing RPI CI path

* Fixing RPI remote run integration test
thomas-bc pushed a commit that referenced this pull request Aug 4, 2023
* Removes 1-to-1 relation of deployments and projects

* Fixing CMake UTs

* Making deployment subject to toolchain

* Fixing CI and minimum tools version

* Python formatting and fixing integration test run

* Fixing RPI CI path

* Fixing RPI remote run integration test
thomas-bc added a commit that referenced this pull request Aug 4, 2023
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.

4 participants