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: minor changes to improve the CLion IDE CMake experience #474

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

stanislaw
Copy link
Contributor

@stanislaw stanislaw commented Jan 10, 2020

Describe the contribution

This is needed when CFE's root CMakeLists.txt is not the highest level CMakeLists.txt in the project. My IDE is CLion and it works best when there is the highest level CMakeLists.txt in the root of a project's source code.

This changeset enables a necessary indirection: from CMAKE_SOURCE_PATH to CFE_SOURCE_PATH and this allows building the project with the CMakeLists.txt file in the top-level mission source directory.

Step 1. I have cloned the root cFS repository and have cloned all submodules.
Step 2. I have created a CMakeLists.txt file in the cFS folder with the following contents:

cmake_minimum_required(VERSION 3.13)
add_subdirectory(cfe)

Step 3. I have made the changes from this changeset.

After this sequence of steps, the CLion IDE picks up the root-level CMakeLists automatically and I can already start building targets, such as cpu1-all, mission-all etc. And the only parameter I need to configure CMake is: -DMISSION_SOURCE_DIR=/sandbox/cFS.

Testing performed

Please see the steps above.

Expected behavior changes

With this change, getting IDE to recognize the cFS build targets is way easier if your IDE is CLion.

System(s) tested on:

Contributor Info

Stanislav Pankevich (PTS GmbH, private German space company).

Community contributors

You must attach a signed CLA (required for acceptance) or reference one already submitted

The corporate CLA has been signed on Feb 17th and sent to the email address as specified in the CLA document.

@stanislaw
Copy link
Contributor Author

You must attach a signed CLA (required for acceptance) or reference one already submitted

I am willing to do what is needed but I don't know what I have to do. Please advise.

@stanislaw stanislaw changed the title CMake: minor changes to improve the CLion IDE CMake experience WIP: CMake: minor changes to improve the CLion IDE CMake experience Jan 10, 2020
@stanislaw
Copy link
Contributor Author

I have realized that one more change is needed and it requires also making change in another repository. In our CMake-based project, we have found it very handy to use the CMAKE_BINARY_DIR/bin convention for all of our binary artifacts.

With this, the path to the elf2cfetbl is fixed to be in the bin/ folder, relative to MISSION_BINARY_DIR.

       add_custom_command(
         OUTPUT "${TABLE_DESTDIR}/${TBLWE}.tbl"
         COMMAND ${CMAKE_C_COMPILER} ${TBL_CFLAGS} -c -o ${TBLWE}.o ${TBL_SRC}
-        COMMAND ${MISSION_BINARY_DIR}/tools/elf2cfetbl/elf2cfetbl ${TBLWE}.o
-        DEPENDS ${MISSION_BINARY_DIR}/tools/elf2cfetbl/elf2cfetbl ${TBL_SRC}
+        COMMAND ${MISSION_BINARY_DIR}/bin/elf2cfetbl ${TBLWE}.o
+        DEPENDS ${MISSION_BINARY_DIR}/bin/elf2cfetbl ${TBL_SRC}
         WORKING_DIRECTORY ${TABLE_DESTDIR}
       )

and in the tools/elf2cfetbl/ repository the following is needed:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index a4e2fea..fd2cad9 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -8,4 +8,7 @@ add_executable(elf2cfetbl elf2cfetbl.c)

 install(TARGETS elf2cfetbl DESTINATION host)

-
+set_target_properties(elf2cfetbl
+  PROPERTIES
+  RUNTIME_OUTPUT_DIRECTORY "${MISSION_BINARY_DIR}/bin"
+)

If this course of action is approved, I am happy to suggest the needed changes to the tools/elf2cfetbl repository.

@skliper
Copy link
Contributor

skliper commented Jan 10, 2020

You must attach a signed CLA (required for acceptance) or reference one already submitted

I am willing to do what is needed but I don't know what I have to do. Please advise.

Stand by, I just got approval yesterday to get this into our flow.

Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

In general I'm OK with this idea and some of the basic suggestions, except where noted. Note that historically speaking we do not directly "distribute" a top level directory, which is why some of the files are distributed as "samples" (e.g. sample_defs, Makefile.sample, etc) may be manually copied to the top level when getting started.

The idea is that each project fully owns the top level dir, including the configuration and any top level makefile targets.

Also note that the "Makefile.sample" file, if copied to the to level as "Makefile" is also intended for ease of use with IDE's. This allows the IDE to treat it as a regular Makefile project. This makefile has a dependency rule to automatically call CMake to set up the build tree so an IDE can just simply run make at the top level and it does the right thing, with CMake totally hidden behind the scene.

However, for IDEs that are aware of CMake and can integrate with it, I can see where having a top-level CMakeLists.txt file could be of some added value.

