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

Fix arguments order for generate stage #165

Merged
merged 5 commits into from
Oct 11, 2023

Conversation

SMorettini
Copy link
Contributor

Originating Project/Creator
Affected Component fprime-util generate

Change Description

Cmake arguments come from three different places:

  1. default
  2. settings.ini
  3. user-defined arguments when running fprime-util generate

Before this change, the order was the default, user-defined and settings.ini. Now I changed the order as reported above.

Rationale

I was trying to set the -DCMAKE_BUILD_TYPE=Release during the generation of the unit tests but I was not able to send anything else than Debug. This was happening because when the arguments were read from settings.ini, the build type was put by default to Debug. This overrides the build type set from the command line.

With my change, the user-defined variable when running fprime-util generate ... overrides any other variable.

thomas-bc
thomas-bc previously approved these changes Sep 7, 2023
Copy link
Contributor

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

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

LGTM! Agreed that CLI args should take precedence.

@thomas-bc
Copy link
Contributor

thomas-bc commented Oct 9, 2023

@SMorettini turns out the order of dictionary unpacking was a conscious decision, as users should not override the FPRIME_* variables from get_cmake_args() through the command line, but instead use the settings.ini file. Otherwise issues can arise. This means CMAKE_BUILD_TYPE should not be set as part of the get_cmake_args() call, but rather at the generate() level.

I went ahead and implemented this in your PR branch, I hope you don't mind. Would you want to give this a quick review?

I tested locally and it should behave as you requested. Would very much appreciate some additional eyes on this though!
Thanks!

@thomas-bc
Copy link
Contributor

Merging, let me know if you have any comments!

@thomas-bc thomas-bc merged commit 85e7fb0 into nasa:devel Oct 11, 2023
28 checks passed
Comment on lines +407 to +410
cmake_args["BUILD_TESTING"] = "ON"
cmake_args["CMAKE_BUILD_TYPE"] = user_cmake_args.get(
"CMAKE_BUILD_TYPE", "Debug"
)
Copy link
Contributor Author

@SMorettini SMorettini Oct 13, 2023

Choose a reason for hiding this comment

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

@thomas-bc I originally opened this pull request to give the possibility to build UTs using a CMAKE_BUILD_TYPE different from Debug. I agree with your changes but if I understand correctly the code, you reintroduced back the problem I wanted to remove.

Am I right or the CMAKE_BUILD_TYPE will be always Debug if the UTs are built? Is that something that we want by design? I think that the user should have the flexibility to choose the CMAKE_BUILD_TYPE in any case.
If by design we don't want to give this flexibility then we should document it somewhere because, at least for me, it seems a hidden and unexpected feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thomas-bc do you have any comments about this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For newer versions of F´ (v3+) it only defaults to debug if the user has not set "CMAKE_BUILD_TYPE"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tested this and a custom build type does carry into a UT cache!

@SMorettini SMorettini deleted the Fix-argument-order-for-generate branch January 9, 2024 13:56
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