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.

Sign up for GitHub

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

Jump to bottom

Fix #265, implement cmake name based targets #776

Merged
merged 1 commit into from
Sep 4, 2020
Merged

Fix #265, implement cmake name based targets #776

merged 1 commit into from
Sep 4, 2020

Conversation

jphickey
Copy link
Contributor

Describe the contribution
Allow targets to be configured in targets.cmake by name, rather than a numeric index. This is more user friendly, allows more configuration flexibility, and cleans up a bunch of extra logic too.

Fixes #265

Testing performed
Build system, sanity check CFE, and run all unit tests

Testing not fully completed - Initially submitting PR for design review/feedback.

Expected behavior changes
No change to FSW, but changes the way targets are defined in targets.cmake.
This includes backward compatibility for existing files/configs, however.

System(s) tested on
Ubuntu 20.04

Additional context
Also related to #710, this cleans up hardcoded CPU IDs that were in the platform config header file.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey mentioned this pull request Jul 10, 2020
Closed
@jphickey jphickey linked an issue Jul 10, 2020 that may be closed by this pull request
Add cmake function to add cpus by function rather than assume consecutive cpu ids. #265
Closed
@jphickey jphickey marked this pull request as draft July 10, 2020 21:12
@jphickey jphickey mentioned this pull request Jul 10, 2020
Merged
@skliper
Copy link
Contributor

skliper commented Jul 13, 2020

Would it make sense to consider transitioning the platform and mission cfe config file into cmake, similar to osconfig?

@jphickey
Copy link
Contributor Author

Would it make sense to consider transitioning the platform and mission cfe config file into cmake, similar to osconfig?

Yes, I think it would! Particularly for some of the CFE TIME options - these could then be different source files rather than #ifdef chunks mixed in everywhere.

But that's a fairly big change to existing paradigms obviously - probably should be a separate steering committee discussion.

@skliper
Copy link
Contributor

skliper commented Jul 13, 2020

The msg module as currently drafted has started in this direction with source selection. Which is clumsy since the rest of cFE uses the header file config.

@astrogeco
Copy link
Contributor

CCB-2020-07-15 - Mark as deprecated for 7.0

@astrogeco astrogeco force-pushed the integration-candidate branch 3 times, most recently from 7c34582 to 343f60d Compare July 26, 2020 04:21
@jphickey jphickey mentioned this pull request Jul 27, 2020
Closed
@jphickey jphickey changed the base branch from integration-candidate to main August 18, 2020 20:26
@jphickey
Fix nasa#265, name-based target configuration
8afeb2a 
Remove requirement to define a sequential/numbered list of targets.
Instead, user defines a list of target names, followed by a set of
properties associated with each name.

This makes it much simpler to consistently define CPUs but selectively
add/remove them from the build based on other criteria.
@jphickey
Copy link
Contributor Author

Update - rebased to current main branch. Should be good to go.

@jphickey jphickey marked this pull request as ready for review August 18, 2020 20:31
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 18, 2020
@astrogeco
Copy link
Contributor

CCB 2020-08-19 Might break CI-Lab, test at the bundle level

@skliper skliper added this to the 7.0.0 milestone Aug 21, 2020
@astrogeco astrogeco added build-system and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Aug 26, 2020
@astrogeco astrogeco changed the base branch from main to integration-candidate September 4, 2020 19:59
@astrogeco astrogeco merged commit aaa27b6 into nasa:integration-candidate Sep 4, 2020
astrogeco added a commit to nasa/cFS that referenced this pull request Sep 4, 2020
@astrogeco
Add nasa/cFE#860 and nasa/cFE#776
a0719a6
astrogeco added a commit to nasa/cFS that referenced this pull request Sep 4, 2020
@astrogeco
Add nasa/cFE#860 and nasa/cFE#776
90013c2
@skliper skliper mentioned this pull request Sep 11, 2020
Closed
@skliper skliper mentioned this pull request Sep 11, 2020
Merged
@jphickey jphickey deleted the fix-265-name-based-target branch September 29, 2020 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skliper skliper skliper left review comments

Assignees
No one assigned
Labels
build-system
Projects
None yet
Milestone
Caelum
Development

Successfully merging this pull request may close these issues.

Add cmake function to add cpus by function rather than assume consecutive cpu ids.
3 participants
@jphickey @skliper @astrogeco