CMakeLists.txt Show resolved Hide resolved
cmake/mission_build.cmake Outdated Show resolved Hide resolved
@stanislaw stanislaw force-pushed the root-level-cmake branch 3 times, most recently from 3589e87 to f48a0ea Compare January 11, 2020 16:35
@stanislaw
Copy link
Contributor Author

One general question about the reviews: is it a reviewer who is supposed to resolve the conversations or the author? I have seen both workflows in the wild. Please advise.

@skliper
Copy link
Contributor

skliper commented Jan 13, 2020

One general question about the reviews: is it a reviewer who is supposed to resolve the conversations or the author? I have seen both workflows in the wild. Please advise.

Preference is the author/contributor of the pull request.

@skliper
Copy link
Contributor

skliper commented Jan 13, 2020

You must attach a signed CLA (required for acceptance) or reference one already submitted

I am willing to do what is needed but I don't know what I have to do. Please advise.

I just updated the pull request template, and added the associated forms. See https://github.com/nasa/cFE/blob/master/docs/GSC_18128_Corp_CLA_form_1219.pdf with submission instructions on the bottom.

Email is preferred, and when complete could you let me know (still working the bugs out of the process)?

@stanislaw
Copy link
Contributor Author

Preference is the author/contributor of the pull request.

Got it. Thanks!

I just updated the pull request template, and added the associated forms. See https://github.com/nasa/cFE/blob/master/docs/GSC_18128_Corp_CLA_form_1219.pdf with submission instructions on the bottom.

Email is preferred, and when complete could you let me know (still working the bugs out of the process)?

I have passed the agreement to my manager and our lawyer. l guess we could sign it within one or two days. I will notify you here.

@skliper skliper added the CCB:PendingCLA External contribution pending CLA confirmation label Jan 22, 2020
@stanislaw
Copy link
Contributor Author

Just a small update here: the papers are still not signed by our company for @matzipan and me @stanislaw. We will keep you updated.

@stanislaw
Copy link
Contributor Author

@jphickey @skliper sorry for the delay with our responses (me @stanislaw and @matzipan). We finally got the approval from our management for the CLAs to be signed. Our question is: should we sign the CLAs for the current version or should we wait until you integrate current changes and then we would sign the next version? Thanks.

@skliper
Copy link
Contributor

skliper commented Feb 14, 2020

@stanislaw thanks for pushing the paperwork side of things. The current CLAs are what you need to sign to allow us to integrate contributions into the next release (they refer to the released version you are contributing to, not the next release version), so please sign the current versions.

@stanislaw stanislaw changed the title WIP: CMake: minor changes to improve the CLion IDE CMake experience CMake: minor changes to improve the CLion IDE CMake experience Feb 20, 2020
This is needed when CFE's root CMakeLists.txt is not the highest level
CMakeLists.txt in the project.
@stanislaw
Copy link
Contributor Author

@stanislaw thanks for pushing the paperwork side of things. The current CLAs are what you need to sign to allow us to integrate contributions into the next release (they refer to the released version you are contributing to, not the next release version), so please sign the current versions.

The signed corporate CLA has been sent to the GSFC-SoftwareRelease email on February 17th. I have updated the description of the PR accordingly.

@skliper
Copy link
Contributor

skliper commented Feb 25, 2020

@stanislaw could you confirm the corporate forms were sent? I just checked on this side and all we have are your two individual forms.

@stanislaw
Copy link
Contributor Author

Yes, I confirm that we have sent the email Corporate CLA signed for the NASA Core Flight System's CFE project: Planetary Transporation Systems GmbH (Andrei Zisu, Stanislav Pankevich) to the [email protected] email on February 17th.

@skliper
Copy link
Contributor

skliper commented Mar 2, 2020

@stanislaw Confirmed reception on our side, thank you for your patience!

@skliper skliper removed the CCB:PendingCLA External contribution pending CLA confirmation label Mar 2, 2020
@skliper skliper requested a review from jphickey March 2, 2020 15:44
@skliper
Copy link
Contributor

skliper commented Apr 14, 2020

@jphickey are the issues you had with this resolved?

@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 14, 2020
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

Yes, this looks good to me. Seems low-impact enough.

@astrogeco
Copy link
Contributor

CCB 20200415 - APPROVED

@astrogeco astrogeco added CCB:Approved Indicates code review and approval by community CCB CCB - 20200415 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 20, 2020
@astrogeco astrogeco changed the base branch from master to integration-candidate April 21, 2020 22:17
@astrogeco astrogeco merged commit 5a1c7e8 into nasa:integration-candidate Apr 21, 2020
@astrogeco astrogeco added docs This change only affects documentation. and removed docs This change only affects documentation. labels Apr 27, 2020
@skliper skliper added this to the 6.8.0 milestone Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants