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

adding default cmake settings #105

Merged
merged 3 commits into from
Nov 11, 2022

Conversation

LeStarch
Copy link
Collaborator

Originating Project/Creator
Affected Component
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

Change Description

Enables users to set the default_cmake_options settings.ini field to pass in default cmake options.

Copy link

@kevin-f-ortega kevin-f-ortega left a comment

Choose a reason for hiding this comment

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

Looks good. Nice work! Only a few comments.

src/fprime/fbuild/settings.py Show resolved Hide resolved
test/fprime/fbuild/test_settings.py Show resolved Hide resolved
@LeStarch
Copy link
Collaborator Author

@kevin-f-ortega I added a fix to for the missing test case. Formatting is at the whim of our formatter.

Copy link

@kevin-f-ortega kevin-f-ortega left a comment

Choose a reason for hiding this comment

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

LGTM!

@kevin-f-ortega kevin-f-ortega merged commit c20dc3a into nasa:devel Nov 11, 2022
@LeStarch LeStarch linked an issue Nov 11, 2022 that may be closed by this pull request
@Joshua-Anderson
Copy link
Contributor

Totally late and very optional feedback, but I was wondering if using YAML list instead of newlines to separate options might be a bit cleaner and simpler? I think this is generally a more common YAML practice, I've never actually seen multiline strings without quotes like this before, though it does seem to work fine!

Ex:

default_cmake_options:
    - OPTION1=ABC
    - OPTION2=123
    - OPTION3=Something

instead of:

default_cmake_options: OPTION1=ABC
    OPTION2=123
    OPTION3=Something

I figured I might bring this up before this feature goes into regular use and we have to start thinking about backwards compatibility concerns.

@@ -311,6 +311,9 @@ def get_cmake_args(self) -> dict:
if self.get_settings(setting, None) is not None
}

# Load in the default settings
self.get_settings("default_cmake_options", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

@LeStarch this line doesn't seem to do anything, yet it can be misleading to think that it does (as this could potentially tweak the order in which arguments are loaded in). Can you confirm? If so, I'll remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking at this when reviewing #165. Also I don't disagree with Josh's comment above but it may be a little late to switch

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.

Add Default -D flags to settings.ini
4 participants