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

Allow out-of-tree psp paths #134

Open
matzipan opened this issue Jan 22, 2020 · 16 comments
Open

Allow out-of-tree psp paths #134

matzipan opened this issue Jan 22, 2020 · 16 comments
Labels
enhancement New feature or request

Comments

@matzipan
Copy link

Is your feature request related to a problem? Please describe.
Requiring a PSP to always be defined inside the PSP repository means holding a set of patches on top of the upstream version. This adds friction when updating to latest version.

Describe the solution you'd like
At least initially, CFE_SYSTEM_PSPPATH should be introduced. It should be configurable and it should allow arbitrary paths to PSP files.

@skliper
Copy link
Contributor

skliper commented Feb 7, 2020

Got another request/use case for this today. Short term preference is to allow this option (no major architectural changes).

@skliper skliper added this to the 1.5.0 milestone Feb 7, 2020
@skliper skliper added the enhancement New feature or request label Feb 7, 2020
@jphickey
Copy link
Contributor

Can you share details of the use-case?

My position is still that since we don't really ensure stability of the PSP API in the same way we do for the App-facing API it would still be better off for custom PSP's to be put directly inside the PSP directory, rather than outside the tree, as it would need to be versioned with the PSP code. This can be simply a local git branch that contains your custom PSP.

If we really do need to support out-of-tree PSP's then I would rather extend the existing search path to also cover PSP's rather than inventing yet another option for it. There are already too many undocumented configuration options (as many have pointed out elsewhere).

@skliper
Copy link
Contributor

skliper commented Feb 10, 2020

We've got plenty of elements that don't support cross versioning, and we manage them in separate repos. Why force users to stay within the PSP repo (branches or whatever) just to manage versioning? I definitely don't want to force users to branch the cFS Framework PSP repo just two write their own PSP implementation. It should be trivial, as well as easy for others to open source/share their implementations without being explicitly tied to the framework repo (version, yes... repo, no).

If there's a better way to support user implemented PSP out of the PSP tree (extend existing search path), then please provide the details. This existing search path you speak of likely isn't well known to users.

@matzipan
Copy link
Author

as it would need to be versioned with the PSP code.

It can still be versioned together if the PSP code is included as a git submodule in a larger repository.

This can be simply a local git branch that contains your custom PSP.

Having a branch for this means that you constantly have to rebase your changes on top of latest fixes. Whereas using the PSP code as a submodule would simplify this to a simple git checkout.

@matzipan
Copy link
Author

Having a branch for this means that you constantly have to rebase your changes on top of latest fixes.

Besides, this has the extra annoyance of having to keep track of two remotes: one is github/nasa/psp, and the other is the internal repository where the changes would be kept on top.

@jphickey
Copy link
Contributor

To each his own I guess, I find submodules to be far more problematic/difficult to use in a real deployment versus using using a subtree approach. But I won't stand in the way if you really want this.

My only contention (and the point was somewhat confirmed with the earlier comment) that we shouldn't add yet another undocumented purpose-specific option for this, which we already have too many of. There is already an environment variable CFS_APP_PATH and an associated search implementation inside mission_build.cmake. This is currently used for locating all apps and supports apps being out-of-tree.

Here is what I would like to see for a generic solution:

  • Extend the CFS_APP_PATH to also cover PSP implementations, rather than adding another single-purpose option flag.
  • PSP build should use an individual CMakeLists.txt for each PSP rather than doing an aux_source_directory at the top level. The latter only works because the current 3 have a nearly identical pattern and are tightly coupled in the same repo - with more PSPs which are more loosely coupled I can see this becoming a problem. This could be a follow-on but I think it has to happen at some point.

@jphickey
Copy link
Contributor

As a side note/related issue we also have to consider how this affects the documentation build.

Right now the PSP is actually included in the doxygen runs of both the "cfe-usersguide" and the "mission-detaildesign". The inclusion in the users guide seems to be a bit of a hack and uses a direct directly reference so the proposed change for out-of-tree will NOT work here.

My suggestion would be to put the PSP doxygen information into the detail design document only, and take it out of the CFE users guide document. This would be more consistent with how we include the OSAL and app documentation, which AFAIK only gets merged into the mission detail design document.

