fix #759 - deprecates GetLastSenderId() by CDKnightNASA · Pull Request #784 · nasa/cFE

CDKnightNASA · 2020-07-27T19:08:21Z

Describe the contribution
Fixes #759 - deprecates CFE_SB_GetLastSenderId() API.
Fixes #608

Testing performed
make with and without OMIT_DEPRECATED defined.

System(s) tested on
Debian 10.3

Contributor Info - All information REQUIRED for consideration of pull request
[email protected]

jphickey

Looks OK to me.

Two minor nitpicks though:

The existing OMIT_DEPRECATED flags have a CFE prefix.
Also if introducing a new deprecation we need to add it to the global_build_options.cmake file so it gets turned on with OMIT_DEPRECATED=true like the others do.

We might want to revisit the deprecation process WRT the constellation names, as it might make more sense to do e.g. OMIT_DEPRECATED_BOOTES instead of using CFE versions here.

jphickey · 2020-07-27T21:44:30Z

Another note - I believe for other APIs that have been deprecated, rather than clutter up the unit test code with #ifdef directives - we can just remove it outright, as (AFAIK) there is no requirement to unit test deprecated functions. UT code is ugly enough already.

CDKnightNASA · 2020-07-27T22:10:55Z

Looks OK to me.

Two minor nitpicks though:

The existing OMIT_DEPRECATED flags have a CFE prefix.

Whoop, don't know how I missed this!

Also if introducing a new deprecation we need to add it to the global_build_options.cmake file so it gets turned on with OMIT_DEPRECATED=true like the others do.

Copy.

I've squashed updates.

CDKnightNASA · 2020-07-27T22:12:18Z

Another note - I believe for other APIs that have been deprecated, rather than clutter up the unit test code with #ifdef directives - we can just remove it outright, as (AFAIK) there is no requirement to unit test deprecated functions. UT code is ugly enough already.

Done, the stubs remain (of course) but are likewise #ifndef'd.

skliper · 2020-07-28T21:38:14Z

Moved the requirement back to "submitted" in requirement tracking tool. Will need to export and update text requirements.

CDKnightNASA · 2020-07-28T23:25:52Z

Moved the requirement back to "submitted" in requirement tracking tool. Will need to export and update text requirements.

Would it be worthwhile to export the requirements into an integrated requirements tracking system, like https://pypi.org/project/doorstop/ ?

astrogeco · 2020-07-29T16:34:00Z

CCB 2020-07-29, APPROVED, @skliper to provide doxygen deprecation example.

astrogeco · 2020-07-29T16:34:53Z

@CDKnightNASA can you move the cmake 6_8 definition into it's own commit?

skliper · 2020-07-29T17:14:13Z

Example of deprecated doxygen markings:

cFE/fsw/cfe-core/src/inc/cfe_fs.h

Lines 194 to 214 in deeb294

    
           /*****************************************************************************/ 
        
           /** 
        
           ** \brief DEPRECATED; Determines if a file is a Gzip/compressed file. 
        
           ** \deprecated 
        
           ** 
        
           ** \par Description 
        
           **        This API will check the filename and return true if the file is  
        
           **        a gzip file. The check is currently based on the filename, so the  
        
           **        zipped files should use the ".gz" extention.  
        
           ** 
        
           ** \par Assumptions, External Events, and Notes: 
        
           **        -# A gzipped file will use the ".gz" filename extention. 
        
           ** 
        
           ** \param[in] FileName The name of the file. 
        
           ** 
        
           ** \return Boolean for file has ".gz" extension 
        
           ** \retval true  File has ".gz" extension 
        
           ** \retval false File does not have ".gz" extension 
        
           **                
        
           ******************************************************************************/ 
        
           bool CFE_FS_IsGzFile(const char *FileName);

astrogexo · 2020-07-30T12:43:18Z

Moved the requirement back to "submitted" in requirement tracking tool. Will need to export and update text requirements.

