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

CMake for the C target #402

Merged
merged 76 commits into from
Jul 25, 2021
Merged

CMake for the C target #402

merged 76 commits into from
Jul 25, 2021

Conversation

Soroosh129
Copy link
Contributor

@Soroosh129 Soroosh129 commented Jul 10, 2021

This branch adds the option of compiling generated C target code using cmake. This can be enabled by using the cmake: true target property.

This branch is built on top of the standalone-RTI branch, which should be merged before this.

@Soroosh129
Copy link
Contributor Author

I believe this branch is ready to merge. I have changed the default build system to be CMake and added CMake to REQUIREMENTS.md. Is there any other documentation that needs to be modified?

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Great progress toward a more modular code base! I left a few minor comments...

@edwardalee
Copy link
Collaborator

The example given in Writing-Reactors-in-C assumes cmake is not being used. I think this needs to be updated.

@Soroosh129
Copy link
Contributor Author

The example given in Writing-Reactors-in-C assumes cmake is not being used. I think this needs to be updated.

Okay, thank you for pointing that out. I have added the CMake-related target parameters to the text. For now, it includes a mention to the variable LF_MAIN_TARGET, which should be renamed to LF_MAIN_REACTOR. I think we need to do that in unison with the C++ target to avoid confusion, perhaps. What do you think @cmnrd?

@lhstrh
Copy link
Member

lhstrh commented Jul 25, 2021

The example given in Writing-Reactors-in-C assumes cmake is not being used. I think this needs to be updated.

Okay, thank you for pointing that out. I have added the CMake-related target parameters to the text. For now, it includes a mention to the variable LF_MAIN_TARGET, which should be renamed to LF_MAIN_REACTOR. I think we need to do that in unison with the C++ target to avoid confusion, perhaps. What do you think @cmnrd?

How about we create an issue for this and just continue with the merge?

@Soroosh129
Copy link
Contributor Author

The example given in Writing-Reactors-in-C assumes cmake is not being used. I think this needs to be updated.

Okay, thank you for pointing that out. I have added the CMake-related target parameters to the text. For now, it includes a mention to the variable LF_MAIN_TARGET, which should be renamed to LF_MAIN_REACTOR. I think we need to do that in unison with the C++ target to avoid confusion, perhaps. What do you think @cmnrd?

How about we create an issue for this and just continue with the merge?

Okay sounds good. I have created #429.

@Soroosh129 Soroosh129 merged commit ca8602a into master Jul 25, 2021
@cmnrd cmnrd deleted the c-cmake branch March 10, 2023 15:14
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