-
Notifications
You must be signed in to change notification settings - Fork 210
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
CMake: minor changes to improve the CLion IDE CMake experience #474
Conversation
I am willing to do what is needed but I don't know what I have to do. Please advise. |
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 With this, the path to the 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 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 |
Stand by, I just got approval yesterday to get this into our flow. |
There was a problem hiding this 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.
3589e87
to
f48a0ea
Compare
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. |
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)? |
Got it. Thanks!
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. |
Just a small update here: the papers are still not signed by our company for @matzipan and me @stanislaw. We will keep you updated. |
@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. |
@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. |
f48a0ea
to
922c3c5
Compare
This is needed when CFE's root CMakeLists.txt is not the highest level CMakeLists.txt in the project.
922c3c5
to
8ee7c02
Compare
The signed corporate CLA has been sent to the GSFC-SoftwareRelease email on February 17th. I have updated the description of the PR accordingly. |
@stanislaw could you confirm the corporate forms were sent? I just checked on this side and all we have are your two individual forms. |
Yes, I confirm that we have sent the email |
@stanislaw Confirmed reception on our side, thank you for your patience! |
@jphickey are the issues you had with this resolved? |
There was a problem hiding this 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.
CCB 20200415 - APPROVED |
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
toCFE_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:
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
The corporate CLA has been signed on Feb 17th and sent to the email address as specified in the CLA document.