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

Allow removal of text loggers from build with CMake variable. #1071

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

nabcouwer
Copy link
Contributor

Allow removal of text loggers from build with CMake variable. Otherwise build fails if FW_ENABLE_TEXT_LOGGING == 0.

Originating Project/Creator JPL COLDArm / Neil Abcouwer
Affected Component Build Process
Affected Architectures(s) Any
Related Issue(s) n/a
Has Unit Tests (y/n) n/a
Builds Without Errors (y/n) y
Unit Tests Pass (y/n) n/a
Documentation Included (y/n) n

Change Description

In https://github.com/nasa/fprime/blob/devel/Svc/CMakeLists.txt , move adding of Text Logger components into a if (NOT FPRIME_DISABLE_TEXT_LOGGERS) conditional.

Rationale

If FW_ENABLE_TEXT_LOGGING is set to 0 (

#define FW_ENABLE_TEXT_LOGGING 1 //!< Indicates whether text logging is turned on
) , the text logger components, ActiveTextLogger and PassiveConsoleTextLogger, will fail to build, as will the entire build.

Now, deployments that wish to set FW_ENABLE_TEXT_LOGGING to 0 can add FPRIME_DISABLE_TEXT_LOGGERS = 1 to their CMakeLists to prevent the adding of these components.

Testing/Review Recommendations

None

Future Work

There may be a more desirable way to do this.

…se build fails if FW_ENABLE_TEXT_LOGGING == 0.
@@ -40,6 +38,13 @@ add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/Time/")
add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/TlmChan/")
add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/TlmPacketizer/")

# Allow and check for disabling of text loggers
# Building these fails if FW_ENABLE_TEXT_LOGGING == 0
if (NOT FPRIME_DISABLE_TEXT_LOGGERS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is exactly the way I'd disable these from the build. Two comments:

  1. Add an option in cmake/Options.cmake this makes the switch formal, and allows you to set a default.
  2. Our other switches are named in the positive. e.g. FPRIME_ENABLE_TEXT_LOGGERS with an appropriate default.

@nabcouwer
Copy link
Contributor Author

@LeStarch , thanks for the feedback, changes made.

@LeStarch
Copy link
Collaborator

LeStarch commented Oct 4, 2021

This is great! I'll let CI run and merge when it passes!

@LeStarch LeStarch merged commit 00dd602 into nasa:devel Oct 4, 2021
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.

3 participants