On 5 June 2018 at 00:31, Joel Sherrill <j...@rtems.org> wrote: > I will add that covoar was originally designed to generate a report on one > set at a time. The iteration over symbol sets was done at the scripting > level above that. > > This had the advantage of being simple at the time. It may still be simple > but moving the iteration over sets to covoar will probably be faster. > > I think DesiredSymbols is instanced a single time. As a starting point, > there > would have to be multiple instances of this -- one for each symbol set. > > Beyond that, there is a correlation between the report generated and > the desired symbols set. > > So I am thinking that we need to define a "context" structure for each > set. One simple thought is that there is one "master/unified" DesiredSymbol > set under the hood and the symbols in each set are used as a filter > when generating reports. So process the executables for every symbol > but generate the report subdirectories based on one of the sets of > DesiredSymbols. > > I think that should work. > > Cillian.. you have been through the flow. Am I thinking right that it is > > And I think we need to merge before doing this type of work. If we can > process > a single set correctly, that's a baseline. Adding iteration will be easier > to review > as another patch. > > we can process a single set correctly. Shall we proceed with merging the above patch with the suggested small edits and then file a ticket for the iteration and then start working on it ?
I am confused with running of coverage for multiple bsp. If a single report.html has to show the reports of multiple bsps (as hinted by Cillian) Then a lot of rewroking would be needed. If we're headed in that way then I think making separate report.html like leon3-report.html would be simpler to achieve, and then create a master index.html for listing all the report htmls. > > On Mon, Jun 4, 2018 at 1:43 PM, Cillian O'Donnell <cpodonne...@gmail.com> > wrote: > >> >> >> On 4 June 2018 at 19:03, Vijay Kumar Banerjee <vijaykumar9...@gmail.com> >> wrote: >> >>> On 2 June 2018 at 01:00, Vijay Kumar Banerjee <vijaykumar9...@gmail.com> >>> wrote: >>> >>>> On 2 June 2018 at 00:48, Joel Sherrill <j...@rtems.org> wrote: >>>> >>>>> >>>>> >>>>> On Fri, Jun 1, 2018, 11:21 AM Vijay Kumar Banerjee < >>>>> vijaykumar9...@gmail.com> wrote: >>>>> >>>>>> On 1 June 2018 at 20:30, Gedare Bloom <ged...@rtems.org> wrote: >>>>>> >>>>>>> On Fri, Jun 1, 2018 at 10:28 AM, Vijay Kumar Banerjee >>>>>>> <vijaykumar9...@gmail.com> wrote: >>>>>>> > On 1 June 2018 at 19:24, Joel Sherrill <j...@rtems.org> wrote: >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> On Fri, Jun 1, 2018 at 2:46 AM, Vijay Kumar Banerjee >>>>>>> >> <vijaykumar9...@gmail.com> wrote: >>>>>>> >>> >>>>>>> >>> Here's the list of Ideas for improvements: >>>>>>> >>> >>>>>>> >>> 1. Include the section coverage in the bsp config file. >>>>>>> >>> If the section is not found then the script will show >>>>>>> >>> proper error showing coverage is not supported for the >>>>>>> >>> provided bsp config file. >>>>>>> >>> >>>>>>> >>> 2. Update covoar to add support for separate coverage report >>>>>>> >>> for each symbol set. >>>>>>> >>> >>>>>>> >>> 3. Add a method somewhere in covoar to get the size of an >>>>>>> instruction >>>>>>> >>> and fix the hard coded size 4 in ObjdumpProcessor.cc >>>>>>> >> >>>>>>> >> >>>>>>> >> What about a single XXX_run command? What about that suggestion? >>>>>>> >> >>>>>>> > The suggestion was to turn test_run and coverage_run into a single >>>>>>> command. >>>>>>> > I have kept them separate so that there's a possibility to just >>>>>>> run the >>>>>>> > test. >>>>>>> > >>>>>>> > If we want to run coverage everytime we run the test. we can do it. >>>>>>> > Then I think the --coverage option can be changed to >>>>>>> --coverage-sets >>>>>>> > to mention the sets. >>>>>>> > If that's what we're looking for then I don't think a separate >>>>>>> ticket is >>>>>>> > needed, >>>>>>> > I can try to do it within tomorrow and submit an updated patch. >>>>>>> > >>>>>>> >> >>>>>>> >> Will there be an update to this patch? >>>>>>> >> >>>>>>> > This patch is working an showing results. I don't have any work >>>>>>> > going related to this patch currently. >>>>>>> > If there are any suggestions, I'll try to include all the >>>>>>> suggested updates >>>>>>> > as soon as possible and submit. So that we can get it merged. >>>>>>> > >>>>>>> >>>>>>> I get confused by the similarity between test_run() and >>>>>>> coverage_run() >>>>>>> names, and now I'm also seeing some confusion because there is a >>>>>>> function coverage_run() and a class coverage_run. I suggest you >>>>>>> remove >>>>>>> this function coverage_run(), and make either coverage_run.__init__() >>>>>>> or coverage_run.run() take the executables as a parameter directly. >>>>>>> >>>>>>> Thank you for the suggestion. :) >>>>>> I have removed the function and taken executables as a parameter in >>>>>> coverage_run.__init__() >>>>>> >>>>>> I have a question. >>>>>> The ini file that is fed to covoar is written by the script according >>>>>> to the >>>>>> symbols mentioned by the user. I haven't include the ini file in the >>>>>> patch. >>>>>> I'm planning to delete the file after the run, unless --no-clean >>>>>> option is given, >>>>>> which currently deletes the .cov trace files after the run. >>>>>> >>>>> >>>>> That makes sense. >>>>> >>>>> >>>>>> Can I proceed with this ? >>>>>> >>>>> >>>>> Yes. >>>>> >>>> Added. Thanks. :) >>>> >>>>> also, shall I include that in the .gitignore ? >>>>>> >>>>> >>>>> Is the name of the file constant? The same across multiple BSPs? If >>>>> so, then this will be a problem doing automated testing of multiple BSPs >>>>> in >>>>> parallel. >>>>> >>>>> The ini file I'm talking about is the symbol sets config file not the >>>> bsp >>>> config file. yes the name is constant. Should it be unique to the bsp ? >>>> something like, leon3-symbols.ini ? >>>> How does the automated testing work? >>>> >>>>> I think the name needs to be unique enough.to support running testing >>>>> with coverage on multiple BSPs in parallel. That means you can't add it to >>>>> gitignore. And can add another issue and FIXME to the code. >>>>> >>>>>> If it is needed then I have a fix in mind. I can take the bsp name >>>> and add >>>> '-symbols.ini' to it. and add *-symbols.ini to .gitignore . >>>> >>> Shall I add this or put a fixme in the code and post a patch ? >>> Are there any other suggestions for the patch ? >>> >>> I was looking into covoar for generating separate reports for each >>> symbolset. >>> Seems like all the coverage reports are generated together and are >>> written >>> in the _outputDirectory_ . >>> >> >> The output directory should be aligned with the bsp and the report.html >> changed to keep record of this instead of searching for the generic >> coverage dir. Look for leon3-coverage and so on. As well as the >> symbol-sets.ini change you mentioned above. That would probably be enough >> to not cause any conflicts with parallel testing (I may be missing a case >> there, I let you know if anything else comes to mind). The main thing to >> think about is if multiple bsps are being tested at the same time, they >> have to know which config file is there's and which output directory and >> whatever else they may be looking for, the names have to be unique. These >> things are all fed to covoar so the changes can be added to coverage.py. >> >> I couldn't figure out how to cleanly address this. >>> If covoar is intended to generate reports from multiple >>> subsystems/symbolsets, >>> then I think this would be a needed update. Otherwise we can do it from >>> the >>> script, by feeding covoar with a single set ini and putting the result >>> in a separate >>> directory . >>> >>>> Can we do this ? >>>> >>>>> >>>>>> >>>>>>> -Gedare >>>>>>> >>>>>> >>>>>> >>>> >>> >>> _______________________________________________ >>> devel mailing list >>> devel@rtems.org >>> http://lists.rtems.org/mailman/listinfo/devel >>> >> >> >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