Would it be worthwhile to export the requirements into an integrated requirements tracking system, like https://pypi.org/project/doorstop/ ?

I looked into it and it looks interesting but I'm not sure what it buys us. Have you used it?

skliper · 2020-07-30T19:58:44Z

Would need to trade against the internal tools and process requirement compliance. We get some things for "free" using standard internal processes, but that doesn't mean it's better.

CDKnightNASA · 2020-07-31T19:22:00Z

Moved the requirement back to "submitted" in requirement tracking tool. Will need to export and update text requirements.

Would it be worthwhile to export the requirements into an integrated requirements tracking system, like https://pypi.org/project/doorstop/ ?

I looked into it and it looks interesting but I'm not sure what it buys us. Have you used it?

I have not, but some folks at the 2019 FSW mentioned they used it. Certainly would need to do a study of it.

CDKnightNASA · 2020-07-31T19:31:22Z

@CDKnightNASA can you move the cmake 6_8 definition into it's own commit?

done

CDKnightNASA · 2020-07-31T19:31:56Z

CCB 2020-07-29, APPROVED, @skliper to provide doxygen deprecation example.

done, ready for review again

astrogeco · 2020-08-05T13:14:49Z

@skliper this looks good to me, are we good to merge?

skliper · 2020-08-05T13:18:56Z

Can this go in the 6.8 dev cycle? It's wrapped with the 6.8 omit define. Other than that, yes looks good to me.

skliper

Recommend merge to 6.8 development

astrogeco · 2020-08-05T13:21:07Z

Recommend merge to 6.8 development

If you you mean make this part of 6.8 then yes!

skliper · 2020-08-05T13:24:48Z

No, I mean development on top of 6.8. Otherwise the OMIT numbering breaks convention. I'm requesting making the rc tags BEFORE merging this change.

skliper · 2020-08-05T13:27:11Z

Marked my preference as targeting this for 7.0 release. Sorry, my quick comments weren't very clear.

astrogeco · 2020-08-05T13:27:11Z

The milestone helps, thanks!

astrogeco · 2020-08-05T13:28:29Z

What's the convention then? Should the deprecate tag then say deprecate 7_0 or 7_1?

skliper · 2020-08-05T13:29:47Z

What's the convention then? Should the deprecate tag then say deprecate 7_0 or 7_1?

6_8.

astrogeco · 2020-08-05T13:42:29Z

What's the convention then? Should the deprecate tag then say deprecate 7_0 or 7_1?

6_8.

For completeness then, does DEPRECATE_6_8 means that 6.8 was the last version which supports this function?

skliper · 2020-08-05T13:47:08Z

We've typically just marked with the last released version, so it could be interpreted that way but I'm not aware of any hard rules or published deprecation process. See nasa/cFS#67.

jphickey · 2020-08-05T13:48:41Z

does DEPRECATE_6_8 means that 6.8 was the last version which supports this function?

Yes, that is the paradigm we've been using up to this point -- the number reflects the last release were it was not deprecated. The idea of putting a version number in the tag was so we'd know which ones had "aged out" in a future release and should be taken out completely.

However as I mentioned before now that we have a codename for API compatibility then maybe we should consider using that instead.

astrogeco · 2020-08-19T13:36:10Z

@CDKnightNASA does this actually fix #608? Just wondering since you don't mention it in the commits

CDKnightNASA · 2020-08-19T14:52:18Z

@CDKnightNASA does this actually fix #608? Just wondering since you don't mention it in the commits

Yes it fixes #608.

CDKnightNASA requested review from jphickey and skliper July 27, 2020 19:08

CDKnightNASA marked this pull request as draft July 27, 2020 19:09

This was referenced Jul 27, 2020

fix #759 - removes GetLastSenderId() #764

Closed

CFE_SB_GetLastSenderId() future deprecation note #772

Closed

jphickey reviewed Jul 27, 2020

View reviewed changes

