Re: RTEMS/osdMessageQueue.c

[Michael Davidsaver via Core-talk <core-talk at aps.anl.gov>]

On 5/25/20 8:45 PM, Johnson, Andrew N. wrote:
> On May 25, 2020, at 1:37 PM, Michael Davidsaver <mdavidsaver at gmail.com <mailto:mdavidsaver at gmail.com>> wrote:
>>
>> It looks like the switch to ci-scripts for Base added TEST=NO for the RTEMS builds.
>> This needs to be removed.
>>
>> On 5/25/20 10:49 AM, Johnson, Andrew N. via Core-talk wrote:
>>> tl;dr:  I propose that we delete the RTEMS implementation of osdMessageQueue before making the 7.0.3.2 release, it has bugs which the default implementation doesn’t show.
>>
>> I don't have a strong objection to this.  Although, I'm not enthused to be
>> making this change days before a release, when it will go out the door with
>> very little testing.  Whatever is going on, I suspect that it isn't a
>> regression (the bugs in the default impl weren't).
>>
>> How about marking your new test as a skip until after the release?
> 
> Earlier tonight I pushed commits removing “TEST=NO” from the RTEMS builds and implementing the test skips as you suggested, although I would prefer that we have a fix in place for the release. Reducing the amount of code in Base would seem a better solution to me than keeping a known-buggy implementation.

Maybe this is a case of "devil you know".  Also, and you knew I would go here,
we can do another release in a month or so.  I would much rather we released
according to a work schedule, rather than working to a release schedule
as we have been doing.

> However unfortunately the RTEMS builds didn’t run ‘make test-results’ after generating the tapfiles, so although the builds have finished I have no idea what the results were.

Ha.  "grep: /configure/CONFIG_BASE_VERSION: No such file or directory"
Of course the Base build does not define '$EPICS_BASE' as a dependency.
Another special case for ci-scripts.


>>> The long version:
>>>
>>> While merging 3.15 into 7.0 last night I ran the tests on RTEMS (qemu) and discovered that the implementation of the epicsMessageQueue fails the additional tests that I wrote for this API. I was expecting the Travis-CI builds to fail but
>>
>>> I forgot that they don’t actually run the tests at all.
>>
>> I guess there is something to be said for developing on the development branch.
> 
> As an FYI, that merge up from 3.15 was made many times harder than the one before it by the api-headers change. Git didn't recognize /any/ of the C/C++ source changes from the 3.15 branch as applying to files on the 7.0 branch that had earlier been moved into the modules/ structure, so I had to do the merges by hand. I don’t intend to do that again, so unless someone else agrees to pick up the back-porting work I think Base-3.15.8 may have been our last 3.15 release.

It shouldn't surprise anyone that I'm in favor of this.

That said.  I tried this merge for myself and it seemed to go well.
This is with git 2.20.1 with the diff.renameLimit option set to '0'.

Merging origin 3.15 (e923790c41e208304ea9fba92e5e068c69ce0e4b) into 7.0 (b2fb83179a5e533adb3ae892d2e0b326fb3b65b3)

> $ git merge origin/3.15 
> Performing inexact rename detection: 100% (1806215/1806215), done.
> Auto-merging modules/libcom/test/epicsStackTraceTest.c
> CONFLICT (content): Merge conflict in modules/libcom/test/epicsStackTraceTest.c
> Auto-merging modules/libcom/src/osi/os/default/osdMessageQueue.cpp
> Auto-merging modules/libcom/src/osi/os/RTEMS/osdThread.c
> Auto-merging documentation/ReleaseChecklist.html
> CONFLICT (content): Merge conflict in documentation/ReleaseChecklist.html
> Auto-merging documentation/RecordReference.md
> Auto-merging documentation/RELEASE_NOTES.md
> CONFLICT (content): Merge conflict in documentation/RELEASE_NOTES.md
> Auto-merging documentation/KnownProblems.html
> CONFLICT (content): Merge conflict in documentation/KnownProblems.html
> Auto-merging configure/CONFIG_BASE_VERSION
> CONFLICT (content): Merge conflict in configure/CONFIG_BASE_VERSION
> Automatic merge failed; fix conflicts and then commit the result.

> $ git status
...
> Changes to be committed:
> 
>         modified:   documentation/RecordReference.md
>         modified:   modules/libcom/src/osi/os/RTEMS/osdThread.c
>         modified:   modules/libcom/src/osi/os/default/osdMessageQueue.cpp
>         modified:   modules/libcom/test/epicsMessageQueueTest.cpp
...
> Unmerged paths:
>   (use "git add <file>..." to mark resolution)
> 
>         both modified:   configure/CONFIG_BASE_VERSION
>         both modified:   documentation/KnownProblems.html
>         both modified:   documentation/RELEASE_NOTES.md
>         both modified:   documentation/ReleaseChecklist.html
>         both modified:   modules/libcom/test/epicsStackTraceTest.c




>> As I look at epicsMessageQueueTest.cpp I don't see any test which
>> deliberately times out.
> 
> The new SleepySender and SleepyReceiver tests do actually do that, just not obviously. I suspect there may be one or two other APIs in libCom with timeouts which don’t have tests for the timeout operations – a Virtual Codeathon job maybe…
> 
> 
> Heinz, If there is a measurable performance advantage in providing an implementation of the epicsMessageQueue based on Posix message queues I don’t have an objection in our providing one for other OSs, but we would have to work out how to exclude Darwin builds from the Posix rule since MacOS X doesn’t have a mqueue.h header at all.
> 
> I do remember that there are a few places in the default implementation where in the course of a single send or receive operation there may be multiple task switches that could occur between the sending and receiving threads. However if there isn’t likely to be much of a performance gain I don’t know whether it’s worth spending much time on this.
> 
> - Andrew
> 
> -- 
> Complexity comes for free, simplicity you have to work for.
>