nasa / fprime-tools

F´ Python tooling and helpers.
https://github.com/nasa/fprime
Apache License 2.0
20 stars 38 forks source link

fix cookiecutter new proj template #153

Closed saba-ja closed 1 year ago

saba-ja commented 1 year ago
Originating Project/Creator fprime-tools
Affected Component cookiecutter_templates
Affected Architectures(s) N/A
Related Issue(s) N/A
Has Unit Tests (y/n) No
Builds Without Errors (y/n) Yes
Unit Tests Pass (y/n) N/A
Documentation Included (y/n) No

Change Description

Cookiecutter's "new project" CMakeLists.txt template had "CMAKE_CURRENT_LIST_DIR" for FPrime.cmake file as shown below:

include("${FPRIME_FRAMEWORK_PATH}/cmake/FPrime.cmake")

This works fine if fprime folder is inside the project folder, but won't work if the fprime repo is outside of the project folder.

With the new fix it does not matter where the fprime folder is located as the path will be found from FPRIME_FRAMEWORK_PATH.

include("${FPRIME_FRAMEWORK_PATH}/cmake/FPrime.cmake")

Rationale

There is an inconsistency in the generated CMakeLists.txt for the new projects by the cookiecutter. both FPrime.cmake and FPrime-Code.cmake are in the cmake folder in the fprime repo, but one is being search in CMAKE_CURRENT_LIST_DIR/fprime and the other is being searched in ${FPRIME_FRAMEWORK_PATH}/cmake

Testing/Review Recommendations

Created a new project with the cookiecutter CLI and got the following CMakeLists.txt Also was able to generate and build the project with the new path.

####
# This sets up the build system for the 'MyProject' project, including
# components and deployments from project.cmake. In addition, it imports the core F Prime components.
####

cmake_minimum_required(VERSION 3.13)
project(MyProject C CXX)

###
# F' Core Setup
# This includes all of the F prime core components, and imports the make-system.
###
include("${FPRIME_FRAMEWORK_PATH}/cmake/FPrime.cmake")
# NOTE: register custom targets between these two lines
include("${FPRIME_FRAMEWORK_PATH}/cmake/FPrime-Code.cmake")

# This includes project-wide objects
include("${CMAKE_CURRENT_LIST_DIR}/project.cmake")

Future Work

The cookiecutter currently clones the fprime repository inside the project base folder. It would be more convenient to update the cookiecutter to prompt the user for the fprime folder's location. It can then search for the folder, and if found, there would be no need to clone it again. However, if the fprime folder is not found, the cookiecutter should proceed to clone the repository into the given location. Additionally, to accommodate users who prefer to install fprime in the project folder, the cookiecutter can have the default option of placing it in the project root folder.

thomas-bc commented 1 year ago

At the CMake level, FPRIME_FRAMEWORK_PATH is not set when that first include("${FPRIME_FRAMEWORK_PATH}/cmake/FPrime.cmake") is called. This still works with fprime-util because it will parse and feed that variable in to CMake. But if you were to run pure cmake without fprime-util (e.g. in an IDE), this would result in an error (unless you pass it in manually). So rather than an inconsistency, I would call that a debatable design choice. The only downside I can find to keeping it like it currently is, is that if you were to move the fprime/ submodule around, one would have to modify this relative path as well. Am I missing anything else?

Looking at our tutorials and reference suite, it looks like we're kind of using both approaches (see LedBlinker vs SystemReference). We may want to converge on a standard practice. I'm not opposed to taking that change in - I just want to make sure we are on the same page.

@LeStarch thoughts?

saba-ja commented 1 year ago

@thomas-bc, Thanks for the feedback!

As you pointed out, there is indeed no guarantee that a fprime repository will be directly in the root directory of a project. We have observed fprime repositories being placed in various locations based on the project's structure and needs. F Prime tools can either impose restrictions on the subfolder's location, leaving it up to the user to handle manually, or we can update the tool to offer users the flexibility to choose the subrepo's location.

In general, if this turns out to be a complex and potentially problematic issue, we can dismiss this pull request. I am also open to workout our way to update the tool to support this change for both pure cmake call and fprime-util if I can get some pointers to where to start.

LeStarch commented 1 year ago

fprime-util new --project both creates the cookie-cutter and clones F´. Thus, either of the above solutions (original, updated) would both work? The template knows where it will be put, and standard practice is to run with fprime-util that properly sets variables.

Thus the trade is this:

  1. Make IDE easier at the expense of making custom F´ locations harder
  2. Make custom F´ locations harder at the expense of IDE configuration
LeStarch commented 1 year ago

@saba-ja in order to run into this issue, did you run fprime-util new --project and then move the fprime checkout somewhere else?

LeStarch commented 1 year ago

One possible solution would be to have an fprime-util setup-ide command that generates IDE configuration files. This would allow us to specify the appropriate variables for IDEs and make running IDEs much easier.

saba-ja commented 1 year ago

Closing the PR for a favor of a more completed solution. The Future work would be to add option fprime-util setup-ide to generate IDE configuration files.