CDKnightNASA force-pushed the fix-759-getlastsenderid_deprecated branch from cea2fe9 to f0fffbc Compare July 27, 2020 22:09

skliper linked an issue Jul 28, 2020 that may be closed by this pull request

CFE_SB_GetLastSenderID returns pointer to internal data, possible race #608

Closed

astrogeco added the CCB-20200729 label Jul 29, 2020

CDKnightNASA added 2 commits July 31, 2020 12:26

fix nasa#759 - adds CFE_OMIT_DEPRECATED_6_8 tag

144798d

fix nasa#759 - deprecated CFE_SB_GetLastSenderId()

4becb88

CDKnightNASA force-pushed the fix-759-getlastsenderid_deprecated branch from f0fffbc to 4becb88 Compare July 31, 2020 19:30

CDKnightNASA added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jul 31, 2020

CDKnightNASA self-assigned this Jul 31, 2020

astrogeco approved these changes Aug 4, 2020

View reviewed changes

astrogeco marked this pull request as ready for review August 4, 2020 17:44

astrogeco added the IC-20200729 label Aug 5, 2020

astrogeco changed the base branch from master to integration-candidate August 5, 2020 02:32

skliper approved these changes Aug 5, 2020

View reviewed changes

skliper added this to the 7.0.0 milestone Aug 5, 2020

astrogeco removed IC-20200729 CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Aug 5, 2020

astrogeco added CCB:Approved Indicates code review and approval by community CCB IC-20200812 labels Aug 14, 2020

astrogeco mentioned this pull request Aug 14, 2020

Rename OMIT Deprecated flags with cFS codename instead of cFE version #803

Closed

astrogeco merged commit 77ed16d into nasa:integration-candidate Aug 14, 2020

astrogeco mentioned this pull request Aug 18, 2020

Integration Candidate 2020-08-12 #816

Merged

This was referenced Sep 9, 2020

SB Pipe Info 'LastSender' Field is Unused #744

Closed

CFE_SB_GetLastSenderId() returns a pointer to a shared data location #745

Closed

Conversation

CDKnightNASA commented Jul 27, 2020 • edited by skliper Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jphickey left a comment

Choose a reason for hiding this comment

Uh oh!

jphickey commented Jul 27, 2020

Uh oh!

CDKnightNASA commented Jul 27, 2020

Uh oh!

CDKnightNASA commented Jul 27, 2020

Uh oh!

skliper commented Jul 28, 2020

Uh oh!

CDKnightNASA commented Jul 28, 2020

Uh oh!

astrogeco commented Jul 29, 2020

Uh oh!

astrogeco commented Jul 29, 2020

Uh oh!

skliper commented Jul 29, 2020

Uh oh!

astrogexo commented Jul 30, 2020

Uh oh!

skliper commented Jul 30, 2020

Uh oh!

CDKnightNASA commented Jul 31, 2020

Uh oh!

CDKnightNASA commented Jul 31, 2020

Uh oh!

CDKnightNASA commented Jul 31, 2020

Uh oh!

astrogeco commented Aug 5, 2020

Uh oh!

skliper commented Aug 5, 2020

Uh oh!

skliper left a comment

Choose a reason for hiding this comment

Uh oh!

astrogeco commented Aug 5, 2020

Uh oh!

skliper commented Aug 5, 2020

Uh oh!

skliper commented Aug 5, 2020

Uh oh!

astrogeco commented Aug 5, 2020

Uh oh!

astrogeco commented Aug 5, 2020

Uh oh!

skliper commented Aug 5, 2020

Uh oh!

astrogeco commented Aug 5, 2020

Uh oh!

skliper commented Aug 5, 2020

Uh oh!

jphickey commented Aug 5, 2020

Uh oh!

astrogeco commented Aug 19, 2020

Uh oh!

CDKnightNASA commented Aug 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CDKnightNASA commented Jul 27, 2020 •

edited by skliper

Loading