search

nasa / osal

The Core Flight System (cFS) Operating System Abstraction Layer (OSAL)
Apache License 2.0
557 stars 218 forks source link

Fix #1440, Split up BinSemGetInfo() to avoid partial success returns #1480

Open thnkslprpt opened 1 month ago

thnkslprpt commented 1 month ago

Checklist

Describe the contribution

Testing performed GitHub CI actions all passing successfully (incl. Build + Run, Unit/Functional Tests etc.). Tested locally to confirm no loss of coverage compared with the current main branch, and all lines/branches/functions in the new version are covered.

Expected behavior changes Removes this source of 'partial success' which can be confusing and also increases complexity for dealing with the return for users calling into this API.

System(s) tested on Debian 12 using the current main branch of cFS bundle.

Contributor Info Avi Weiss   @thnkslprpt

thnkslprpt commented 1 month ago

Pretty simple change-set, although the change is 'breaking' I guess, so has to wait for a major release?

Note - there is a single reference to BinSemGetInfo() in cFE which will need to be updated along with this if it is merged.

skliper commented 1 month ago

Pretty simple change-set, although the change is 'breaking' I guess, so has to wait for a major release?

Note - there is a single reference to BinSemGetInfo() in cFE which will need to be updated along with this if it is merged.

I recommend just deprecating the old API vs fully removing, then it doesn't have to be a major release (at least per the old rules I used). See other uses of OSAL_OMIT_DEPRECATED.

thnkslprpt commented 1 month ago

I recommend just deprecating the old API vs fully removing, then it doesn't have to be a major release (at least per the old rules I used). See other uses of OSAL_OMIT_DEPRECATED.

Good point. Will update it.

thnkslprpt commented 1 month ago

@skliper I'm not sure if I did the deprecation guards correctly, but do you know if it's possible or have the debug and release builds pass the tests given that the deprecated elements are only removed in the debug workflow? https://github.com/nasa/osal/blob/827016689c1acba664843f18c4081be4dc174743/.github/workflows/standalone-build.yml#L60

Is it trying to run the same set of tests, but with and without the deprecated code? (that doesn't seem to make sense right)

(I'm assuming we only upkeep tests for the non-deprecated code...)

skliper commented 1 month ago

@skliper I'm not sure if I did the deprecation guards correctly...

I'm not really following your full question, but I recommend only wrapping the old API in the omit deprecated ifndef not the new stuff. Then hopefully the tests would pass?

thnkslprpt commented 1 month ago

@skliper I'm not sure if I did the deprecation guards correctly...

I'm not really following your full question, but I recommend only wrapping the old API in the omit deprecated ifndef not the new stuff. Then hopefully the tests would pass?

OK all good now - it wasn't building for me locally but that might have been a symptom of my own build command. Tests passing with a small addition as per the above comment.