@skliper
Copy link
Contributor

skliper commented Feb 10, 2020

The API needs to go in either a PSP API document or remain in the cfe-usersguide.

The mission implementation (links to possible out of tree implementation) can go in mission-detaildesign.

@skliper
Copy link
Contributor

skliper commented Feb 10, 2020

Here is what I would like to see for a generic solution:

  • Extend the CFS_APP_PATH to also cover PSP implementations, rather than adding another single-purpose option flag.
  • PSP build should use an individual CMakeLists.txt for each PSP rather than doing an aux_source_directory at the top level. The latter only works because the current 3 have a nearly identical pattern and are tightly coupled in the same repo - with more PSPs which are more loosely coupled I can see this becoming a problem. This could be a follow-on but I think it has to happen at some point.

I agree with both. I'd think abstracting into everything being a PSP "module", and just select which pieces you want (at the target config level) may also be worth considering. Basically build x, y, and z to create my target PSP.

@jphickey
Copy link
Contributor

I'd think abstracting into everything being a PSP "module", and just select which pieces you want (at the target config level) may also be worth considering

I like this a lot. Note that the search path is already employed for the existing "module" concept so one could do this even for the basic/core PSP functions and I think it would (almost) just work with the current code.

The only potential gotcha I can currently think of is handling the entry point, but if we transition to utilizing the existing OSAL BSP to serve as the entry point then I think that covers that issue. (nasa/osal#261)

@jphickey
Copy link
Contributor

The API needs to go in either a PSP API document or remain in the cfe-usersguide.

The mission implementation (links to possible out of tree implementation) can go in mission-detaildesign.

What if we include the include directory (i.e. psp/fsw/inc) in the doxygen for cfe-usersguide, which should cover the actual API, but not the implementation-specific bits. This directory should remain constant even if an out-of-tree PSP is used.

@skliper
Copy link
Contributor

skliper commented Feb 10, 2020

The API needs to go in either a PSP API document or remain in the cfe-usersguide.
The mission implementation (links to possible out of tree implementation) can go in mission-detaildesign.

What if we include the include directory (i.e. psp/fsw/inc) in the doxygen for cfe-usersguide, which should cover the actual API, but not the implementation-specific bits. This directory should remain constant even if an out-of-tree PSP is used.

Exactly. Need to clean up references to implementation-specific bits, but that needs to be fixed either way.

@skliper
Copy link
Contributor

skliper commented Jul 29, 2020

With the module framework in place, how much work would it be to just treat PSP like any other module that gets linked to the core?

@jphickey
Copy link
Contributor

With the module framework in place, how much work would it be to just treat PSP like any other module that gets linked to the core?

To some degree, it already is treated that way. At this point, CFE apps/libs and PSP modules are handled by the build system quite similarly, differing only in how its linked and installed (static vs. dynamic) and flags used during compile.

The main thing I'd (still) like to do for the next version is to make the PSP fully modular, so even the startup logic and other stuff are just modular blobs that can be mixed and matched. This would not be hard to do, and would allow much easier extension and overriding/replacement/customization of PSP functions without forking the whole PSP.

@skliper
Copy link
Contributor

skliper commented Jul 29, 2020

I guess what I'm asking is how hard would it be to update such that we could remove "psp" from MISSION_CORE_MODULES, replace with the psp that you want (say psp-pc-linux), add psp/fsw to the MISSION_MODULE_SEARCH_PATH and have everything work? Then if a project wants to go out of tree, mix and match, extend, etc they could add their own psp module (or pick and choose existing modules) add the out of tree path to MISSION_MODULE_SEARCH_PATH if they want, etc. Basically eliminate the special handling and CMake logic around psps. Treat it as just another library that gets linked to the core...

I think we are on similar paths? Could make the api implementations into PSP modules, assemble the entire PSP from say "psp-pc-linux" (as a framework example) that really just picks what modules to add.

@skliper
Copy link
Contributor

skliper commented Jul 29, 2020

Not a priority this round... but path forward.

@skliper skliper removed this from the 1.5.0 milestone Aug 21, 2020
@skliper skliper linked a pull request Oct 21, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants